BTW...if the only fix we need is to prevent a write to a read-only
page that contains a device path, then there may be a simpler
fix.
Given that the shell mappings are for file systems and not
consoles, we never expect a multi-instance device path.
This means the that the loop that is looking for EndType
will stop at an end entire device path node. The call
to SetDevicePathEndNode() is setting the same end type
that is already there:
if (PathForReturn != NULL) {
while (!IsDevicePathEndType (*DevicePath)) {
*DevicePath = NextDevicePathNode (*DevicePath);
}
SetDevicePathEndNode (*DevicePath);
}
If this is changed to check if the end type is already
an end entire device path and skip the call if it is,
then the write will never occur:
if (PathForReturn != NULL) {
while (!IsDevicePathEndType (*DevicePath)) {
*DevicePath = NextDevicePathNode (*DevicePath);
}
if (!IsDevicePathEnd (*DevicePath)) {
SetDevicePathEndNode (*DevicePath);
}
}
Mike
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kin...@intel.com>
> Sent: Thursday, December 8, 2022 1:58 PM
> To: devel@edk2.groups.io; a...@kernel.org; Kinney, Michael D
> <michael.d.kin...@intel.com>
> Cc: Ni, Ray <ray...@intel.com>; Gao, Zhichao <zhichao....@intel.com>
> Subject: RE: [edk2-devel] [PATCH] ShellPkg: Avoid corrupting installed device
> path protocols
>
> 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 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.
>
> 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.
>
> Mike
>
> > -----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.kin...@intel.com>
> > Cc: Ni, Ray <ray...@intel.com>; Gao, Zhichao <zhichao....@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.kin...@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.
> >
> >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97151): https://edk2.groups.io/g/devel/message/97151
Mute This Topic: https://groups.io/mt/95518373/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
