Re: [PATCH v2 1/2] MdePkg/UefiDevicePathLib: Add sanity check for FilePath device path

["Ni, Ray" <ray.ni@intel.com>]

> -----Original Message-----
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Tuesday, February 19, 2019 5:22 PM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Subject: [PATCH v2 1/2] MdePkg/UefiDevicePathLib: Add sanity check for
> FilePath device path
> 
> > v2: fix wrong detection of FilePath device path
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1497
> 
> Current implementation of IsDevicePathValid() is not enough for type of
> MEDIA_FILEPATH_DP, which has NULL-terminated string in the device path.
> This patch add a simple NULL character check at Length position.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> b/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> index 5d7635fe3e..dd1bddc1c2 100644
> --- a/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> +++ b/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> @@ -95,6 +95,15 @@ IsDevicePathValid (
>          return FALSE;
>        }
>      }
> +
> +    //
> +    // FilePath must be a NULL-terminated string.
> +    //
> +    if (DevicePathType (DevicePath) == MEDIA_DEVICE_PATH &&
> +        DevicePathSubType (DevicePath) == MEDIA_FILEPATH_DP &&
> +        *(CHAR16 *)((UINT8 *)DevicePath + NodeLength - 2) != 0) {
Can we assume the device path node buffer contains exact the null terminated string?
What if the buffer contains some padding bytes after null terminated string?

To handle this case, can we convert the DevicePath to FILEPATH_DEVICE_PATH and use logic similar as below?

FILEPATH_DEVICE_PATH *FilePath;
FilePath = (FILEPATH_DEVICE_PATH *) DevicePath;
MaxSize = (NodeLength - sizeof (EFI_DEVICE_PATH_PROTOCOL)) / sizeof (CHAR16);
If (StrnLenS (FilePath->PathName, MaxSize) == MaxSize) {
  Return FALSE;
}



> +      return FALSE;
> +    }
>    }
> 
>    //
> --
> 2.17.1.windows.2