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

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

On Thu, 8 Dec 2022 at 23:35, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> 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?
>

I sent this before I noticed your other reply.

So let's go with your fix to preserve the existing behavior without
triggering the fault.