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

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

On 18 August 2017 at 22:42, Laszlo Ersek <lersek@redhat.com> wrote:
> 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!)
>

It is actually quite useful in TCG mode, and the fact that QEMU
currently allows unaligned accesses to device memory is not something
we should be relying upon.

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

Thanks for digging that up. I think we should fix the PCI bus driver
if it does not currently support this

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

OK

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

Sure, thanks for reminding me