diff mbox series

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

Message ID 20181122172645.20819-4-ard.biesheuvel@linaro.org
State New
Headers show
Series Platform/ARM: fix DevicePath mishandling in BdsLib | expand

Commit Message

Ard Biesheuvel Nov. 22, 2018, 5:26 p.m. UTC 
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(-)

-- 
2.17.1

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

Comments

Laszlo Ersek Nov. 22, 2018, 6:09 p.m. UTC | #1 
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;

>  }

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Nov. 22, 2018, 6:14 p.m. UTC | #2 
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Nov. 22, 2018, 6:23 p.m. UTC | #3 
On 11/22/18 19:14, Ard Biesheuvel wrote:
> 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.

> 


Exactly!

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

Patch

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;
+  }
+
+  Status = BdsLoadImageAndUpdateDevicePath (&Path, Type, Image, FileSize);
+  FreePool (Path);
+
+  return Status;
 }