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

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

On Thu, 8 Dec 2022 at 17:51, Michael D Kinney
<michael.d.kinney@intel.com> wrote:
>
> Hi Ard,
>
> Are you saying that ConvertEfiFileProtocolToShellHandle() modifies the device path passed in?
>
> Is that a bug in this API?
>

No, the issue is in EfiShellGetMapFromDevicePath(), which calls
SetDevicePathEndNode() (on line 297) on the provided device path, to
turn an end-of-instance node into a end-of-entire-device-path node. It
even does this if the device path end node ends the entire device path
already, and the resulting store results in a fault if the device path
protocol points to a device path in .rodata.

But the problem is really that this function should not be called with
the original device path pointer from the protocol, which is owned by
the agent that installed it, not some random consumer of the protocol.

>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> > Sent: Wednesday, December 7, 2022 8:13 AM
> > To: devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Ard Biesheuvel <ardb@kernel.org>
> > Subject: [edk2-devel] [PATCH] ShellPkg: Avoid corrupting installed device path protocols
> >
> > The Shell locates device path protocol instances from the database and
> > happily passes them to destructive device path operations, resulting in
> > the original protocol to get corrupted as well. So take a copy instead,
> > and discard it once we no longer need it.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  ShellPkg/Application/Shell/ShellProtocol.c    | 10 +++-
> >  .../Library/UefiShellLevel2CommandsLib/Map.c  | 47 +++++++++++--------
> >  2 files changed, 36 insertions(+), 21 deletions(-)
> >
> > diff --git a/ShellPkg/Application/Shell/ShellProtocol.c b/ShellPkg/Application/Shell/ShellProtocol.c
> > index 509eb60e40f4..6dbf344520d0 100644
> > --- a/ShellPkg/Application/Shell/ShellProtocol.c
> > +++ b/ShellPkg/Application/Shell/ShellProtocol.c
> > @@ -838,7 +838,9 @@ EfiShellOpenRootByHandle (
> >    EFI_STATUS                       Status;
> >
> >    EFI_SIMPLE_FILE_SYSTEM_PROTOCOL  *SimpleFileSystem;
> >
> >    EFI_FILE_PROTOCOL                *RealFileHandle;
> >
> > -  EFI_DEVICE_PATH_PROTOCOL         *DevPath;
> >
> > +  CONST EFI_DEVICE_PATH_PROTOCOL   *DevPath;
> >
> > +  VOID                             *DevPathBuf;
> >
> > +  EFI_DEVICE_PATH_PROTOCOL         *DevPathCopy;
> >
> >
> >
> >    //
> >
> >    // get the simple file system interface
> >
> > @@ -875,7 +877,11 @@ EfiShellOpenRootByHandle (
> >      return Status;
> >
> >    }
> >
> >
> >
> > -  *FileHandle = ConvertEfiFileProtocolToShellHandle (RealFileHandle, EfiShellGetMapFromDevicePath (&DevPath));
> >
> > +  DevPathCopy = DevPathBuf = DuplicateDevicePath (DevPath);
> >
> > +  *FileHandle = ConvertEfiFileProtocolToShellHandle (RealFileHandle,
> >
> > +                                                     EfiShellGetMapFromDevicePath (&DevPathCopy)
> >
> > +                                                     );
> >
> > +  SHELL_FREE_NON_NULL (DevPathBuf);
> >
> >    return (EFI_SUCCESS);
> >
> >  }
> >
> >
> >
> > diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Map.c b/ShellPkg/Library/UefiShellLevel2CommandsLib/Map.c
> > index f3c888edd48c..094e08eab4a5 100644
> > --- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Map.c
> > +++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Map.c
> > @@ -134,7 +134,7 @@ SearchList (
> >  **/
> >
> >  CHAR16 *
> >
> >  GetDeviceMediaType (
> >
> > -  IN  EFI_DEVICE_PATH_PROTOCOL  *DevicePath
> >
> > +  IN  CONST EFI_DEVICE_PATH_PROTOCOL  *DevicePath
> >
> >    )
> >
> >  {
> >
> >    ACPI_HID_DEVICE_PATH  *Acpi;
> >
> > @@ -179,7 +179,7 @@ GetDeviceMediaType (
> >  **/
> >
> >  BOOLEAN
> >
> >  IsRemoveableDevice (
> >
> > -  IN EFI_DEVICE_PATH_PROTOCOL  *DevicePath
> >
> > +  IN CONST EFI_DEVICE_PATH_PROTOCOL  *DevicePath
> >
> >    )
> >
> >  {
> >
> >    if (NULL == DevicePath) {
> >
> > @@ -307,24 +307,29 @@ PerformSingleMappingDisplay (
> >    IN CONST EFI_HANDLE  Handle
> >
> >    )
> >
> >  {
> >
> > -  EFI_DEVICE_PATH_PROTOCOL  *DevPath;
> >
> > -  EFI_DEVICE_PATH_PROTOCOL  *DevPathCopy;
> >
> > -  CONST CHAR16              *MapList;
> >
> > -  CHAR16                    *CurrentName;
> >
> > -  CHAR16                    *MediaType;
> >
> > -  CHAR16                    *DevPathString;
> >
> > -  CHAR16                    *TempSpot;
> >
> > -  CHAR16                    *Alias;
> >
> > -  UINTN                     TempLen;
> >
> > -  BOOLEAN                   Removable;
> >
> > -  CONST CHAR16              *TempSpot2;
> >
> > +  CONST EFI_DEVICE_PATH_PROTOCOL  *DevPath;
> >
> > +  VOID                            *DevPathBuf;
> >
> > +  EFI_DEVICE_PATH_PROTOCOL        *DevPathCopy;
> >
> > +  CONST CHAR16                    *MapList;
> >
> > +  CHAR16                          *CurrentName;
> >
> > +  CHAR16                          *MediaType;
> >
> > +  CHAR16                          *DevPathString;
> >
> > +  CHAR16                          *TempSpot;
> >
> > +  CHAR16                          *Alias;
> >
> > +  UINTN                           TempLen;
> >
> > +  BOOLEAN                         Removable;
> >
> > +  CONST CHAR16                    *TempSpot2;
> >
> >
> >
> >    Alias       = NULL;
> >
> >    TempSpot2   = NULL;
> >
> >    CurrentName = NULL;
> >
> >    DevPath     = DevicePathFromHandle (Handle);
> >
> > -  DevPathCopy = DevPath;
> >
> > +  DevPathBuf  = DuplicateDevicePath (DevPath);
> >
> > +  DevPathCopy = DevPathBuf;
> >
> >    MapList     = gEfiShellProtocol->GetMapFromDevicePath (&DevPathCopy);
> >
> > +
> >
> > +  SHELL_FREE_NON_NULL (DevPathBuf);
> >
> > +
> >
> >    if (MapList == NULL) {
> >
> >      return EFI_NOT_FOUND;
> >
> >    }
> >
> > @@ -485,16 +490,20 @@ PerformSingleMappingDelete (
> >    IN CONST EFI_HANDLE  Handle
> >
> >    )
> >
> >  {
> >
> > -  EFI_DEVICE_PATH_PROTOCOL  *DevPath;
> >
> > -  EFI_DEVICE_PATH_PROTOCOL  *DevPathCopy;
> >
> > -  CONST CHAR16              *MapList;
> >
> > -  CHAR16                    *CurrentName;
> >
> > +  CONST EFI_DEVICE_PATH_PROTOCOL  *DevPath;
> >
> > +  VOID                            *DevPathBuf;
> >
> > +  EFI_DEVICE_PATH_PROTOCOL        *DevPathCopy;
> >
> > +  CONST CHAR16                    *MapList;
> >
> > +  CHAR16                          *CurrentName;
> >
> >
> >
> >    DevPath     = DevicePathFromHandle (Handle);
> >
> > -  DevPathCopy = DevPath;
> >
> > +  DevPathBuf  = DuplicateDevicePath (DevPath);
> >
> > +  DevPathCopy = DevPathBuf;
> >
> >    MapList     = gEfiShellProtocol->GetMapFromDevicePath (&DevPathCopy);
> >
> >    CurrentName = NULL;
> >
> >
> >
> > +  SHELL_FREE_NON_NULL (DevPathBuf);
> >
> > +
> >
> >    if (MapList == NULL) {
> >
> >      return (EFI_NOT_FOUND);
> >
> >    }
> >
> > --
> > 2.35.1
> >
> >
> >
> > -=-=-=-=-=-=
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#97090): https://edk2.groups.io/g/devel/message/97090
> > Mute This Topic: https://groups.io/mt/95518373/1643496
> > Group Owner: devel+owner@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
> > -=-=-=-=-=-=
> >
>
>
>
> 
>
>