* [RFC PATCH 0/2] ArmVirtQemu: unmap page #0 to catch NULL pointer dereferences @ 2018-11-28 19:16 Ard Biesheuvel 2018-11-28 19:16 ` [RFC PATCH 1/2] ArmVirtPkg/NorFlashQemuLib: disregard our primary FV Ard Biesheuvel 2018-11-28 19:16 ` [RFC PATCH 2/2] ArmVirtPkg/QemuVirtMemInfoLib: trim the MMIO region mapping Ard Biesheuvel 0 siblings, 2 replies; 7+ messages in thread From: Ard Biesheuvel @ 2018-11-28 19:16 UTC (permalink / raw) To: edk2-devel Cc: Ard Biesheuvel, Laszlo Ersek, Eric Auger, Andrew Jones, Philippe Mathieu-Daude Rationale in patch #2. Patch #1 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. 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 (2): ArmVirtPkg/NorFlashQemuLib: disregard our primary FV ArmVirtPkg/QemuVirtMemInfoLib: trim the MMIO region mapping .../NorFlashQemuLib/NorFlashQemuLib.inf | 5 +++++ .../QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf | 4 ++-- .../QemuVirtMemInfoPeiLib.inf | 2 ++ .../Library/NorFlashQemuLib/NorFlashQemuLib.c | 13 +++++++++++-- .../QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 18 +++++++++++------- 5 files changed, 31 insertions(+), 11 deletions(-) -- 2.19.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [RFC PATCH 1/2] ArmVirtPkg/NorFlashQemuLib: disregard our primary FV 2018-11-28 19:16 [RFC PATCH 0/2] ArmVirtQemu: unmap page #0 to catch NULL pointer dereferences Ard Biesheuvel @ 2018-11-28 19:16 ` Ard Biesheuvel 2018-11-28 22:54 ` Laszlo Ersek 2018-11-28 19:16 ` [RFC PATCH 2/2] ArmVirtPkg/QemuVirtMemInfoLib: trim the MMIO region mapping Ard Biesheuvel 1 sibling, 1 reply; 7+ messages in thread From: Ard Biesheuvel @ 2018-11-28 19:16 UTC (permalink / raw) To: edk2-devel Cc: Ard Biesheuvel, Laszlo Ersek, Eric Auger, Andrew Jones, Philippe Mathieu-Daude 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 don't already expose via the FVB protocol instances that DXE core creates for us based on the FV HOBs (and so there is nothing the partition or file system drivers could potentially attach to via the block I/O and disk I/O protocol instances that the NOR flash driver creates) 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..72b47bdb5a78 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 1/2] ArmVirtPkg/NorFlashQemuLib: disregard our primary FV 2018-11-28 19:16 ` [RFC PATCH 1/2] ArmVirtPkg/NorFlashQemuLib: disregard our primary FV Ard Biesheuvel @ 2018-11-28 22:54 ` Laszlo Ersek 2018-11-28 23:06 ` Ard Biesheuvel 0 siblings, 1 reply; 7+ messages in thread From: Laszlo Ersek @ 2018-11-28 22:54 UTC (permalink / raw) To: Ard Biesheuvel, edk2-devel Cc: Eric Auger, Andrew Jones, Philippe Mathieu-Daude On 11/28/18 20:16, 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. With you so far. > It is also rather pointless, since we don't > keep anything there that we don't already expose via the FVB > protocol instances that DXE core creates for us based on the > FV HOBs I don't follow -- the DXE core does rely on the FV HOBs that we create for it, but I don't remember the DXE core creating FVB protocol instances. An FVB ("firmware volume block") protocol instance is usually created by a flash driver. What am I missing? Do you mean handles with MemoryMapped(...)/FvFile(...) and Fv(...)/FvFile(...) device paths on them? That point into firmware volumes (that have been supposedly decompressed from flash to RAM)? > (and so there is nothing the partition or file system > drivers could potentially attach to via the block I/O and disk > I/O protocol instances that the NOR flash driver creates) Ugh, NorFlashDxe creates BlockIo and DiskIo interfaces itself??? Let's see... /* Although DiskIoDxe will automatically install the DiskIO protocol whenever we install the BlockIO protocol, its implementation is sub-optimal as it reads and writes entire blocks using the BlockIO protocol. In fact we can access NOR flash with a finer granularity than that, so we can improve performance by directly producing the DiskIO protocol. */ Umm... this flash driver does a lot more than I thought it did... or should. :) Anyway I think it should suffice to say in the commit message that we don't want to expose the first flash device as an FVB protocol instance, because (a) it's read-only, and (b) in the DXE phase, we don't use anything from that flash device. It contains: - the reset vector, - the SEC module, - (for ArmVirtQemu) the non-compressed PEI core, and PEIMs, - and a compressed bunch of DXE modules (incl. the DXE core) which are decompressed to RAM anyway. > So let's disregard the NOR flash block that covers the primary > FV. OK. > > 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..72b47bdb5a78 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; > + } > + The overlap condition is expressed correctly, in general, I think; however, both subconditions are off-by-one each. In each, we compare an exclusive limit (one's end) with an inclusive limit (the other's base). And, when exclusive equals inclusive, there is no overlap; they are directly adjacent only. I'd drop the equal signs. > 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); > } > } > > Thanks Laszlo ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 1/2] ArmVirtPkg/NorFlashQemuLib: disregard our primary FV 2018-11-28 22:54 ` Laszlo Ersek @ 2018-11-28 23:06 ` Ard Biesheuvel 2018-11-28 23:37 ` Laszlo Ersek 0 siblings, 1 reply; 7+ messages in thread From: Ard Biesheuvel @ 2018-11-28 23:06 UTC (permalink / raw) To: Laszlo Ersek Cc: edk2-devel@lists.01.org, Auger Eric, Andrew Jones, Philippe Mathieu-Daudé On Wed, 28 Nov 2018 at 23:54, Laszlo Ersek <lersek@redhat.com> wrote: > > On 11/28/18 20:16, 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. > > With you so far. > > > It is also rather pointless, since we don't > > keep anything there that we don't already expose via the FVB > > protocol instances that DXE core creates for us based on the > > FV HOBs > > I don't follow -- the DXE core does rely on the FV HOBs that we create for it, but I don't remember the DXE core creating FVB protocol instances. An FVB ("firmware volume block") protocol instance is usually created by a flash driver. What am I missing? > > Do you mean handles with MemoryMapped(...)/FvFile(...) and Fv(...)/FvFile(...) device paths on them? That point into firmware volumes (that have been supposedly decompressed from flash to RAM)? > The DXE core dispatcher calls CoreProcessFvImageFile() for all FV images it encounters, but looking closer, that only happens for embedded FV images, not the primary one. But that still means the primary FV contains nothing we actually need. > > (and so there is nothing the partition or file system > > drivers could potentially attach to via the block I/O and disk > > I/O protocol instances that the NOR flash driver creates) > > Ugh, NorFlashDxe creates BlockIo and DiskIo interfaces itself??? > > Let's see... > > /* > Although DiskIoDxe will automatically install the DiskIO protocol whenever > we install the BlockIO protocol, its implementation is sub-optimal as it reads > and writes entire blocks using the BlockIO protocol. In fact we can access > NOR flash with a finer granularity than that, so we can improve performance > by directly producing the DiskIO protocol. > */ > > Umm... this flash driver does a lot more than I thought it did... or should. :) > > > Anyway I think it should suffice to say in the commit message that we don't want to expose the first flash device as an FVB protocol instance, because (a) it's read-only, and (b) in the DXE phase, we don't use anything from that flash device. It contains: > - the reset vector, > - the SEC module, > - (for ArmVirtQemu) the non-compressed PEI core, and PEIMs, > - and a compressed bunch of DXE modules (incl. the DXE core) which are decompressed to RAM anyway. > > > So let's disregard the NOR flash block that covers the primary > > FV. > > OK. > > > > > 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..72b47bdb5a78 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; > > + } > > + > > The overlap condition is expressed correctly, in general, I think; however, both subconditions are off-by-one each. In each, we compare an exclusive limit (one's end) with an inclusive limit (the other's base). And, when exclusive equals inclusive, there is no overlap; they are directly adjacent only. I'd drop the equal signs. > > > > 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); > > } > > } > > > > > > Thanks > Laszlo ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 1/2] ArmVirtPkg/NorFlashQemuLib: disregard our primary FV 2018-11-28 23:06 ` Ard Biesheuvel @ 2018-11-28 23:37 ` Laszlo Ersek 0 siblings, 0 replies; 7+ messages in thread From: Laszlo Ersek @ 2018-11-28 23:37 UTC (permalink / raw) To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, Auger Eric, Andrew Jones, Philippe Mathieu-Daudé On 11/29/18 00:06, Ard Biesheuvel wrote: > On Wed, 28 Nov 2018 at 23:54, Laszlo Ersek <lersek@redhat.com> wrote: >> >> On 11/28/18 20:16, 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. >> >> With you so far. >> >>> It is also rather pointless, since we don't >>> keep anything there that we don't already expose via the FVB >>> protocol instances that DXE core creates for us based on the >>> FV HOBs >> >> I don't follow -- the DXE core does rely on the FV HOBs that we create for it, but I don't remember the DXE core creating FVB protocol instances. An FVB ("firmware volume block") protocol instance is usually created by a flash driver. What am I missing? >> >> Do you mean handles with MemoryMapped(...)/FvFile(...) and Fv(...)/FvFile(...) device paths on them? That point into firmware volumes (that have been supposedly decompressed from flash to RAM)? >> > > The DXE core dispatcher calls CoreProcessFvImageFile() for all FV > images it encounters, but looking closer, that only happens for > embedded FV images, not the primary one. > > But that still means the primary FV contains nothing we actually need. OK; I missed CoreProcessFvImageFile(). Even though it's specified in Vol2, "10.4 Firmware Volume Image Files", of the PI-1.6 spec. Thanks Laszlo > >>> (and so there is nothing the partition or file system >>> drivers could potentially attach to via the block I/O and disk >>> I/O protocol instances that the NOR flash driver creates) >> >> Ugh, NorFlashDxe creates BlockIo and DiskIo interfaces itself??? >> >> Let's see... >> >> /* >> Although DiskIoDxe will automatically install the DiskIO protocol whenever >> we install the BlockIO protocol, its implementation is sub-optimal as it reads >> and writes entire blocks using the BlockIO protocol. In fact we can access >> NOR flash with a finer granularity than that, so we can improve performance >> by directly producing the DiskIO protocol. >> */ >> >> Umm... this flash driver does a lot more than I thought it did... or should. :) >> >> >> Anyway I think it should suffice to say in the commit message that we don't want to expose the first flash device as an FVB protocol instance, because (a) it's read-only, and (b) in the DXE phase, we don't use anything from that flash device. It contains: >> - the reset vector, >> - the SEC module, >> - (for ArmVirtQemu) the non-compressed PEI core, and PEIMs, >> - and a compressed bunch of DXE modules (incl. the DXE core) which are decompressed to RAM anyway. >> >>> So let's disregard the NOR flash block that covers the primary >>> FV. >> >> OK. >> >>> >>> 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..72b47bdb5a78 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; >>> + } >>> + >> >> The overlap condition is expressed correctly, in general, I think; however, both subconditions are off-by-one each. In each, we compare an exclusive limit (one's end) with an inclusive limit (the other's base). And, when exclusive equals inclusive, there is no overlap; they are directly adjacent only. I'd drop the equal signs. >> >> >>> 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); >>> } >>> } >>> >>> >> >> Thanks >> Laszlo ^ permalink raw reply [flat|nested] 7+ messages in thread
* [RFC PATCH 2/2] ArmVirtPkg/QemuVirtMemInfoLib: trim the MMIO region mapping 2018-11-28 19:16 [RFC PATCH 0/2] ArmVirtQemu: unmap page #0 to catch NULL pointer dereferences Ard Biesheuvel 2018-11-28 19:16 ` [RFC PATCH 1/2] ArmVirtPkg/NorFlashQemuLib: disregard our primary FV Ard Biesheuvel @ 2018-11-28 19:16 ` Ard Biesheuvel 2018-11-28 23:38 ` Laszlo Ersek 1 sibling, 1 reply; 7+ messages in thread From: Ard Biesheuvel @ 2018-11-28 19:16 UTC (permalink / raw) To: edk2-devel Cc: Ard Biesheuvel, Laszlo Ersek, Eric Auger, Andrew Jones, Philippe Mathieu-Daude 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(-) 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 -- 2.19.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 2/2] ArmVirtPkg/QemuVirtMemInfoLib: trim the MMIO region mapping 2018-11-28 19:16 ` [RFC PATCH 2/2] ArmVirtPkg/QemuVirtMemInfoLib: trim the MMIO region mapping Ard Biesheuvel @ 2018-11-28 23:38 ` Laszlo Ersek 0 siblings, 0 replies; 7+ messages in thread From: Laszlo Ersek @ 2018-11-28 23:38 UTC (permalink / raw) To: Ard Biesheuvel, edk2-devel Cc: Eric Auger, Andrew Jones, Philippe Mathieu-Daude 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-11-28 23:38 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-11-28 19:16 [RFC PATCH 0/2] ArmVirtQemu: unmap page #0 to catch NULL pointer dereferences Ard Biesheuvel 2018-11-28 19:16 ` [RFC PATCH 1/2] ArmVirtPkg/NorFlashQemuLib: disregard our primary FV Ard Biesheuvel 2018-11-28 22:54 ` Laszlo Ersek 2018-11-28 23:06 ` Ard Biesheuvel 2018-11-28 23:37 ` Laszlo Ersek 2018-11-28 19:16 ` [RFC PATCH 2/2] ArmVirtPkg/QemuVirtMemInfoLib: trim the MMIO region mapping Ard Biesheuvel 2018-11-28 23:38 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox