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

["Ard Biesheuvel" <ardb@kernel.org>]

On Fri, 19 Mar 2021 at 15:39, Martin Radev <martin.b.radev@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.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.

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