From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id F39ED21CFA5F7 for ; Fri, 18 Aug 2017 10:18:15 -0700 (PDT) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Aug 2017 10:20:44 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,393,1498546800"; d="scan'208";a="139208250" Received: from pcastin-mobl1.amr.corp.intel.com (HELO localhost) ([10.254.176.77]) by orsmga005.jf.intel.com with ESMTP; 18 Aug 2017 10:20:43 -0700 MIME-Version: 1.0 To: "edk2-devel@lists.01.org" , Ard Biesheuvel , Laszlo Ersek , Leif Lindholm Message-ID: <150307684325.17751.1587324408306927566@jljusten-skl> From: Jordan Justen In-Reply-To: Cc: Ard Biesheuvel References: <20170818130243.4645-1-ard.biesheuvel@linaro.org> User-Agent: alot/0.6.0dev Date: Fri, 18 Aug 2017 10:20:43 -0700 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 17:18:16 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2017-08-18 06:04:01, Ard Biesheuvel wrote: > On 18 August 2017 at 14:02, Ard Biesheuvel wr= ote: > > 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. > = > EFI_MEMORY_WC not WT If a single pixel is written, then WC may not write it through immediately. Would WT be more appropriate? Did you get a chance to test x86 systems with the change? > = > > 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 > > --- > > 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/Drive= r.c > > index 0dce80e59ba8..d81be49d91f3 100644 > > --- a/OvmfPkg/QemuVideoDxe/Driver.c > > +++ b/OvmfPkg/QemuVideoDxe/Driver.c > > @@ -69,6 +69,8 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] =3D { > > } > > }; > > > > +EFI_CPU_ARCH_PROTOCOL *gCpu; > > + > > static QEMU_VIDEO_CARD* > > QemuVideoDetect( > > IN UINT16 VendorId, > > @@ -1103,5 +1105,8 @@ InitializeQemuVideo ( > > ); > > ASSERT_EFI_ERROR (Status); > > > > + Status =3D gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOI= D **)&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 =3D &Private->ModeData[Mode->Mode]; > > Info =3D 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 tr= eat 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. I'm pretty sure Laszlo would disagree, but I think a more terse comment would do fine here. (We always have git blame to get the full story.) // // Set the framebuffer region as write combining (through?) and // non-executable. For ARM the memory range can't be left in the // uncachable state. // > > + // > > + Status =3D 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 g= QemuVideoDriverBinding; > > extern EFI_COMPONENT_NAME_PROTOCOL gQemuVideoComponentN= ame; > > extern EFI_COMPONENT_NAME2_PROTOCOL gQemuVideoComponentN= ame2; > > extern EFI_DRIVER_SUPPORTED_EFI_VERSION_PROTOCOL gQemuVideoDriverSupp= ortedEfiVersion; > > +extern EFI_CPU_ARCH_PROTOCOL *gCpu; > > > > // > > // Io Registers defined by VGA > > diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoD= xe/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_CONS= UMED I don't think a 'Driver Model' driver needs to add arch protocols into the depex. -Jordan > > gEfiDriverSupportedEfiVersionProtocolGuid # PROTOCOL ALWAYS_PROD= UCED > > gEfiGraphicsOutputProtocolGuid # PROTOCOL BY_START > > gEfiDevicePathProtocolGuid # PROTOCOL BY_START > > -- > > 2.11.0 > >