Re: [PATCH v1] OvmfPkg/X86QemuLoadImageLib: Handle allocation failure for CommandLine

[Martin Radev <martin.b.radev@gmail.com>]

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.radev@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.radev@gmail.com>
> 
> This seems reasonable to me.
> 
> Acked-by: Ard Biesheuvel <ardb@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.