From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 1D5AB21E79010 for ; Fri, 18 Aug 2017 14:39:54 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 428314E341; Fri, 18 Aug 2017 21:42:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 428314E341 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-20.phx2.redhat.com [10.3.117.20]) by smtp.corp.redhat.com (Postfix) with ESMTP id B6A4560468; Fri, 18 Aug 2017 21:42:20 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org, leif.lindholm@linaro.org, jordan.l.justen@intel.com References: <20170818130243.4645-1-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: Date: Fri, 18 Aug 2017 23:42:19 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20170818130243.4645-1-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Fri, 18 Aug 2017 21:42:22 +0000 (UTC) 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 21:39:54 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit comments at the bottom On 08/18/17 15:02, 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 > --- > 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 > (1) When we added VirtioGpuDxe to the ArmVirtPkg platforms, the only reason I didn't propose removing QemuVideoDxe from the same platforms was that QemuVideoDxe was usable on QEMU/TCG, and I figured it wouldn't hurt to keep it. Other than that, I see zero point in using this driver on ARM. (And, apparently, it does hurt to keep it.) Can we please consider simply removing this driver from the ArmVirtPkg platforms? (And then some now-conditional compilation could be simplified in the driver too!) (2) If there is an agreement that the suggested changes are required (or "more correct") for x86 as well, then I don't think we should directly access the CPU arch protocol. While it is true that any UEFI driver has a built-in depex on all the *UEFI* architectural protocols, the CPU arch protocol is a PI/DXE arch protocol, not a UEFI one. (2a) Instead, the PciIo protocol has member functions like GetBarAttributes() and SetBarAttributes(). The former can retrieve what attributes a BAR supports -- and this member func is already called just above the addition in QemuVideoCompleteModeData() --, whereas the latter member can apply those attributes. The attributes I have in mind are EFI_PCI_IO_ATTRIBUTE_MEMORY_WRITE_COMBINE and/or EFI_PCI_IO_ATTRIBUTE_MEMORY_CACHED. And, the "executable or not" question should be handled by the PciIo implementation internally (all BARs should be mapped noexec automatically), so we shouldn't have to care about that. ... On the other hand, I see comments in PciIoSetBarAttributes() [MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c] that make me doubt this approach would be viable in practice. I guess it should be tested. (2b) Even if we can't use PciIo for this, I feel that we should still use the highest-level service that we can, and then I would recommend gDS->SetMemorySpaceAttributes(). Note that the CPU arch protocol is introduced like this in the PI spec: Abstracts the processor services that are required to implement some of the DXE services. This protocol must be produced by a boot service or runtime DXE driver and may only be consumed by the DXE Foundation and DXE drivers that produce architectural protocols. IOW, if we decide to explicitly depend on PI/DXE in this UEFI driver, then at least go through the most abstract DXE service that applies. (3) More closely regarding the actual patch: The memory attributes are massaged in QemuVideoCompleteModeData(), but not in QemuVideoVmwareSvgaCompleteModeData(). (Both are called from QemuVideoGraphicsOutputSetMode(), dependent on the QEMU video card model.) I believe the omission is unintended. (If you agree that this is becoming messy and hard to test, especially on aarch64/KVM, then please see my point (1).) (4) I always consider the #inclusion of Protocol and Guid headers necessary for compilation, and the addition of the same to [Protocols] and [Guids] in the INF file necessary for linking. Modifying the INF file might (more recently) pull the necessary GUID declarations and initializer macros into AutoGen.h as well. But, the same definitely doesn't work with libraries (i.e., if you add a lib class to [LibraryClasses], you won't get the lib class header #included automatically!). So, for consistency, I always do both the #include and the INF modification. (5) I don't always insist on very long comment blocks :) (6) Please update your edk2 git signoff template to "TianoCore Contribution Agreement 1.1" (from "1.0"). Thanks, Laszlo