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