On Thu, 22 Nov 2018 at 19:35, Laszlo Ersek <[email protected]> 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 <[email protected]>
> > ---
> > 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
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
