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 <[email protected]> --- 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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
