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 6:12 PM
> To: Ni, Ray <ray.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: RE: [PATCH v2 1/2] MdePkg/UefiDevicePathLib: Add sanity check for
> FilePath device path
> 
> Hi Ray,
> 
> > -----Original Message-----
> > From: Ni, Ray
> > Sent: Tuesday, February 19, 2019 5:34 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > Subject: RE: [PATCH v2 1/2] MdePkg/UefiDevicePathLib: Add sanity check
> > for FilePath device path
> >
> >
> >
> > > -----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;
> > }
> >
> 
> I did think about your implementation and I agree it might be the best way from
> compatibility perspective. But I have three considerations:
> 
> 1. It's not a good programming habit to pass unwanted data around and should
> not
>      be encouraged to do so.
> 2. It might leave potential security hole (maybe I'm a little too cautious) 3. The
> UEFI spec has following statement on this type of device path (ch10.3.6.4).
> 
> " A NULL-terminated Path string including directory and file names. The length
> of this string n can be determined by subtracting 4 from the Length entry."
> 

With such statement, I am ok to your changes.

Reviewed-by: Ray Ni <ray.ni@intel.com>

> But I'm still open to your suggestion, if all agree that compatibility is more
> important.
> 
> Regards,
> Jian
> 
> >
> >
> > > +      return FALSE;
> > > +    }
> > >    }
> > >
> > >    //
> > > --
> > > 2.17.1.windows.2