Re: [PATCH edk2-platforms 3/4] Platform/ARM/BdsLib: don't clobber BdsLoadImage() DevicePath IN param

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Thu, 22 Nov 2018 at 19:09, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 11/22/18 18:26, Ard Biesheuvel wrote:
> > BdsLoadImage () is part of the BdsLib library API and is not documented
> > as modifying its DevicePath argument, but does so nonetheless. So take
> > a copy instead, and free it after use.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  Platform/ARM/Library/BdsLib/BdsFilePath.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/Platform/ARM/Library/BdsLib/BdsFilePath.c b/Platform/ARM/Library/BdsLib/BdsFilePath.c
> > index 67dafa4f3651..74fdbbee773d 100644
> > --- a/Platform/ARM/Library/BdsLib/BdsFilePath.c
> > +++ b/Platform/ARM/Library/BdsLib/BdsFilePath.c
> > @@ -1351,5 +1351,16 @@ BdsLoadImage (
> >    OUT    UINTN                 *FileSize
> >    )
> >  {
> > -  return BdsLoadImageAndUpdateDevicePath (&DevicePath, Type, Image, FileSize);
> > +  EFI_DEVICE_PATH       *Path;
> > +  EFI_STATUS            Status;
> > +
> > +  Path = DuplicateDevicePath (DevicePath);
> > +  if (Path == NULL) {
> > +    return EFI_OUT_OF_RESOURCES;
> > +  }
>
> This introduces a minor change in behavior.
>
> Previously, if BdsLoadImage() got DevicePath==NULL, then
> BdsLoadImageAndUpdateDevicePath() -> BdsConnectAndUpdateDevicePath()
> would hit (*DevicePath == NULL), and return EFI_INVALID_PARAMETER.
>
> Now, (DevicePath==NULL) causes DuplicateDevicePath() to return NULL, and
> we translate that to EFI_OUT_OF_RESOURCES.
>
> Can you check for (DevicePath==NULL) first, and preserve
> EFI_INVALID_PARAMETER?
>
> > +
> > +  Status = BdsLoadImageAndUpdateDevicePath (&Path, Type, Image, FileSize);
> > +  FreePool (Path);
>
> This is not safe; BdsLoadImageAndUpdateDevicePath() may change Path.
> Namely, in BdsConnectAndUpdateDevicePath(), we have at one location,
>
>       *DevicePath = NewDevicePath;
>
> ... Which, in fact, makes me wonder whether we need this patch at all. I
> believe BdsLoadImageAndUpdateDevicePath() -- and
> BdsConnectAndUpdateDevicePath() -- are supposed to update the caller's
> *pointer* to the device path, and not the pointed-to device path itself.
>
> Do you agree?
>

Indeed.

EFI_STATUS
BdsLoadImage (
  IN     EFI_DEVICE_PATH       *DevicePath,

vs

EFI_STATUS
BdsLoadImageAndUpdateDevicePath (
  IN OUT EFI_DEVICE_PATH       **DevicePath,

and I didn't spot the diference in * vs **

So you are right: BdsConnectAndUpdateDevicePath() assigns to
*DevicePath, which means it updates BdsLoadImage()'s local copy of the
pointer, but not the memory it points to.

The IN/OUT notation makes this a bit ambiguous, though. Having
something like EFI_DEVICE_PATH CONST ** vs EFI_DEVICE_PATH * CONST *
is not necessarily easier to read, but less ambiguous.