[edk2,v2,4/6] BaseTools/DevicePath: use MAX_UINT16 as default device path max size

Message ID 20181130224537.18936-5-ard.biesheuvel@linaro.org
State Accepted
Commit c0b7379a31091f9640f7d6592222b286669ef510
Headers show
Series
  • BaseTools: get rid of MAX_UINTN
Related show

Commit Message

Ard Biesheuvel Nov. 30, 2018, 10:45 p.m.
Replace the default size limit of IsDevicePathValid() with a value
that does not depend on the native word size of the build host.

64 KB seems sufficient as the upper bound of a device path handled
by UEFI.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>

---
 BaseTools/Source/C/DevicePath/DevicePathUtilities.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.19.1

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

Comments

Laszlo Ersek Dec. 3, 2018, 1:05 p.m. | #1
On 11/30/18 23:45, Ard Biesheuvel wrote:
> Replace the default size limit of IsDevicePathValid() with a value

> that does not depend on the native word size of the build host.

> 

> 64 KB seems sufficient as the upper bound of a device path handled

> by UEFI.

> 

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>

> ---

>  BaseTools/Source/C/DevicePath/DevicePathUtilities.c | 4 ++--

>  1 file changed, 2 insertions(+), 2 deletions(-)

> 

> diff --git a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c

> index d4ec2742b7c8..ba7f83e53070 100644

> --- a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c

> +++ b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c

> @@ -62,7 +62,7 @@ IsDevicePathValid (

>    ASSERT (DevicePath != NULL);

>  

>    if (MaxSize == 0) {

> -    MaxSize = MAX_UINTN;

> +    MaxSize = MAX_UINT16;

>   }

>  

>    //

> @@ -78,7 +78,7 @@ IsDevicePathValid (

>        return FALSE;

>      }

>  

> -    if (NodeLength > MAX_UINTN - Size) {

> +    if (NodeLength > MAX_UINT16 - Size) {

>        return FALSE;

>      }

>      Size += NodeLength;

> 


I'm somewhat undecided about this patch.

(1) IsDevicePathValid() also exists in:

- MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
- MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLib.c

Both have:

  if (MaxSize == 0) {
    MaxSize = MAX_UINTN;
  }

Relative to those, this change departs quite strongly.


(2) In addition, a single device path node may extend up to 64KB. That
would be pathologic, yes, but the option is there.


... Of course, we are discussing theoretical limits. Still I'd feel more
comfortable with MAX_UINT32. Lifting the limit from 64K to 4G wouldn't
cost us anything (in development effort), it would be a no-op on 32-bit
build hosts, it would be a theoretical-only change on 64-bit build
hosts, and it would leave us with a larger "safety margin".

I won't insist, but I thought I should raise this. (Sorry if this has
been discussed under v1 already.) If you agree, no need to repost (from
my side anyway) just for this.

With or without the update:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>


Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Gao, Liming Dec. 5, 2018, 12:04 a.m. | #2
Laszlo:
  I agree with you. MAX_UINT32 is more comfortable. 

Thanks
Liming
>-----Original Message-----

>From: Laszlo Ersek [mailto:lersek@redhat.com]

>Sent: Monday, December 03, 2018 9:06 PM

>To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org

>Cc: Zhu, Yonghong <yonghong.zhu@intel.com>; Gao, Liming

><liming.gao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Carsey, Jaben

><jaben.carsey@intel.com>

>Subject: Re: [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as

>default device path max size

>

>On 11/30/18 23:45, Ard Biesheuvel wrote:

>> Replace the default size limit of IsDevicePathValid() with a value

>> that does not depend on the native word size of the build host.

>>

>> 64 KB seems sufficient as the upper bound of a device path handled

>> by UEFI.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.1

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>

>> ---

>>  BaseTools/Source/C/DevicePath/DevicePathUtilities.c | 4 ++--

>>  1 file changed, 2 insertions(+), 2 deletions(-)

>>

>> diff --git a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c

>b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c

>> index d4ec2742b7c8..ba7f83e53070 100644

>> --- a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c

>> +++ b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c

>> @@ -62,7 +62,7 @@ IsDevicePathValid (

>>    ASSERT (DevicePath != NULL);

>>

>>    if (MaxSize == 0) {

>> -    MaxSize = MAX_UINTN;

>> +    MaxSize = MAX_UINT16;

>>   }

>>

>>    //

>> @@ -78,7 +78,7 @@ IsDevicePathValid (

>>        return FALSE;

>>      }

>>

>> -    if (NodeLength > MAX_UINTN - Size) {

>> +    if (NodeLength > MAX_UINT16 - Size) {

>>        return FALSE;

>>      }

>>      Size += NodeLength;

>>

>

>I'm somewhat undecided about this patch.

>

>(1) IsDevicePathValid() also exists in:

>

>- MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c

>- MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLib.c

>

>Both have:

>

>  if (MaxSize == 0) {

>    MaxSize = MAX_UINTN;

>  }

>

>Relative to those, this change departs quite strongly.

>

>

>(2) In addition, a single device path node may extend up to 64KB. That

>would be pathologic, yes, but the option is there.

>

>

>... Of course, we are discussing theoretical limits. Still I'd feel more

>comfortable with MAX_UINT32. Lifting the limit from 64K to 4G wouldn't

>cost us anything (in development effort), it would be a no-op on 32-bit

>build hosts, it would be a theoretical-only change on 64-bit build

>hosts, and it would leave us with a larger "safety margin".

>

>I won't insist, but I thought I should raise this. (Sorry if this has

>been discussed under v1 already.) If you agree, no need to repost (from

>my side anyway) just for this.

>

>With or without the update:

>

>Reviewed-by: Laszlo Ersek <lersek@redhat.com>

>

>Thanks

>Laszlo

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Dec. 5, 2018, 7:42 a.m. | #3
On Wed, 5 Dec 2018 at 01:04, Gao, Liming <liming.gao@intel.com> wrote:
>

> Laszlo:

>   I agree with you. MAX_UINT32 is more comfortable.

>


Liming,

No definitions for MAX_UINT32 exist currently in BaseTools, so I will
have to add the following:

diff --git a/BaseTools/Source/C/Common/CommonLib.h
b/BaseTools/Source/C/Common/CommonLib.h
index b1c6c00a3478..1c40180329c4 100644
--- a/BaseTools/Source/C/Common/CommonLib.h
+++ b/BaseTools/Source/C/Common/CommonLib.h
@@ -23,6 +23,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
EITHER EXPRESS OR IMPLIED.
 #define MAX_LONG_FILE_PATH 500

 #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL)
+#define MAX_UINT32 ((UINT32)0xFFFFFFFFULL)
 #define MAX_UINT16  ((UINT16)0xFFFF)
 #define MAX_UINT8   ((UINT8)0xFF)
 #define ARRAY_SIZE(Array) (sizeof (Array) / sizeof ((Array)[0]))

Does your Reviewed-by cover that as well?




> >-----Original Message-----

> >From: Laszlo Ersek [mailto:lersek@redhat.com]

> >Sent: Monday, December 03, 2018 9:06 PM

> >To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org

> >Cc: Zhu, Yonghong <yonghong.zhu@intel.com>; Gao, Liming

> ><liming.gao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Carsey, Jaben

> ><jaben.carsey@intel.com>

> >Subject: Re: [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as

> >default device path max size

> >

> >On 11/30/18 23:45, Ard Biesheuvel wrote:

> >> Replace the default size limit of IsDevicePathValid() with a value

> >> that does not depend on the native word size of the build host.

> >>

> >> 64 KB seems sufficient as the upper bound of a device path handled

> >> by UEFI.

> >>

> >> Contributed-under: TianoCore Contribution Agreement 1.1

> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>

> >> ---

> >>  BaseTools/Source/C/DevicePath/DevicePathUtilities.c | 4 ++--

> >>  1 file changed, 2 insertions(+), 2 deletions(-)

> >>

> >> diff --git a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c

> >b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c

> >> index d4ec2742b7c8..ba7f83e53070 100644

> >> --- a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c

> >> +++ b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c

> >> @@ -62,7 +62,7 @@ IsDevicePathValid (

> >>    ASSERT (DevicePath != NULL);

> >>

> >>    if (MaxSize == 0) {

> >> -    MaxSize = MAX_UINTN;

> >> +    MaxSize = MAX_UINT16;

> >>   }

> >>

> >>    //

> >> @@ -78,7 +78,7 @@ IsDevicePathValid (

> >>        return FALSE;

> >>      }

> >>

> >> -    if (NodeLength > MAX_UINTN - Size) {

> >> +    if (NodeLength > MAX_UINT16 - Size) {

> >>        return FALSE;

> >>      }

> >>      Size += NodeLength;

> >>

> >

> >I'm somewhat undecided about this patch.

> >

> >(1) IsDevicePathValid() also exists in:

> >

> >- MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c

> >- MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLib.c

> >

> >Both have:

> >

> >  if (MaxSize == 0) {

> >    MaxSize = MAX_UINTN;

> >  }

> >

> >Relative to those, this change departs quite strongly.

> >

> >

> >(2) In addition, a single device path node may extend up to 64KB. That

> >would be pathologic, yes, but the option is there.

> >

> >

> >... Of course, we are discussing theoretical limits. Still I'd feel more

> >comfortable with MAX_UINT32. Lifting the limit from 64K to 4G wouldn't

> >cost us anything (in development effort), it would be a no-op on 32-bit

> >build hosts, it would be a theoretical-only change on 64-bit build

> >hosts, and it would leave us with a larger "safety margin".

> >

> >I won't insist, but I thought I should raise this. (Sorry if this has

> >been discussed under v1 already.) If you agree, no need to repost (from

> >my side anyway) just for this.

> >

> >With or without the update:

> >

> >Reviewed-by: Laszlo Ersek <lersek@redhat.com>

> >

> >Thanks

> >Laszlo

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Gao, Liming Dec. 5, 2018, 7:53 a.m. | #4
Yes. The change is necessary. 

> -----Original Message-----

> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> Sent: Wednesday, December 5, 2018 3:42 PM

> To: Gao, Liming <liming.gao@intel.com>

> Cc: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org; Zhu, Yonghong <yonghong.zhu@intel.com>; Feng, Bob C

> <bob.c.feng@intel.com>; Carsey, Jaben <jaben.carsey@intel.com>

> Subject: Re: [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as default device path max size

> 

> On Wed, 5 Dec 2018 at 01:04, Gao, Liming <liming.gao@intel.com> wrote:

> >

> > Laszlo:

> >   I agree with you. MAX_UINT32 is more comfortable.

> >

> 

> Liming,

> 

> No definitions for MAX_UINT32 exist currently in BaseTools, so I will

> have to add the following:

> 

> diff --git a/BaseTools/Source/C/Common/CommonLib.h

> b/BaseTools/Source/C/Common/CommonLib.h

> index b1c6c00a3478..1c40180329c4 100644

> --- a/BaseTools/Source/C/Common/CommonLib.h

> +++ b/BaseTools/Source/C/Common/CommonLib.h

> @@ -23,6 +23,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,

> EITHER EXPRESS OR IMPLIED.

>  #define MAX_LONG_FILE_PATH 500

> 

>  #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL)

> +#define MAX_UINT32 ((UINT32)0xFFFFFFFFULL)

>  #define MAX_UINT16  ((UINT16)0xFFFF)

>  #define MAX_UINT8   ((UINT8)0xFF)

>  #define ARRAY_SIZE(Array) (sizeof (Array) / sizeof ((Array)[0]))

> 

> Does your Reviewed-by cover that as well?

> 

> 

> 

> 

> > >-----Original Message-----

> > >From: Laszlo Ersek [mailto:lersek@redhat.com]

> > >Sent: Monday, December 03, 2018 9:06 PM

> > >To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org

> > >Cc: Zhu, Yonghong <yonghong.zhu@intel.com>; Gao, Liming

> > ><liming.gao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Carsey, Jaben

> > ><jaben.carsey@intel.com>

> > >Subject: Re: [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as

> > >default device path max size

> > >

> > >On 11/30/18 23:45, Ard Biesheuvel wrote:

> > >> Replace the default size limit of IsDevicePathValid() with a value

> > >> that does not depend on the native word size of the build host.

> > >>

> > >> 64 KB seems sufficient as the upper bound of a device path handled

> > >> by UEFI.

> > >>

> > >> Contributed-under: TianoCore Contribution Agreement 1.1

> > >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > >> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>

> > >> ---

> > >>  BaseTools/Source/C/DevicePath/DevicePathUtilities.c | 4 ++--

> > >>  1 file changed, 2 insertions(+), 2 deletions(-)

> > >>

> > >> diff --git a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c

> > >b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c

> > >> index d4ec2742b7c8..ba7f83e53070 100644

> > >> --- a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c

> > >> +++ b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c

> > >> @@ -62,7 +62,7 @@ IsDevicePathValid (

> > >>    ASSERT (DevicePath != NULL);

> > >>

> > >>    if (MaxSize == 0) {

> > >> -    MaxSize = MAX_UINTN;

> > >> +    MaxSize = MAX_UINT16;

> > >>   }

> > >>

> > >>    //

> > >> @@ -78,7 +78,7 @@ IsDevicePathValid (

> > >>        return FALSE;

> > >>      }

> > >>

> > >> -    if (NodeLength > MAX_UINTN - Size) {

> > >> +    if (NodeLength > MAX_UINT16 - Size) {

> > >>        return FALSE;

> > >>      }

> > >>      Size += NodeLength;

> > >>

> > >

> > >I'm somewhat undecided about this patch.

> > >

> > >(1) IsDevicePathValid() also exists in:

> > >

> > >- MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c

> > >- MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLib.c

> > >

> > >Both have:

> > >

> > >  if (MaxSize == 0) {

> > >    MaxSize = MAX_UINTN;

> > >  }

> > >

> > >Relative to those, this change departs quite strongly.

> > >

> > >

> > >(2) In addition, a single device path node may extend up to 64KB. That

> > >would be pathologic, yes, but the option is there.

> > >

> > >

> > >... Of course, we are discussing theoretical limits. Still I'd feel more

> > >comfortable with MAX_UINT32. Lifting the limit from 64K to 4G wouldn't

> > >cost us anything (in development effort), it would be a no-op on 32-bit

> > >build hosts, it would be a theoretical-only change on 64-bit build

> > >hosts, and it would leave us with a larger "safety margin".

> > >

> > >I won't insist, but I thought I should raise this. (Sorry if this has

> > >been discussed under v1 already.) If you agree, no need to repost (from

> > >my side anyway) just for this.

> > >

> > >With or without the update:

> > >

> > >Reviewed-by: Laszlo Ersek <lersek@redhat.com>

> > >

> > >Thanks

> > >Laszlo

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

Patch

diff --git a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
index d4ec2742b7c8..ba7f83e53070 100644
--- a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
+++ b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
@@ -62,7 +62,7 @@  IsDevicePathValid (
   ASSERT (DevicePath != NULL);
 
   if (MaxSize == 0) {
-    MaxSize = MAX_UINTN;
+    MaxSize = MAX_UINT16;
  }
 
   //
@@ -78,7 +78,7 @@  IsDevicePathValid (
       return FALSE;
     }
 
-    if (NodeLength > MAX_UINTN - Size) {
+    if (NodeLength > MAX_UINT16 - Size) {
       return FALSE;
     }
     Size += NodeLength;