diff mbox series

[edk2,RFC,2/2] ArmVirtPkg/QemuVirtMemInfoLib: trim the MMIO region mapping

Message ID 20181128191646.31526-3-ard.biesheuvel@linaro.org
State Superseded
Headers show
Series ArmVirtQemu: unmap page #0 to catch NULL pointer dereferences | expand

Commit Message

Ard Biesheuvel Nov. 28, 2018, 7:16 p.m. UTC 
QEMU/mach-virt is rather unhelpful when it comes to tracking down
NULL pointer dereferences that occur while running in UEFI: since
we have NOR flash mapped at address 0x0, inadvertent reads go
unnoticed, and even most writes are silently dropped, unless you're
unlucky and the instruction in question is one that KVM cannot
emulate, in which case you end up with a QEMU crash like this:

  error: kvm run failed Function not implemented
   PC=000000013f7ff804 X00=000000013f7ab108 X01=0000000000000064
  X02=000000013f801988 X03=00000000800003c4 X04=0000000000000000
  X05=0000000096000044 X06=fffffffffffd8270 X07=000000013f7ab4a0
  X08=0000000000000001 X09=000000013f803b88 X10=000000013f7e88d0
  X11=0000000000000009 X12=000000013f7ab554 X13=0000000000000008
  X14=0000000000000002 X15=0000000000000000 X16=0000000000000000
  X17=0000000000000000 X18=0000000000000000 X19=0000000000000000
  X20=000000013f81c000 X21=000000013f7ab170 X22=000000013f81c000
  X23=0000000009000018 X24=000000013f407020 X25=000000013f81c000
  X26=000000013f803530 X27=000000013f802000 X28=000000013f7ab270
  X29=000000013f7ab0d0 X30=000000013f7fee10  SP=000000013f7a6f30
  PSTATE=800003c5 N--- EL1h

and a warning in the host kernel log that load/store instruction
decoding is not supported by KVM.

Given that the first page of the flash device is not actually
used anyway, let's reduce the mappings of the peripheral space
and the flash device (both of which cover page #0) to only cover
what is actually required.

After this change, any inadvertent read or write from/to the first
physical page will trigger a translation fault inside the guest,
regardless of the nature of the instruction, without crashing QEMU.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf    |  4 ++--
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf |  2 ++
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c      | 18 +++++++++++-------
 3 files changed, 15 insertions(+), 9 deletions(-)

-- 
2.19.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek Nov. 28, 2018, 11:38 p.m. UTC | #1 
On 11/28/18 20:16, Ard Biesheuvel wrote:
> QEMU/mach-virt is rather unhelpful when it comes to tracking down

> NULL pointer dereferences that occur while running in UEFI: since

> we have NOR flash mapped at address 0x0, inadvertent reads go

> unnoticed, and even most writes are silently dropped, unless you're

> unlucky and the instruction in question is one that KVM cannot

> emulate, in which case you end up with a QEMU crash like this:

> 

>   error: kvm run failed Function not implemented

>    PC=000000013f7ff804 X00=000000013f7ab108 X01=0000000000000064

>   X02=000000013f801988 X03=00000000800003c4 X04=0000000000000000

>   X05=0000000096000044 X06=fffffffffffd8270 X07=000000013f7ab4a0

>   X08=0000000000000001 X09=000000013f803b88 X10=000000013f7e88d0

>   X11=0000000000000009 X12=000000013f7ab554 X13=0000000000000008

>   X14=0000000000000002 X15=0000000000000000 X16=0000000000000000

>   X17=0000000000000000 X18=0000000000000000 X19=0000000000000000

>   X20=000000013f81c000 X21=000000013f7ab170 X22=000000013f81c000

>   X23=0000000009000018 X24=000000013f407020 X25=000000013f81c000

>   X26=000000013f803530 X27=000000013f802000 X28=000000013f7ab270

>   X29=000000013f7ab0d0 X30=000000013f7fee10  SP=000000013f7a6f30

>   PSTATE=800003c5 N--- EL1h

> 

> and a warning in the host kernel log that load/store instruction

> decoding is not supported by KVM.

> 

> Given that the first page of the flash device is not actually

> used anyway, let's reduce the mappings of the peripheral space

> and the flash device (both of which cover page #0) to only cover

> what is actually required.


Please insert a pointer here, to the PcdFvBaseAddress / PcdFvSize
settings in the "ArmVirtQemu.fdf" and "ArmVirtQemuKernel.fdf" files. You
could even quote them verbatim (from both FDF files):

ArmVirtQemu.fdf:
> 0x00001000|0x001ff000

> gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize


ArmVirtQemuKernel.fdf:
> 0x00008000|0x001f8000

> gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize


> After this change, any inadvertent read or write from/to the first

> physical page will trigger a translation fault inside the guest,

> regardless of the nature of the instruction, without crashing QEMU.

> 

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf    |  4 ++--

>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf |  2 ++

>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c      | 18 +++++++++++-------

>  3 files changed, 15 insertions(+), 9 deletions(-)

> 

> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf

> index 5c5b841051ad..b6abc52531a8 100644

> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf

> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf

> @@ -39,9 +39,9 @@ [LibraryClasses]

>    PcdLib

>  

>  [Pcd]

> -  gArmTokenSpaceGuid.PcdFdBaseAddress

> +  gArmTokenSpaceGuid.PcdFvBaseAddress

>    gArmTokenSpaceGuid.PcdSystemMemoryBase

>    gArmTokenSpaceGuid.PcdSystemMemorySize

>  

>  [FixedPcd]

> -  gArmTokenSpaceGuid.PcdFdSize

> +  gArmTokenSpaceGuid.PcdFvSize

> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf

> index d12089760b22..16802c5c414b 100644

> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf

> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf

> @@ -43,9 +43,11 @@ [LibraryClasses]

>  

>  [Pcd]

>    gArmTokenSpaceGuid.PcdFdBaseAddress

> +  gArmTokenSpaceGuid.PcdFvBaseAddress

>    gArmTokenSpaceGuid.PcdSystemMemoryBase

>    gArmTokenSpaceGuid.PcdSystemMemorySize

>  

>  [FixedPcd]

>    gArmTokenSpaceGuid.PcdFdSize

> +  gArmTokenSpaceGuid.PcdFvSize

>    gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress

> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> index 0285a11b1d77..0818d0b42d6c 100644

> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> @@ -21,6 +21,10 @@

>  // Number of Virtual Memory Map Descriptors

>  #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          5

>  

> +// mach-virt's 'miscellaneous device I/O' region

> +#define MACH_VIRT_PERIPH_BASE       0x08000000

> +#define MACH_VIRT_PERIPH_SIZE       SIZE_128MB

> +


So this extends from the end of VIRT_FLASH (that is, one past it) to
just before VIRT_PCIE_MMIO. Correct? If so, can you work that into the
comment? (Not necessarily with the QEMU enum constants.)

>  /**

>    Return the Virtual Memory Map of your platform

>  

> @@ -66,16 +70,16 @@ ArmVirtGetMemoryMap (

>        VirtualMemoryTable[0].VirtualBase,

>        VirtualMemoryTable[0].Length));

>  

> -  // Peripheral space before DRAM

> -  VirtualMemoryTable[1].PhysicalBase = 0x0;

> -  VirtualMemoryTable[1].VirtualBase  = 0x0;

> -  VirtualMemoryTable[1].Length       = VirtualMemoryTable[0].PhysicalBase;

> +  // Memory mapped peripherals (UART, RTC, GIC, virtio-mmio, etc)


The comment helps.

> +  VirtualMemoryTable[1].PhysicalBase = MACH_VIRT_PERIPH_BASE;

> +  VirtualMemoryTable[1].VirtualBase  = MACH_VIRT_PERIPH_BASE;

> +  VirtualMemoryTable[1].Length       = MACH_VIRT_PERIPH_SIZE;

>    VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;


So we lose coverage for:

    [VIRT_PCIE_MMIO] =          { 0x10000000, 0x2eff0000 },
    [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
    [VIRT_PCIE_ECAM] =          { 0x3f000000, 0x01000000 },

Among these, VIRT_PCIE_MMIO is handled by our FDT-based PciHostBridgeLib
(just the exposure), and then added to GCD / mapped indirectly by
PciHostBridgeLib.

VIRT_PCIE_PIO is covered by your "[edk2] [PATCH v3 03/16]
ArmVirtPkg/FdtPciHostBridgeLib: map ECAM and I/O spaces in GCD memory map".

Ditto for VIRT_PCIE_ECAM.

Should be safe.

>  

> -  // Remap the FD region as normal executable memory

> -  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFdBaseAddress);

> +  // Map the FV region as normal executable memory

> +  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFvBaseAddress);

>    VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;

> -  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFdSize);

> +  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFvSize);

>    VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;

>  

>    // End of Table

> 


Seems OK. The end remains exactly the same.

Can you add a summary to the commit message, about the mappings we end
up here with:

- [0, 4K): flash, unmapped
- [4K, 128M): flash, mapped as WB+X RAM
- [128M, 256M): periferals, mapped as device
- [256M, 1GB): 32-bit MMIO aperture, translated IO aperture, ECAM --
               none of them mapped here just yet
- [1GB, ...): RAM, mapped.

... So all my remarks are about documentation; please incorporate what
you see fit. :)

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
index 5c5b841051ad..b6abc52531a8 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
@@ -39,9 +39,9 @@  [LibraryClasses]
   PcdLib
 
 [Pcd]
-  gArmTokenSpaceGuid.PcdFdBaseAddress
+  gArmTokenSpaceGuid.PcdFvBaseAddress
   gArmTokenSpaceGuid.PcdSystemMemoryBase
   gArmTokenSpaceGuid.PcdSystemMemorySize
 
 [FixedPcd]
-  gArmTokenSpaceGuid.PcdFdSize
+  gArmTokenSpaceGuid.PcdFvSize
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
index d12089760b22..16802c5c414b 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
@@ -43,9 +43,11 @@  [LibraryClasses]
 
 [Pcd]
   gArmTokenSpaceGuid.PcdFdBaseAddress
+  gArmTokenSpaceGuid.PcdFvBaseAddress
   gArmTokenSpaceGuid.PcdSystemMemoryBase
   gArmTokenSpaceGuid.PcdSystemMemorySize
 
 [FixedPcd]
   gArmTokenSpaceGuid.PcdFdSize
+  gArmTokenSpaceGuid.PcdFvSize
   gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
index 0285a11b1d77..0818d0b42d6c 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
@@ -21,6 +21,10 @@ 
 // Number of Virtual Memory Map Descriptors
 #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          5
 
+// mach-virt's 'miscellaneous device I/O' region
+#define MACH_VIRT_PERIPH_BASE       0x08000000
+#define MACH_VIRT_PERIPH_SIZE       SIZE_128MB
+
 /**
   Return the Virtual Memory Map of your platform
 
@@ -66,16 +70,16 @@  ArmVirtGetMemoryMap (
       VirtualMemoryTable[0].VirtualBase,
       VirtualMemoryTable[0].Length));
 
-  // Peripheral space before DRAM
-  VirtualMemoryTable[1].PhysicalBase = 0x0;
-  VirtualMemoryTable[1].VirtualBase  = 0x0;
-  VirtualMemoryTable[1].Length       = VirtualMemoryTable[0].PhysicalBase;
+  // Memory mapped peripherals (UART, RTC, GIC, virtio-mmio, etc)
+  VirtualMemoryTable[1].PhysicalBase = MACH_VIRT_PERIPH_BASE;
+  VirtualMemoryTable[1].VirtualBase  = MACH_VIRT_PERIPH_BASE;
+  VirtualMemoryTable[1].Length       = MACH_VIRT_PERIPH_SIZE;
   VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
 
-  // Remap the FD region as normal executable memory
-  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFdBaseAddress);
+  // Map the FV region as normal executable memory
+  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFvBaseAddress);
   VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
-  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFdSize);
+  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFvSize);
   VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
 
   // End of Table