| Message ID | 20181130224537.18936-5-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | c0b7379a31091f9640f7d6592222b286669ef510 |
| Headers | show |
| Series | BaseTools: get rid of MAX_UINTN | expand |
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
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
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
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
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;