Hi Ray,
> -----Original Message-----
> From: Ni, Ray
> Sent: Tuesday, February 19, 2019 5:34 PM
> To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming....@intel.com>; Kinney, Michael D
> <michael.d.kin...@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.w...@intel.com>
> > Sent: Tuesday, February 19, 2019 5:22 PM
> > To: edk2-devel@lists.01.org
> > Cc: Gao, Liming <liming....@intel.com>; Ni, Ray <ray...@intel.com>; Kinney,
> > Michael D <michael.d.kin...@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....@intel.com>
> > Cc: Ray Ni <ray...@intel.com>
> > Cc: Michael D Kinney <michael.d.kin...@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.w...@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."
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
