[PATCH v2 0/4] ArmVirtQemu: unmap page #0 to catch NULL pointer dereferences

[PATCH v2 0/4] ArmVirtQemu: unmap page #0 to catch NULL pointer dereferences

[Ard Biesheuvel]

Rationale in patch #4. Patch #3 is a prerequisite patch that ensures
that we no longer need page #0 to be mapped for the NOR flash driver
to be able to expose it as a read/write block device.

Patches #1 and #2 are fixes for the ARM version of the ArmMmuLib driver
for bugs that get triggered by these changes.

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Auger <eric.auger@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Cc: Philippe Mathieu-Daude <philmd@redhat.com>

Ard Biesheuvel (4):
  ArmPkg/ArmMmuLib ARM: handle unmapped section in GetMemoryRegion()
  ArmPkg/ArmMmuLib ARM: handle unmapped sections when updating
    permissions
  ArmVirtPkg/NorFlashQemuLib: disregard our primary FV
  ArmVirtPkg/QemuVirtMemInfoLib: trim the MMIO region mapping

 .../NorFlashQemuLib/NorFlashQemuLib.inf       |  5 ++++
 .../QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf |  4 ++--
 .../QemuVirtMemInfoPeiLib.inf                 |  2 ++
 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c               |  3 +++
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c  |  8 +++++--
 .../Library/NorFlashQemuLib/NorFlashQemuLib.c | 13 +++++++++--
 .../QemuVirtMemInfoLib/QemuVirtMemInfoLib.c   | 23 +++++++++++++------
 7 files changed, 45 insertions(+), 13 deletions(-)

-- 
2.19.1

[PATCH v2 1/4] ArmPkg/ArmMmuLib ARM: handle unmapped section in GetMemoryRegion()

[Ard Biesheuvel]

GetMemoryRegion() is used to obtain the attributes of an existing
mapping, to permit permission attribute changes to be optimized
away if the attributes don't actually change.

The current ARM code assumes that a section mapping or a page mapping
exists for any region passed into GetMemoryRegion(), but the region
may be unmapped entirely, in which case the code will crash. So check
if a section mapping exists before dereferencing it.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 12ca5b26673e..3b29d33d0a9c 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -457,6 +457,9 @@ GetMemoryRegion (
 
   // Get the section at the given index
   SectionDescriptor = FirstLevelTable[TableIndex];
+  if (!SectionDescriptor) {
+    return EFI_NOT_FOUND;
+  }
 
   // If 'BaseAddress' belongs to the section then round it to the section boundary
   if (((SectionDescriptor & TT_DESCRIPTOR_SECTION_TYPE_MASK) == TT_DESCRIPTOR_SECTION_TYPE_SECTION) ||
-- 
2.19.1

Re: [PATCH v2 1/4] ArmPkg/ArmMmuLib ARM: handle unmapped section in GetMemoryRegion()

[Leif Lindholm]

On Fri, Nov 30, 2018 at 12:28:26PM +0100, Ard Biesheuvel wrote:
> GetMemoryRegion() is used to obtain the attributes of an existing
> mapping, to permit permission attribute changes to be optimized
> away if the attributes don't actually change.
> 
> The current ARM code assumes that a section mapping or a page mapping
> exists for any region passed into GetMemoryRegion(), but the region
> may be unmapped entirely, in which case the code will crash. So check
> if a section mapping exists before dereferencing it.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> index 12ca5b26673e..3b29d33d0a9c 100644
> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> @@ -457,6 +457,9 @@ GetMemoryRegion (
>  
>    // Get the section at the given index
>    SectionDescriptor = FirstLevelTable[TableIndex];
> +  if (!SectionDescriptor) {
> +    return EFI_NOT_FOUND;
> +  }
>  
>    // If 'BaseAddress' belongs to the section then round it to the section boundary
>    if (((SectionDescriptor & TT_DESCRIPTOR_SECTION_TYPE_MASK) == TT_DESCRIPTOR_SECTION_TYPE_SECTION) ||
> -- 
> 2.19.1
> 

Re: [PATCH v2 1/4] ArmPkg/ArmMmuLib ARM: handle unmapped section in GetMemoryRegion()

[Philippe Mathieu-Daudé]

On 30/11/18 12:28, Ard Biesheuvel wrote:
> GetMemoryRegion() is used to obtain the attributes of an existing
> mapping, to permit permission attribute changes to be optimized
> away if the attributes don't actually change.
> 
> The current ARM code assumes that a section mapping or a page mapping
> exists for any region passed into GetMemoryRegion(), but the region
> may be unmapped entirely, in which case the code will crash. So check
> if a section mapping exists before dereferencing it.

Good catch!

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

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> index 12ca5b26673e..3b29d33d0a9c 100644
> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> @@ -457,6 +457,9 @@ GetMemoryRegion (
>  
>    // Get the section at the given index
>    SectionDescriptor = FirstLevelTable[TableIndex];
> +  if (!SectionDescriptor) {
> +    return EFI_NOT_FOUND;
> +  }
>  
>    // If 'BaseAddress' belongs to the section then round it to the section boundary
>    if (((SectionDescriptor & TT_DESCRIPTOR_SECTION_TYPE_MASK) == TT_DESCRIPTOR_SECTION_TYPE_SECTION) ||
> 

[PATCH v2 2/4] ArmPkg/ArmMmuLib ARM: handle unmapped sections when updating permissions

[Ard Biesheuvel]

The ARM ArmMmuLib code currently does not take into account that
setting permissions on a region should take into account that a
region may not be mapped yet to begin with.

So when updating a section descriptor whose old value is zero,
pass in the address explicitly.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
index ec51e072ab43..889b22867dc7 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
@@ -695,8 +695,12 @@ UpdateSectionEntries (
     } else {
       // still a section entry
 
-      // mask off appropriate fields
-      Descriptor = CurrentDescriptor & ~EntryMask;
+      if (CurrentDescriptor != 0) {
+        // mask off appropriate fields
+        Descriptor = CurrentDescriptor & ~EntryMask;
+      } else {
+        Descriptor = ((UINTN)FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT;
+      }
 
       // mask in new attributes and/or permissions
       Descriptor |= EntryValue;
-- 
2.19.1

Re: [PATCH v2 2/4] ArmPkg/ArmMmuLib ARM: handle unmapped sections when updating permissions

[Leif Lindholm]

On Fri, Nov 30, 2018 at 12:28:27PM +0100, Ard Biesheuvel wrote:
> The ARM ArmMmuLib code currently does not take into account that
> setting permissions on a region should take into account that a
> region may not be mapped yet to begin with.
> 
> So when updating a section descriptor whose old value is zero,
> pass in the address explicitly.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> index ec51e072ab43..889b22867dc7 100644
> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> @@ -695,8 +695,12 @@ UpdateSectionEntries (
>      } else {
>        // still a section entry
>  
> -      // mask off appropriate fields
> -      Descriptor = CurrentDescriptor & ~EntryMask;
> +      if (CurrentDescriptor != 0) {
> +        // mask off appropriate fields
> +        Descriptor = CurrentDescriptor & ~EntryMask;
> +      } else {
> +        Descriptor = ((UINTN)FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT;
> +      }
>  
>        // mask in new attributes and/or permissions
>        Descriptor |= EntryValue;
> -- 
> 2.19.1
> 

[PATCH v2 3/4] ArmVirtPkg/NorFlashQemuLib: disregard our primary FV

[Ard Biesheuvel]

The primary FV contains the firmware boot image, which is not
runtime updatable in our case. So exposing it to the NOR flash
driver is undesirable, since it may attempt to modify the NOR
flash contents. It is also rather pointless, since we don't
keep anything there that we care to expose. (the SEC and PEI
phase modules are not executable from DXE context, and the
contents of the embedded DXE phase FV are exposed by the DXE
core directly via the FVB2 protocol)

So let's disregard the NOR flash block that covers the primary
FV.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf |  5 +++++
 ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c   | 13 +++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
index d86ff36dbd58..c5752a243e6b 100644
--- a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
+++ b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
@@ -28,6 +28,7 @@ [Sources.common]
 [Packages]
   MdePkg/MdePkg.dec
   ArmPlatformPkg/ArmPlatformPkg.dec
+  ArmPkg/ArmPkg.dec
   ArmVirtPkg/ArmVirtPkg.dec
 
 [LibraryClasses]
@@ -40,3 +41,7 @@ [Protocols]
 
 [Depex]
   gFdtClientProtocolGuid
+
+[Pcd]
+  gArmTokenSpaceGuid.PcdFvBaseAddress
+  gArmTokenSpaceGuid.PcdFvSize
diff --git a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
index 2678f57eaaad..d238e39a59f1 100644
--- a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
+++ b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
@@ -75,13 +75,22 @@ NorFlashPlatformGetDevices (
       Size = SwapBytes64 (ReadUnaligned64 ((VOID *)&Reg[2]));
       Reg += 4;
 
+      PropSize -= 4 * sizeof (UINT32);
+
+      //
+      // Disregard any flash devices that overlap with the primary FV.
+      // The firmware is not updatable from inside the guest anyway.
+      //
+      if ((PcdGet64 (PcdFvBaseAddress) + PcdGet32 (PcdFvSize) > Base) &&
+          (Base + Size) > PcdGet64 (PcdFvBaseAddress)) {
+        continue;
+      }
+
       mNorFlashDevices[Num].DeviceBaseAddress = (UINTN)Base;
       mNorFlashDevices[Num].RegionBaseAddress = (UINTN)Base;
       mNorFlashDevices[Num].Size              = (UINTN)Size;
       mNorFlashDevices[Num].BlockSize         = QEMU_NOR_BLOCK_SIZE;
       Num++;
-
-      PropSize -= 4 * sizeof (UINT32);
     }
   }
 
-- 
2.19.1

Re: [PATCH v2 3/4] ArmVirtPkg/NorFlashQemuLib: disregard our primary FV

[Laszlo Ersek]

On 11/30/18 12:28, Ard Biesheuvel wrote:
> The primary FV contains the firmware boot image, which is not
> runtime updatable in our case. So exposing it to the NOR flash
> driver is undesirable, since it may attempt to modify the NOR
> flash contents. It is also rather pointless, since we don't
> keep anything there that we care to expose. (the SEC and PEI
> phase modules are not executable from DXE context, and the
> contents of the embedded DXE phase FV are exposed by the DXE
> core directly via the FVB2 protocol)
> 
> So let's disregard the NOR flash block that covers the primary
> FV.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf |  5 +++++
>  ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c   | 13 +++++++++++--
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
> index d86ff36dbd58..c5752a243e6b 100644
> --- a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
> +++ b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
> @@ -28,6 +28,7 @@ [Sources.common]
>  [Packages]
>    MdePkg/MdePkg.dec
>    ArmPlatformPkg/ArmPlatformPkg.dec
> +  ArmPkg/ArmPkg.dec
>    ArmVirtPkg/ArmVirtPkg.dec
>  
>  [LibraryClasses]
> @@ -40,3 +41,7 @@ [Protocols]
>  
>  [Depex]
>    gFdtClientProtocolGuid
> +
> +[Pcd]
> +  gArmTokenSpaceGuid.PcdFvBaseAddress
> +  gArmTokenSpaceGuid.PcdFvSize
> diff --git a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
> index 2678f57eaaad..d238e39a59f1 100644
> --- a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
> +++ b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
> @@ -75,13 +75,22 @@ NorFlashPlatformGetDevices (
>        Size = SwapBytes64 (ReadUnaligned64 ((VOID *)&Reg[2]));
>        Reg += 4;
>  
> +      PropSize -= 4 * sizeof (UINT32);
> +
> +      //
> +      // Disregard any flash devices that overlap with the primary FV.
> +      // The firmware is not updatable from inside the guest anyway.
> +      //
> +      if ((PcdGet64 (PcdFvBaseAddress) + PcdGet32 (PcdFvSize) > Base) &&
> +          (Base + Size) > PcdGet64 (PcdFvBaseAddress)) {
> +        continue;
> +      }
> +
>        mNorFlashDevices[Num].DeviceBaseAddress = (UINTN)Base;
>        mNorFlashDevices[Num].RegionBaseAddress = (UINTN)Base;
>        mNorFlashDevices[Num].Size              = (UINTN)Size;
>        mNorFlashDevices[Num].BlockSize         = QEMU_NOR_BLOCK_SIZE;
>        Num++;
> -
> -      PropSize -= 4 * sizeof (UINT32);
>      }
>    }
>  
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[PATCH v2 4/4] ArmVirtPkg/QemuVirtMemInfoLib: trim the MMIO region mapping

[Ard Biesheuvel]

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:

  ArmVirtQemu.fdf:
  > 0x00001000|0x001ff000
  > gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize

  ArmVirtQemuKernel.fdf:
  > 0x00008000|0x001f8000
  > gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize

For ArmVirtQemu, the resulting virtual mapping looks roughly like:
- [0, 4K)       : flash, unmapped
- [4K, 2M)      : flash, mapped as WB+X RAM
- [2M, 64M)     : flash, unmapped
- [64M, 128M)   : varstore flash, will be mapped by the NOR flash driver
- [128M, 256M)  : peripherals, mapped as device
- [256M, 1GB)   : 32-bit MMIO aperture, translated IO aperture, ECAM,
                  will be mapped by the PCI host bridge driver
- [1GB, ...)    : RAM, mapped.

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      | 23 ++++++++++++++------
 3 files changed, 20 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..a26b2fbad9be 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
@@ -21,6 +21,15 @@
 // Number of Virtual Memory Map Descriptors
 #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          5
 
+//
+// mach-virt's core peripherals such as the UART, the GIC and the RTC are
+// all mapped in the 'miscellaneous device I/O' region, which we just map
+// in its entirety rather than device by device. Note that it does not
+// cover any of the NOR flash banks or PCI resource windows.
+//
+#define MACH_VIRT_PERIPH_BASE       0x08000000
+#define MACH_VIRT_PERIPH_SIZE       SIZE_128MB
+
 /**
   Return the Virtual Memory Map of your platform
 
@@ -66,16 +75,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
-- 
2.19.1

Re: [PATCH v2 4/4] ArmVirtPkg/QemuVirtMemInfoLib: trim the MMIO region mapping

[Laszlo Ersek]

On 11/30/18 12:28, 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:
> 
>   ArmVirtQemu.fdf:
>   > 0x00001000|0x001ff000
>   > gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize
> 
>   ArmVirtQemuKernel.fdf:
>   > 0x00008000|0x001f8000
>   > gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize
> 
> For ArmVirtQemu, the resulting virtual mapping looks roughly like:
> - [0, 4K)       : flash, unmapped
> - [4K, 2M)      : flash, mapped as WB+X RAM
> - [2M, 64M)     : flash, unmapped
> - [64M, 128M)   : varstore flash, will be mapped by the NOR flash driver
> - [128M, 256M)  : peripherals, mapped as device
> - [256M, 1GB)   : 32-bit MMIO aperture, translated IO aperture, ECAM,
>                   will be mapped by the PCI host bridge driver
> - [1GB, ...)    : RAM, mapped.
> 
> 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      | 23 ++++++++++++++------
>  3 files changed, 20 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..a26b2fbad9be 100644
> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> @@ -21,6 +21,15 @@
>  // Number of Virtual Memory Map Descriptors
>  #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          5
>  
> +//
> +// mach-virt's core peripherals such as the UART, the GIC and the RTC are
> +// all mapped in the 'miscellaneous device I/O' region, which we just map
> +// in its entirety rather than device by device. Note that it does not
> +// cover any of the NOR flash banks or PCI resource windows.
> +//
> +#define MACH_VIRT_PERIPH_BASE       0x08000000
> +#define MACH_VIRT_PERIPH_SIZE       SIZE_128MB
> +
>  /**
>    Return the Virtual Memory Map of your platform
>  
> @@ -66,16 +75,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
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

Re: [PATCH v2 0/4] ArmVirtQemu: unmap page #0 to catch NULL pointer dereferences

[Ard Biesheuvel]

On Fri, 30 Nov 2018 at 12:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> Rationale in patch #4. Patch #3 is a prerequisite patch that ensures
> that we no longer need page #0 to be mapped for the NOR flash driver
> to be able to expose it as a read/write block device.
>
> Patches #1 and #2 are fixes for the ARM version of the ArmMmuLib driver
> for bugs that get triggered by these changes.
>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Cc: Philippe Mathieu-Daude <philmd@redhat.com>
>
> Ard Biesheuvel (4):
>   ArmPkg/ArmMmuLib ARM: handle unmapped section in GetMemoryRegion()
>   ArmPkg/ArmMmuLib ARM: handle unmapped sections when updating
>     permissions
>   ArmVirtPkg/NorFlashQemuLib: disregard our primary FV
>   ArmVirtPkg/QemuVirtMemInfoLib: trim the MMIO region mapping
>

Pushed as a2df8587bf7a..51bb05c79595

Thanks all