From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x234.google.com (mail-wm0-x234.google.com [IPv6:2a00:1450:400c:c09::234]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id DCA5021E79020 for ; Fri, 18 Aug 2017 11:24:12 -0700 (PDT) Received: by mail-wm0-x234.google.com with SMTP id t201so5877240wmt.1 for ; Fri, 18 Aug 2017 11:26:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=h4MR47W2uIbI1eOjvE/vMrecRXWvVKl8upan8ZnJBqs=; b=PsR3HO6/aYv8dYnfQlizVReq21GfOzm0vBGod0bmfIIeqA7NWkcY76BkyfLFBIw+Qb YyaPBasqmkHsOipnJVmEEwko1lueX672RgHnxL1amDHfSSaf1bmSG5dzcEUXpao9wWOo smYWeF0ZtEQojzlz9f9Yp84ujYkj/ruRACIOM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=h4MR47W2uIbI1eOjvE/vMrecRXWvVKl8upan8ZnJBqs=; b=stghbLSkZeRKGh4o9HGopxTxxUWhbDs7I8mEMB8QsIHUxbst6wdtFL1VTBS3e8dLj+ y9xu7FA8UnMUul4zRkDZDsmIh7eRyEtWUZtxdPzoT+VQzw+19CfRYiv0yk3SegP9iSn8 3zkxuqaFjPoXJey3M3kjd1DYDstt4Z6G3OY3XCRCUUl+hgIJmyy4WA0j8beY1myy12YT T4qLjqNJBZELHCvNb3PqTI1cVwgdwshhQTIpz1H/wfOvONLY1ctSyX4nidkUvRwDX3FV cFUJrg0PPNfsrHHgaDI3LTD7r1Je/dpHYSjHEl/b7ZKmjxeT6w955s4tVvqLpdezqEMY p4Sg== X-Gm-Message-State: AHYfb5jNWNgSvIzagik83esfp74c7NPUNQ4ilc0+D5TWLKIXp4zZ5Vzc viom3f1bMVX2PjtIotPucg== X-Received: by 10.28.188.85 with SMTP id m82mr1814335wmf.3.1503080799495; Fri, 18 Aug 2017 11:26:39 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id q16sm6224997wrg.44.2017.08.18.11.26.38 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 18 Aug 2017 11:26:38 -0700 (PDT) Date: Fri, 18 Aug 2017 19:26:36 +0100 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, lersek@redhat.com, jordan.l.justen@intel.com Message-ID: <20170818182636.a6nj4seicwhkx62d@bivouac.eciton.net> References: <20170818130243.4645-1-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <20170818130243.4645-1-ard.biesheuvel@linaro.org> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH] OvmfPkg/QemuVideoDxe: map framebuffer as write-combining/non-executable X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 18 Aug 2017 18:24:13 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Aug 18, 2017 at 02:02:43PM +0100, Ard Biesheuvel wrote: > When QemuVideoDxe takes control of the framebuffer, it is already > mapped EFI_MEMORY_UC by core code, and QemuVideoDxe simply records > the base and size from the PCI BAR. > > On x86 systems, this is sufficient, but on ARM systems, the semantics > of EFI_MEMORY_UC regions are quite different from EFI_MEMORY_WC regions, > and treating a region like memory (i.e., dereferencing pointers into it > or using ordinary CopyMem()/SetMem() functions on it) requires that it > be mapped with memory semantics, i.e., EFI_MEMORY_WC, EFI_MEMORY_WT or > EFI_MEMORY_WB. > > Since caching is not appropriate for regions where we rely on side > effects, remap the frame buffer EFI_MEMORY_WT. Given that the ARM > architecture requires device mappings to be non-executable (to avoid > inadvertent speculative instruction fetches from device registers), > retain the non-executable nature by adding the EFI_MEMORY_XP attribute > as well. > > Note that the crashes that this patch aims to prevent can currently only > occur under KVM, in which case the VGA driver does not operate correctly > in the first place. However, this is an implementation detail of QEMU > while running under KVM, and given that the ARM architecture simply does > not permit unaligned accesses to device memory, it is best to conform > to this in the code. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel >>From an ARM perspective, this change is clearly correct. If it is acceptable on IA32/X64 also (with the WT->WC fix in message): Reviewed-by: Leif Lindholm If not, we may need some architecture-specicic intermediary. / Leif > --- > OvmfPkg/QemuVideoDxe/Driver.c | 5 +++++ > OvmfPkg/QemuVideoDxe/Gop.c | 18 ++++++++++++++++-- > OvmfPkg/QemuVideoDxe/Qemu.h | 2 ++ > OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 1 + > 4 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c > index 0dce80e59ba8..d81be49d91f3 100644 > --- a/OvmfPkg/QemuVideoDxe/Driver.c > +++ b/OvmfPkg/QemuVideoDxe/Driver.c > @@ -69,6 +69,8 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] = { > } > }; > > +EFI_CPU_ARCH_PROTOCOL *gCpu; > + > static QEMU_VIDEO_CARD* > QemuVideoDetect( > IN UINT16 VendorId, > @@ -1103,5 +1105,8 @@ InitializeQemuVideo ( > ); > ASSERT_EFI_ERROR (Status); > > + Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&gCpu); > + ASSERT_EFI_ERROR(Status); > + > return Status; > } > diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c > index 512fd27acbda..a820524db293 100644 > --- a/OvmfPkg/QemuVideoDxe/Gop.c > +++ b/OvmfPkg/QemuVideoDxe/Gop.c > @@ -53,7 +53,8 @@ QemuVideoCompleteModeData ( > { > EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *Info; > EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *FrameBufDesc; > - QEMU_VIDEO_MODE_DATA *ModeData; > + QEMU_VIDEO_MODE_DATA *ModeData; > + EFI_STATUS Status; > > ModeData = &Private->ModeData[Mode->Mode]; > Info = Mode->Info; > @@ -72,8 +73,21 @@ QemuVideoCompleteModeData ( > DEBUG ((EFI_D_INFO, "FrameBufferBase: 0x%Lx, FrameBufferSize: 0x%Lx\n", > Mode->FrameBufferBase, (UINT64)Mode->FrameBufferSize)); > > + // > + // Remap the framebuffer region as write combining. On x86 systems, this is > + // merely a performance optimization, but on ARM systems, it prevents > + // crashes that may result from unaligned accesses, given that we treat the > + // frame buffer as ordinary memory by using CopyMem()/SetMem() on it. While > + // we're at it, set the non-exec attribute so the framebuffer is not > + // exploitable by malware. > + // > + Status = gCpu->SetMemoryAttributes (gCpu, Mode->FrameBufferBase, > + ALIGN_VALUE (Mode->FrameBufferSize, EFI_PAGE_SIZE), > + EFI_MEMORY_WC | EFI_MEMORY_XP); > + ASSERT_EFI_ERROR (Status); > + > FreePool (FrameBufDesc); > - return EFI_SUCCESS; > + return Status; > } > > STATIC > diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h > index 7fbb25b3efd3..2966c77c78b3 100644 > --- a/OvmfPkg/QemuVideoDxe/Qemu.h > +++ b/OvmfPkg/QemuVideoDxe/Qemu.h > @@ -21,6 +21,7 @@ > > > #include > +#include > #include > #include > #include > @@ -164,6 +165,7 @@ extern EFI_DRIVER_BINDING_PROTOCOL gQemuVideoDriverBinding; > extern EFI_COMPONENT_NAME_PROTOCOL gQemuVideoComponentName; > extern EFI_COMPONENT_NAME2_PROTOCOL gQemuVideoComponentName2; > extern EFI_DRIVER_SUPPORTED_EFI_VERSION_PROTOCOL gQemuVideoDriverSupportedEfiVersion; > +extern EFI_CPU_ARCH_PROTOCOL *gCpu; > > // > // Io Registers defined by VGA > diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > index 346a5aed94fa..bbe11257c002 100644 > --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > @@ -68,6 +68,7 @@ [LibraryClasses] > UefiLib > > [Protocols] > + gEfiCpuArchProtocolGuid # PROTOCOL ALWAYS_CONSUMED > gEfiDriverSupportedEfiVersionProtocolGuid # PROTOCOL ALWAYS_PRODUCED > gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START > gEfiDevicePathProtocolGuid # PROTOCOL BY_START > -- > 2.11.0 >