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

[Laszlo Ersek <lersek@redhat.com>]

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?

Thanks,
Laszlo

> +
> +  return Status;
>  }
>