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

["Ard Biesheuvel" <ardb@kernel.org>]

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.