Re: [PATCH] OvmfPkg/QemuVideoDxe: map framebuffer as write-combining/non-executable

[Laszlo Ersek <lersek@redhat.com>]

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 <ard.biesheuvel@linaro.org>
> ---
>  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 <Uefi.h>
> +#include <Protocol/Cpu.h>
>  #include <Protocol/GraphicsOutput.h>
>  #include <Protocol/PciIo.h>
>  #include <Protocol/DriverSupportedEfiVersion.h>
> @@ -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