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 <[email protected]>
> Reviewed-by: Jaben Carsey <[email protected]>
> ---
> 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 <[email protected]>
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
