From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x22c.google.com (mail-io0-x22c.google.com [IPv6:2607:f8b0:4001:c06::22c]) (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 6E02821E7903A for ; Fri, 18 Aug 2017 10:22:47 -0700 (PDT) Received: by mail-io0-x22c.google.com with SMTP id o9so35373826iod.1 for ; Fri, 18 Aug 2017 10:25:15 -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=5Woa7SziuRi1GkVdZQlmxIBYsd2LnBW+whn69v4tf/s=; b=QgjrjR5DVUYoIeub9TnbCdkfgS2VBfpHUm60HtbIFYdKIohXo+i4mVrdgo86c0CNWn fXtLLwpDXsrcQwB6qMOYrgPEx2EZkKeszPIIj74ULry9GQLKaRNTRMZFIeAIajU2YsH3 lRzfs6wnl5/fBrW+L1li9fwaCwc6dr3cfsDvA= 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=5Woa7SziuRi1GkVdZQlmxIBYsd2LnBW+whn69v4tf/s=; b=HOMRapBw92QOlngrgSwLbsC16gl7FyMBJy0vCZJNndBOIrVzKaVqQHW5djT0i8byCd +GeaE/te8YJfyhAY3SwXo9nYbUwyBwtka3DbHs/az5py1+TxWpLb0rzEDYEkozMugJA6 70uxoIRhk4A3VWlMOEM60dgWqprQ2bf1+wMgdDF9AT+hgkZMTu3sM3lyhKCgUIgV/QYq Emns7s1JxlDnME3tvCj+E/RO6t9cQJiGY3NCu7tk+bu5L+qJupAE7nJFXxH6TNoJBXmQ E+p5geOtbhBvF8f7ojJ2eC1aEmmZuVZ8AFFiXimPbHMMMCFN1yJfUW5XWCXYwVs0zE2I wHVg== X-Gm-Message-State: AHYfb5h1fUPRXZOUyK5bOzOEd5x1fg8a0OablBQErPR+5zPnzTV/ngcz XpyzNiJ1VdOkY+Olxp8UQk54SL6RW/AB X-Received: by 10.107.135.226 with SMTP id r95mr9951721ioi.3.1503077115093; Fri, 18 Aug 2017 10:25:15 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.162.1 with HTTP; Fri, 18 Aug 2017 10:25:14 -0700 (PDT) In-Reply-To: <150307684325.17751.1587324408306927566@jljusten-skl> References: <20170818130243.4645-1-ard.biesheuvel@linaro.org> <150307684325.17751.1587324408306927566@jljusten-skl> From: Ard Biesheuvel Date: Fri, 18 Aug 2017 18:25:14 +0100 Message-ID: To: Jordan Justen Cc: "edk2-devel@lists.01.org" , Laszlo Ersek , Leif Lindholm 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:22:47 -0000 Content-Type: text/plain; charset="UTF-8" On 18 August 2017 at 18:20, Jordan Justen wrote: > On 2017-08-18 06:04:01, Ard Biesheuvel wrote: >> On 18 August 2017 at 14: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. >> >> EFI_MEMORY_WC not WT > > If a single pixel is written, then WC may not write it through > immediately. Would WT be more appropriate? > For ARM, that applies equally to WT AFAIK. > Did you get a chance to test x86 systems with the change? > No, I haven't yet. >> >> > 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. > > 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. > // > Fine by me. >> > + // >> > + 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 > > I don't think a 'Driver Model' driver needs to add arch protocols into > the depex. > To be pedantic: this is not the depex. You can't rely on the protocol header to declare gEfiCpuArchProtocolGuid, and declaring it here makes the build tools add its declaration to AutoGen.h (I think this has to do with the exact .dsc version. Perhaps Laszlo has a better recollection of the details.)