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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 18 August 2017 at 18:20, Jordan Justen <jordan.l.justen@intel.com> wrote:
> 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?
>

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

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

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