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

["Michael D Kinney" <michael.d.kinney@intel.com>]

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);  ?

Thanks,

Mike

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Thursday, December 8, 2022 9:23 AM
> 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 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]
> > > -=-=-=-=-=-=
> > >
> >
> >
> >
> > 
> >
> >