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

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, 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)

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(-)

-- 
2.17.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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()

> 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. :)

> 

> 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.

> @@ -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.

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?

>  

>    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
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 11/23/18 09:35, Ard Biesheuvel wrote:
> On Thu, 22 Nov 2018 at 19:35, Laszlo Ersek <lersek@redhat.com> wrote:


>> [...]


> Indeed. I am updating the second patch to get rid of everything in

> BdsLib we are not currently using.


Thanks!

> Looking at 56bed2f41022afcbadecc9f2d537bd31c3d44cbc ("^W never mind

> ...


heh :)

> 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.


Yeah, that's certainly for working around bugs elsewhere in the
firmware. The UEFI spec 2.7 says in "10.3.1 Generic Device Path
Structures":

    A Device Path is a series of generic Device Path nodes. The first
    Device Path node starts at byte offset zero of the Device Path. The
    next Device Path node starts at the end of the previous Device Path
    node. Therefore all nodes are byte-packed data structures that may
    appear on any byte boundary. *All code references to device path
    notes must assume all fields are unaligned.*

(Emphasis mine. And yes, the exact sentence that I'm quoting this
section for contains a typo, s/notes/nodes/.)

[...]

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel