From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x232.google.com (mail-it0-x232.google.com [IPv6:2607:f8b0:4001:c0b::232]) (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 30C9421B0E537 for ; Fri, 18 Aug 2017 14:49:23 -0700 (PDT) Received: by mail-it0-x232.google.com with SMTP id 76so12463282ith.0 for ; Fri, 18 Aug 2017 14:51:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=XeIBlE0UEqx+ylyy0yhzuOY8tjaB5dlffF09U8lU7ZQ=; b=ChYt+UwKPgkMnWOqA2rYpNr2hIiqTYrFTi7n+rCpCj6TE38h/axNQHkRQWG6mYfh5m AAMof6Cm5XBMSDV+CDQYly1PBpX+5C9ryOzSvZTCquyIqbRM9r4nh0BUghSZXZ6Cbrvj inyZDP8zJbSR96Moyu7RH+dC372LVezlkb9i0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=XeIBlE0UEqx+ylyy0yhzuOY8tjaB5dlffF09U8lU7ZQ=; b=NqKfvXCDWPgNmNf7l68LpSnRa61d6ben9QC7zJ+iP7AVg2FIVKPRPygEkBZoDpxW0v wmsg+DwEZQ5bCJe/tH97JpoI0aGaCcI0wsji+h6nOnteXzEP7pN4aATbtqCWbk5dM6k7 vf5jDLdSvEWkQcHeQgYtRu7KCCSdZHOibm6fOIWu9vbPZWUyBGcwSHQ6KknsZ6J7LfA4 2n4ScdxUXxWN0E8Vqj8EUFyEViUjOm0T9wPuRQHQFGDShLhg4+HCXF/ExZA7N9XMGxqk 3oaBirF/TmmPtzgj6HgyGjk8Z2nUxWfmoeGTovsMLmSHSU04RqMQxGAiGhvfhBU2Xj9R A9cQ== X-Gm-Message-State: AHYfb5g0koNuLNxwxWKbf+2DeU0bvCkQmYbxhDBzMKSTaX8DDGum21wr 1z4JyGi/Dl//12dqNVxciRH3IwKRsJA3 X-Received: by 10.36.206.71 with SMTP id v68mr517046itg.167.1503093110787; Fri, 18 Aug 2017 14:51:50 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.162.1 with HTTP; Fri, 18 Aug 2017 14:51:50 -0700 (PDT) In-Reply-To: References: <20170818130243.4645-1-ard.biesheuvel@linaro.org> From: Ard Biesheuvel Date: Fri, 18 Aug 2017 22:51:50 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Leif Lindholm , Jordan Justen 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:49:23 -0000 Content-Type: text/plain; charset="UTF-8" On 18 August 2017 at 22:42, Laszlo Ersek wrote: > 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!) > It is actually quite useful in TCG mode, and the fact that QEMU currently allows unaligned accesses to device memory is not something we should be relying upon. > > (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. > Thanks for digging that up. I think we should fix the PCI bus driver if it does not currently support this > (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. > OK > > (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"). > Sure, thanks for reminding me