On Fri, 19 Mar 2021 at 15:39, Martin Radev <martin.b.ra...@gmail.com> wrote: > > On Fri, Mar 19, 2021 at 03:27:00PM +0100, Ard Biesheuvel wrote: > > On Thu, 18 Mar 2021 at 22:44, Martin Radev <martin.b.ra...@gmail.com> wrote: > > > > > > The CommandLine and InitrdData may be set to NULL if the provided > > > size is too large. Because the zero page is mapped, this would not > > > cause an immediate crash but can lead to memory corruption instead. > > > This patch just adds validation and returns error if either allocation > > > has failed. > > > > > > Ref: > > > https://github.com/martinradev/edk2/commit/6c0ce748b97393240c006e24b73652f30e597a05 > > > > > > Signed-off-by: Martin Radev <martin.b.ra...@gmail.com> > > > > This seems reasonable to me. > > > > Acked-by: Ard Biesheuvel <a...@kernel.org> > > > > > --- > > > OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 11 > > > +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c > > > b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c > > > index 931553c0c1..b983c4d7d0 100644 > > > --- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c > > > +++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c > > > @@ -161,6 +161,12 @@ QemuLoadLegacyImage ( > > > LoadedImage->CommandLine = LoadLinuxAllocateCommandLinePages ( > > > EFI_SIZE_TO_PAGES ( > > > LoadedImage->CommandLineSize)); > > > + > > > + if (LoadedImage->CommandLine == NULL) { > > > + DEBUG ((DEBUG_ERROR, "Unable to allocate memory for kernel command > > > line!\n")); > > > + Status = EFI_OUT_OF_RESOURCES; > > > + goto FreeImage; > > > + } > > > QemuFwCfgSelectItem (QemuFwCfgItemCommandLineData); > > > QemuFwCfgReadBytes (LoadedImage->CommandLineSize, > > > LoadedImage->CommandLine); > > > } > > > @@ -178,6 +184,11 @@ QemuLoadLegacyImage ( > > > LoadedImage->InitrdData = LoadLinuxAllocateInitrdPages ( > > > LoadedImage->SetupBuf, > > > EFI_SIZE_TO_PAGES > > > (LoadedImage->InitrdSize)); > > > + if (LoadedImage->InitrdData == NULL) { > > > + DEBUG ((DEBUG_ERROR, "Unable to allocate memory for initrd!\n")); > > > + Status = EFI_OUT_OF_RESOURCES; > > > + goto FreeImage; > > > + } > > > DEBUG ((DEBUG_INFO, "Initrd size: 0x%x\n", > > > (UINT32)LoadedImage->InitrdSize)); > > > DEBUG ((DEBUG_INFO, "Reading initrd image ...")); > > > -- > > > 2.17.1 > > > > > Thanks. I have a curiousity question: > Is there a good reason the zero page is kept mapped? > IMO, it makes sense to have the first page unmapped to avoid cases when some > piece of code > returns NULL as a failure to an allocation, but then later code uses it by > mistake. > Unmapping it could be a security hardening.
I agree. Amusingly, we had to add a special case to the X86 option rom emulator [0] because many proprietary EFI drivers written for x86 contain NULL pointer dereferences that simply go unnoticed when executed natively, but under emulation on a AArch64 host, they trigger page faults as ARM systems usually don't have anything accessible at physical address 0x0. [0] https://github.com/ardbiesheuvel/X86EmulatorPkg -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#73044): https://edk2.groups.io/g/devel/message/73044 Mute This Topic: https://groups.io/mt/81445621/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
