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

[Laszlo Ersek <lersek@redhat.com>]

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