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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#73050): https://edk2.groups.io/g/devel/message/73050 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] -=-=-=-=-=-=-=-=-=-=-=-
