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

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

On Thu, 8 Dec 2022 at 22:57, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> Ard,
>
> Thank you for the correction.
>
> If we add that CONST, then the ShellPkg build breaks with an error
>
> c:\work\github\tianocore\edk2\ShellPkg\Application\Shell\ShellProtocol.c(297): error C2220: the following warning is treated as an error
> c:\work\github\tianocore\edk2\ShellPkg\Application\Shell\ShellProtocol.c(297): warning C4090: 'function': different 'const' qualifiers
>
> Which is exactly the line we want to remove to prevent the destructive behavior.
>
>     SetDevicePathEndNode (*DevicePath);
>
> If I comment out that line, the ShellPkg build completes with no errors.
>

I'm surprised that this is the only offending line, and I suppose that
is good news.

But as you said, the shell protocol is used much more widely, and
existing callers may rely on the destructive behavior.

At the very least, we should only perform the SetDevicePathEndNode()
call if the node in question is not already an
end-of-entire-devicepath node, as the update is pointless in that
case, but will still trigger a fault if the memory is read-only.

But it really depends on whether any callers might exist that expect a
multi-instance devicepath to be split up.

> I agree that it would be better to update the prototype and get help
> from the compiler to find incorrect implementations.  Even though
> CONST is not in the prototype, from reading the description of the API
> it does not state that the contents are modified, so I think the
> intent was no modifications.
>

Agreed.
> Your suggested change is safe, but it is incomplete because there
> are additional calls through the protocol that are not covered
> by your patch.  We also do not know how many places this API
> is used in downstream projects.  This side effect of a write to
> a read-only page and potential corruption of a multi-instance
> device path looks like a bug to me and we should fix the root
> cause and not fix just some of the callers.
>

OK, so what is the way forward here?


> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> > Sent: Thursday, December 8, 2022 1:40 PM
> > To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: 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 22:15, Michael D Kinney
> > <michael.d.kinney@intel.com> wrote:
> > >
> > > 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.
> > >
> >
> > No, this is incorrect.
> >
> > The function takes a pointer (1) to a pointer(2)  to a device path protocol
> >
> > EFI_DEVICE_PATH_PROTOCOL **
> >
> > So the function can dereference pointer 1 and modify pointer 2
> > *unless* it is marked as CONST, i.e.
> >
> > EFI_DEVICE_PATH_PROTOCOL * CONST *
> >
> > in which case the pointer is not modifiable, but it is permitted to
> > dereference that pointer to modify the underlying object.
> >
> > I am arguing that the prototype should be
> >
> > EFI_DEVICE_PATH_PROTOCOL CONST **
> >
> > (which is the same as putting the CONST at the beginning)
> >
> > where the caller's pointer can be advanced by the callee via the
> > pointer-to-pointer. But that would still not permit the object to be
> > modified.
> >
> > > The API clear describes returning an updated device path pointer, so
> > > the API is declared correctly without CONST.
> > >
> >
> > The pointer may be updated but not the object. It really comes down to
> > the difference between
> >
> > CONST EFI_DEVICE_PATH_PROTOCOL **
> > EFI_DEVICE_PATH_PROTOCOL CONST **
> > EFI_DEVICE_PATH_PROTOCOL * CONST *
> > EFI_DEVICE_PATH_PROTOCOL **CONST
> >
> > (where the first two mean the same thing0
> >
> > > 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
> > >   );
> > >
> >
> > Yes, but this one is a pointer, not a pointer-to-pointer. Big difference.
> >
> >
> > 
> >
>