Re: [edk2-devel] [PATCH] ShellPkg: Avoid corrupting installed device path protocols

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Ard,

There is a difference between returning a pointer to a device path 
and modifying the device path contents.

If you add CONST to the argument, then an updated pointer to a device
path can not be returned.

The API clear describes returning an updated device path pointer, so
the API is declared correctly without CONST.

The API does not state that the contents of the device path are modified.

An API that uses CONST EFI_DEVICE_PATH* would indicate that the API
should not modify the contents of the device path.  For example:

/**
  Returns the size of a device path in bytes.

  This function returns the size, in bytes, of the device path data structure
  specified by DevicePath including the end of device path node.
  If DevicePath is NULL or invalid, then 0 is returned.

  @param  DevicePath  A pointer to a device path data structure.

  @retval 0           If DevicePath is NULL or invalid.
  @retval Others      The size of a device path in bytes.

**/
UINTN
EFIAPI
GetDevicePathSize (
  IN CONST EFI_DEVICE_PATH_PROTOCOL  *DevicePath
  );

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> Sent: Thursday, December 8, 2022 12:12 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
> Subject: Re: [edk2-devel] [PATCH] ShellPkg: Avoid corrupting installed device path protocols
> 
> On Thu, 8 Dec 2022 at 20:20, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> >
> > Hi Ard,
> >
> > Much of this code has not been updated since initially added in 2010.
> >
> > Looks like a bug to me that has been there the whole time.
> >
> > I agree it is a behavior change in the implementation.  But unless
> > new code use of this API looks at the implementation, they would
> > not know it is destructive and they need to make a copy.  This
> > API is available to external shell apps that use the shell protocol.
> >
> 
> Well, not entirely. The function takes EFI_DEVICE_PATH_PROTOCOL** not
> CONST EFI_DEVICE_PATH_PROTOCOL**, and so one might argue that the
> underlying object is modifiable by the callee. And similarly, that
> shell code should not grab a EFI device path protocol pointer from the
> database and pass it to a function that does not use a CONST qualified
> EFI_DEVICE_PATH_PROTOCOL pointer to accept the argument.
> 
> > We should get the shell owners to evaluate removing the destructive
> > behavior.
> >
> 
> I suppose changing the prototypes is out of the question, as doing so
> would require a new version of the shell protocol?
> 
> > > -----Original Message-----
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > > Sent: Thursday, December 8, 2022 10:45 AM
> > > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > > Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
> > > Subject: Re: [edk2-devel] [PATCH] ShellPkg: Avoid corrupting installed device path protocols
> > >
> > > On Thu, 8 Dec 2022 at 19:28, Kinney, Michael D
> > > <michael.d.kinney@intel.com> wrote:
> > > >
> > > > Hi Ard,
> > > >
> > > > From this description, it does not look like it should be modifying the
> > > > contents of the device path.  Just point to the device path end node that
> > > > follows the match found.
> > > >
> > > > /**
> > > >   Gets the mapping that most closely matches the device path.
> > > >
> > > >   This function gets the mapping which corresponds to the device path *DevicePath. If
> > > >   there is no exact match, then the mapping which most closely matches *DevicePath
> > > >   is returned, and *DevicePath is updated to point to the remaining portion of the
> > > >   device path. If there is an exact match, the mapping is returned and *DevicePath
> > > >   points to the end-of-device-path node.
> > > >
> > > >   @param DevicePath             On entry, points to a device path pointer. On
> > > >                                 exit, updates the pointer to point to the
> > > >                                 portion of the device path after the mapping.
> > > >
> > > >   @retval NULL                  No mapping was found.
> > > >   @return !=NULL                Pointer to NULL-terminated mapping. The buffer
> > > >                                 is callee allocated and should be freed by the caller.
> > > > **/
> > > > CONST CHAR16 *
> > > > EFIAPI
> > > > EfiShellGetMapFromDevicePath (
> > > >   IN OUT EFI_DEVICE_PATH_PROTOCOL  **DevicePath
> > > >   );
> > > >
> > > > I see this API used in many places, and it looks like it would be
> > > > destructive to any multi-instance device path. Multi-instance
> > > > device paths are typically used for consoles, so we may not have
> > > > noticed this destructive behavior with file system mapping paths.
> > > >
> > > > Did you try removing the call to SetDevicePathEndNode (*DevicePath);  ?
> > > >
> > >
> > > No, but that would be a functional change visible to all users of the
> > > current API.
> > >
> > > And note that the calling code already has 'DevicePathCopy' variables,
> > > it just doesn't bother using them, so the intent is clearly to pass a
> > > copy, not the actual device path.
> 
> 
> 
>