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

[Jordan Justen <jordan.l.justen@intel.com>]

On 2017-08-18 06:04:01, Ard Biesheuvel wrote:
> On 18 August 2017 at 14:02, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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?

Did you get a chance to test x86 systems with the change?

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

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.
//

> > +  //
> > +  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

I don't think a 'Driver Model' driver needs to add arch protocols into
the depex.

-Jordan

> >    gEfiDriverSupportedEfiVersionProtocolGuid     # PROTOCOL ALWAYS_PRODUCED
> >    gEfiGraphicsOutputProtocolGuid                # PROTOCOL BY_START
> >    gEfiDevicePathProtocolGuid                    # PROTOCOL BY_START
> > --
> > 2.11.0
> >