Re: [edk2-devel] [PATCH v3 5/6] MdeModulePkg/DxeCore: defer PE/COFF emulator registration to StartImage

["Liming Gao" <liming.gao@intel.com>]

Ard:
  I think this change is OK. Ack-by: Liming Gao <liming.gao@intel.com>

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> Sent: Friday, February 28, 2020 1:11 AM
> To: edk2-devel-groups-io <devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao,
> Zhichao <zhichao.gao@intel.com>; Ni, Ray <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] [PATCH v3 5/6] MdeModulePkg/DxeCore: defer PE/COFF emulator registration to StartImage
> 
> On Wed, 26 Feb 2020 at 20:43, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > EDK2's implementation of the LoadImage() boot service permits non-native
> > binaries to be loaded (i.e., X64 images on IA32 firmware), but any
> > attempts to start such an image using StartImage() will return
> > EFI_UNSUPPORTED.
> >
> > The integration of the PE/COFF emulator protocol into the DXE core
> > deviates slightly from this paradigm, given that its IsImageSupported
> > hook as well as its RegisterImage hook are invoked from LoadImage,
> > and by the time StartImage is called, no opportunity is given to the
> > provider of the PE/COFF emulator protocol to prevent an image from
> > being started if it only supports loading it.
> >
> > To address this disparity, let's move the invocation of RegisterImage()
> > to the implementation of the StartImage() boot service, allowing the
> > emulator to permit LoadImage() but reject StartImage() on images that
> > turn out not to meet the requirements of the emulator as it is being
> > started.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> @MdeModulePkg maintainers: do you have any comments on this patch?
> 
> 
> > ---
> >  MdeModulePkg/Core/Dxe/Image/Image.c | 24 +++++++++++---------
> >  1 file changed, 13 insertions(+), 11 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
> > index 22a87ecf6d7c..d86da89ee704 100644
> > --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> > +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> > @@ -756,17 +756,6 @@ CoreLoadPeImage (
> >    // Get the image entry point.
> >    //
> >    Image->EntryPoint   = (EFI_IMAGE_ENTRY_POINT)(UINTN)Image->ImageContext.EntryPoint;
> > -  if (Image->PeCoffEmu != NULL) {
> > -    Status = Image->PeCoffEmu->RegisterImage (Image->PeCoffEmu,
> > -                                 Image->ImageBasePage,
> > -                                 EFI_PAGES_TO_SIZE (Image->NumberOfPages),
> > -                                 &Image->EntryPoint);
> > -    if (EFI_ERROR (Status)) {
> > -      DEBUG ((DEBUG_LOAD | DEBUG_ERROR,
> > -        "CoreLoadPeImage: Failed to register foreign image with emulator.\n"));
> > -      goto Done;
> > -    }
> > -  }
> >
> >    //
> >    // Fill in the image information for the Loaded Image Protocol
> > @@ -1603,6 +1592,19 @@ CoreStartImage (
> >      return EFI_UNSUPPORTED;
> >    }
> >
> > +  if (Image->PeCoffEmu != NULL) {
> > +    Status = Image->PeCoffEmu->RegisterImage (Image->PeCoffEmu,
> > +                                 Image->ImageBasePage,
> > +                                 EFI_PAGES_TO_SIZE (Image->NumberOfPages),
> > +                                 &Image->EntryPoint);
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((DEBUG_LOAD | DEBUG_ERROR,
> > +        "CoreLoadPeImage: Failed to register foreign image with emulator - %r\n",
> > +          Status));
> > +      return Status;
> > +    }
> > +  }
> > +
> >    PERF_START_IMAGE_BEGIN (Handle);
> >
> >
> > --
> > 2.17.1
> >
> 
>