Re: [PATCH edk2-platforms 4/4] Platform/ARM/BdsLib: maintain alignment for DevicePaths

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

On Thu, 22 Nov 2018 at 19:35, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 11/22/18 18:26, Ard Biesheuvel wrote:
> > DevicePath node types may have any size, and so it is up to the
> > code that manipulates them to ensure that dereferencing them only
> > occurs when the pointer is aligned explicitly.
> >
> > Since BdsConnectAndUpdateDevicePath() has only two callers,
>
> at d9e68a756cfb ("Platform/ARM/SgiPkg: increase max variable size to
> 8KB", 2018-11-20), it seems to have three callers:
>
> - itself
> - BdsConnectDevicePath()
> - BdsLoadImageAndUpdateDevicePath()
>

Indeed. I am updating the second patch to get rid of everything in
BdsLib we are not currently using.

> > one of
> > which itself, we can simply duplicate the device path (similar to
> > how DxeCore's CoreConnectController () does it), and free the pool
> > allocation again on the way out. (Note that the allocation only
> > occurs when the non-recursive path is taken)
>
> I think this rather works around than fixes the problem -- just because
> every remaining device path "slice" is realigned as we advance, it's not
> guaranteed that any and all CHAR16 fields in the now-first node will be
> naturally aligned.
>
> ... However, it certainly applies to FILEPATH_DEVICE_PATH.PathName,
> which is likely the only such field that we care about. :)
>

Looking at 56bed2f41022afcbadecc9f2d537bd31c3d44cbc ("^W never mind ...
the intent appears to be that device path struct members do appear
naturally aligned, even if the size of the data structure is not a
multiple of the max alignment we expect to encounter.

Presumably, this is why CoreConnectController () does the same in this regard.

> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  Platform/ARM/Library/BdsLib/BdsFilePath.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/Platform/ARM/Library/BdsLib/BdsFilePath.c b/Platform/ARM/Library/BdsLib/BdsFilePath.c
> > index 74fdbbee773d..543ac8f83086 100644
> > --- a/Platform/ARM/Library/BdsLib/BdsFilePath.c
> > +++ b/Platform/ARM/Library/BdsLib/BdsFilePath.c
> > @@ -421,7 +421,7 @@ BdsConnectAndUpdateDevicePath (
> >    }
> >
> >    if (RemainingDevicePath) {
> > -    *RemainingDevicePath = Remaining;
> > +    *RemainingDevicePath = DuplicateDevicePath (Remaining);
> >    }
> >
> >    return Status;
>
> OK, so this makes BdsConnectAndUpdateDevicePath()'s RemainingDevicePath
> output param dynamically allocated. And this change works fine with the
> recursive logic too, as you say in the commit message.
>

Yep.

> > @@ -1333,14 +1333,18 @@ BdsLoadImageAndUpdateDevicePath (
> >    }
>
> We already need some error handling here. The control flow in
> BdsConnectAndUpdateDevicePath() boggles my mind a bit, but I think it
> can output a dynamically allocated RemainingDevicePath *and* return an
> error.
>
> Namely, assume that TryRemovableDevice() is reached, and it fails.
>

That doesn't make sense. I'll update that routine to only do the clone
if it returns EFI_SUCCESS.

> So, I think we should add an error handling label
> ("FreeRemainingDevicePath"), and jump to it, from both first "return"
> statements in this function.
>
> Also, we should likely set RemainingDevicePath to NULL at the top of the
> function, and check it at the end, because... ugh...
> BdsConnectAndUpdateDevicePath() might also fail without assigning
> *RemainingDevicePath?
>

The above change should fix that as well afaict.

> >
> >    FileLoader = FileLoaders;
> > +  Status = EFI_UNSUPPORTED;
> >    while (FileLoader->Support != NULL) {
> >      if (FileLoader->Support (*DevicePath, Handle, RemainingDevicePath)) {
> > -      return FileLoader->LoadImage (DevicePath, Handle, RemainingDevicePath, Type, Image, FileSize);
> > +      Status = FileLoader->LoadImage (DevicePath, Handle, RemainingDevicePath,
> > +                             Type, Image, FileSize);
> > +      break;
> >      }
> >      FileLoader++;
> >    }
> >
> > -  return EFI_UNSUPPORTED;
> > +  FreePool (RemainingDevicePath);
> > +  return Status;
> >  }
> >
> >  EFI_STATUS
> >
>
> As I mention near the commit message, BdsConnectDevicePath() is not
> updated. Is that OK? ... Oh wait, BdsConnectDevicePath() is not called
> by anything. Append another patch to drop it, like
> BdsStartEfiApplication()? Then this patch will be fine, assuming you add
> the "goto"s.
>
> Thanks!
> Laszlo