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

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Aug 18, 2017 at 02:02:43PM +0100, 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>

>From an ARM perspective, this change is clearly correct.
If it is acceptable on IA32/X64 also (with the WT->WC fix in message):
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

If not, we may need some architecture-specicic intermediary.

/
    Leif

> ---
>  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
> -- 
> 2.11.0
>