From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22b.google.com (mail-it0-x22b.google.com [IPv6:2607:f8b0:4001:c0b::22b]) (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 424E021E79021 for ; Fri, 18 Aug 2017 06:01:35 -0700 (PDT) Received: by mail-it0-x22b.google.com with SMTP id 77so6682398itj.1 for ; Fri, 18 Aug 2017 06:04:03 -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=NOjxXqVZASx3k/WycZCnLZZvtG4O4LoI++LWG67SHps=; b=OxVVmTCeBtnuV+iyMWxyR9Ql6KZ+AdOWAKUopAAOH8YZ3jVc+WWvrOb+EePZ/zbvix q5FcPMvGIJ5x90fEVr8KxGLpIhUHBPkaBK0eW60SEgFmjRC3Zl+RyHeUbnPTJBMk4ec3 o5IlmRCnT/YqPZ5PsM1KljNeYktdfgd5gYYdk= 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=NOjxXqVZASx3k/WycZCnLZZvtG4O4LoI++LWG67SHps=; b=A1jU8WItkoTHqVtnyiB76+l0VzFeh3tLfw3ZlVImE1AKzUsijxNkALGVtsDVcyHimM lZCrE5pfybO0uV6ThSYWkUVt6f4WKxm/Sm0juCH00oOl5uhkSWu/0dnyJi5BdS4lzfb6 7C7+QyDNnxLOZ2NwvzbEhLQkPzwI5cSeOEU1hcRNqYQLk4umJA6Ky+7c8sXHqDXObrXY R8tlI6AVfRM08kP6+35UbRLGBoyaG2PC4MvCPbv9uxerBuSv4WwD4twyhVMFP5s5sV5y yxJnumvY1juWZ1pw6yTXceNfXNPs5A1Q8zTUYc1tW8aZCHQ2xcKysfbw6wcwvbC5aX+X UfkQ== X-Gm-Message-State: AHYfb5jNCdA0nlrIqDxYFzkdHriJIHiTVtKBkAhI5LxjzZBWVGSMjqyD 3MXjrOzWWPOga0PiEbI5fUEtXvZF844syw8= X-Received: by 10.36.41.143 with SMTP id p137mr1556734itp.98.1503061442351; Fri, 18 Aug 2017 06:04:02 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.162.1 with HTTP; Fri, 18 Aug 2017 06:04:01 -0700 (PDT) In-Reply-To: <20170818130243.4645-1-ard.biesheuvel@linaro.org> References: <20170818130243.4645-1-ard.biesheuvel@linaro.org> From: Ard Biesheuvel Date: Fri, 18 Aug 2017 14:04:01 +0100 Message-ID: To: "edk2-devel@lists.01.org" , Laszlo Ersek , Leif Lindholm , Jordan Justen Cc: Ard Biesheuvel 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 13:01:35 -0000 Content-Type: text/plain; charset="UTF-8" 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 > 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 > -- > 2.11.0 >