Re: [RFC PATCH 07/11] MdeModulePkg/DxeCore: Execute loaded images in place if possible

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

On Tue, 30 May 2023 at 08:32, Ni, Ray <ray.ni@intel.com> wrote:
>
> I didn't review the existing logic carefully. Several comments:
> 1. Assignments of several fields are skipped when executing in place. Do they matter?
>     a). Image->NumberOfPages
>     b). Image->ImageBasePage
>

These are used when freeing the image again, so NumberOfPages should
remain 0x0 in this case, so that unloading the image does not free any
pages.

> 2. PeCoffLoaderRelocateImage() is called even for XIP case. But I don't think it's expected that relocation really happens. Even if the fixed data is the same as the original data stored in MMIO device, MMIO writing might cause unexpected behavior.
>

PeCoffLoaderRelocateImage() currently calculates the adjustment
offset, and does nothing to the image if it is 0x0.

> 3.  CoreFreePages() is called when image is not loaded successfully. Is it expected for XIP case?
>

Yes, but Image->NumberOfPages will be 0x0.

>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Monday, May 29, 2023 6:17 PM
> > To: devel@edk2.groups.io
> > Cc: Ard Biesheuvel <ardb@kernel.org>; Ni, Ray <ray.ni@intel.com>; Yao, Jiewen
> > <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Taylor Beebe
> > <t@taylorbeebe.com>; Oliver Smith-Denny <osd@smith-denny.com>; Bi, Dandan
> > <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Kinney,
> > Michael D <michael.d.kinney@intel.com>; Leif Lindholm
> > <quic_llindhol@quicinc.com>; Michael Kubacki <mikuback@linux.microsoft.com>
> > Subject: [RFC PATCH 07/11] MdeModulePkg/DxeCore: Execute loaded images in
> > place if possible
> >
> > In the image loader, check whether an image has already been relocated
> > to the address from which it is being loaded. This is not something that
> > can happen by accident, and so we can assume that this means that the
> > image was intended to be executed in place.
> >
> > This removes a redundant copy of the image contents, and also permits
> > the image to be mapped with restricted permissions even before the CPU
> > arch protocol has been dispatched.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  MdeModulePkg/Core/Dxe/Image/Image.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
> > b/MdeModulePkg/Core/Dxe/Image/Image.c
> > index 3dfab4829b3ca17f..621637e869daf62d 100644
> > --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> > +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> > @@ -573,7 +573,7 @@ STATIC
> >  EFI_STATUS
> >
> >  CoreLoadPeImage (
> >
> >    IN BOOLEAN                    BootPolicy,
> >
> > -  IN VOID                       *Pe32Handle,
> >
> > +  IN IMAGE_FILE_HANDLE          *Pe32Handle,
> >
> >    IN LOADED_IMAGE_PRIVATE_DATA  *Image,
> >
> >    IN  UINT32                    Attribute
> >
> >    )
> >
> > @@ -630,10 +630,16 @@ CoreLoadPeImage (
> >        return EFI_UNSUPPORTED;
> >
> >    }
> >
> >
> >
> > +  //
> >
> > +  // Check whether the loaded image can be executed in place
> >
> > +  //
> >
> > +  if (Image->ImageContext.ImageAddress ==
> > (PHYSICAL_ADDRESS)(UINTN)Pe32Handle->Source) {
> >
> > +    goto ExecuteInPlace;
> >
> > +  }
> >
> > +
> >
> >    //
> >
> >    // Allocate Destination Buffer as caller did not pass it in
> >
> >    //
> >
> > -
> >
> >    if (Image->ImageContext.SectionAlignment > EFI_PAGE_SIZE) {
> >
> >      Size = (UINTN)Image->ImageContext.ImageSize + Image-
> > >ImageContext.SectionAlignment;
> >
> >    } else {
> >
> > @@ -704,6 +710,7 @@ CoreLoadPeImage (
> >    //
> >
> >    // Load the image from the file into the allocated memory
> >
> >    //
> >
> > +ExecuteInPlace:
> >
> >    Status = PeCoffLoaderLoadImage (&Image->ImageContext);
> >
> >    if (EFI_ERROR (Status)) {
> >
> >      goto Done;
> >
> > --
> > 2.39.2
>