ShellPkg/Application/Shell/ShellProtocol.c | 10 +++- .../Library/UefiShellLevel2CommandsLib/Map.c | 47 +++++++++++-------- 2 files changed, 36 insertions(+), 21 deletions(-)
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-Hi Ard,
Are you saying that ConvertEfiFileProtocolToShellHandle() modifies the device path passed in?
Is that a bug in this API?
Mike
> -----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]
> -=-=-=-=-=-=
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97141): https://edk2.groups.io/g/devel/message/97141
Mute This Topic: https://groups.io/mt/95518373/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.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]
> > -=-=-=-=-=-=
> >
>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97142): https://edk2.groups.io/g/devel/message/97142
Mute This Topic: https://groups.io/mt/95518373/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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]
> > > -=-=-=-=-=-=
> > >
> >
> >
> >
> >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97144): https://edk2.groups.io/g/devel/message/97144
Mute This Topic: https://groups.io/mt/95518373/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Thu, 8 Dec 2022 at 19:28, Kinney, Michael D <michael.d.kinney@intel.com> wrote: > > 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); ? > No, but that would be a functional change visible to all users of the current API. And note that the calling code already has 'DevicePathCopy' variables, it just doesn't bother using them, so the intent is clearly to pass a copy, not the actual device path. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97145): https://edk2.groups.io/g/devel/message/97145 Mute This Topic: https://groups.io/mt/95518373/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Ard, Much of this code has not been updated since initially added in 2010. Looks like a bug to me that has been there the whole time. I agree it is a behavior change in the implementation. But unless new code use of this API looks at the implementation, they would not know it is destructive and they need to make a copy. This API is available to external shell apps that use the shell protocol. We should get the shell owners to evaluate removing the destructive behavior. Mike > -----Original Message----- > From: Ard Biesheuvel <ardb@kernel.org> > Sent: Thursday, December 8, 2022 10:45 AM > To: Kinney, Michael D <michael.d.kinney@intel.com> > Cc: devel@edk2.groups.io; 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 19:28, Kinney, Michael D > <michael.d.kinney@intel.com> wrote: > > > > 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); ? > > > > No, but that would be a functional change visible to all users of the > current API. > > And note that the calling code already has 'DevicePathCopy' variables, > it just doesn't bother using them, so the intent is clearly to pass a > copy, not the actual device path. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97146): https://edk2.groups.io/g/devel/message/97146 Mute This Topic: https://groups.io/mt/95518373/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Thu, 8 Dec 2022 at 20:20, Kinney, Michael D <michael.d.kinney@intel.com> wrote: > > Hi Ard, > > Much of this code has not been updated since initially added in 2010. > > Looks like a bug to me that has been there the whole time. > > I agree it is a behavior change in the implementation. But unless > new code use of this API looks at the implementation, they would > not know it is destructive and they need to make a copy. This > API is available to external shell apps that use the shell protocol. > Well, not entirely. The function takes EFI_DEVICE_PATH_PROTOCOL** not CONST EFI_DEVICE_PATH_PROTOCOL**, and so one might argue that the underlying object is modifiable by the callee. And similarly, that shell code should not grab a EFI device path protocol pointer from the database and pass it to a function that does not use a CONST qualified EFI_DEVICE_PATH_PROTOCOL pointer to accept the argument. > We should get the shell owners to evaluate removing the destructive > behavior. > I suppose changing the prototypes is out of the question, as doing so would require a new version of the shell protocol? > > -----Original Message----- > > From: Ard Biesheuvel <ardb@kernel.org> > > Sent: Thursday, December 8, 2022 10:45 AM > > To: Kinney, Michael D <michael.d.kinney@intel.com> > > Cc: devel@edk2.groups.io; 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 19:28, Kinney, Michael D > > <michael.d.kinney@intel.com> wrote: > > > > > > 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); ? > > > > > > > No, but that would be a functional change visible to all users of the > > current API. > > > > And note that the calling code already has 'DevicePathCopy' variables, > > it just doesn't bother using them, so the intent is clearly to pass a > > copy, not the actual device path. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97147): https://edk2.groups.io/g/devel/message/97147 Mute This Topic: https://groups.io/mt/95518373/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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. The API clear describes returning an updated device path pointer, so the API is declared correctly without CONST. 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 ); Mike > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel > Sent: Thursday, December 8, 2022 12:12 PM > To: Kinney, Michael D <michael.d.kinney@intel.com> > Cc: devel@edk2.groups.io; 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 20:20, Kinney, Michael D > <michael.d.kinney@intel.com> wrote: > > > > Hi Ard, > > > > Much of this code has not been updated since initially added in 2010. > > > > Looks like a bug to me that has been there the whole time. > > > > I agree it is a behavior change in the implementation. But unless > > new code use of this API looks at the implementation, they would > > not know it is destructive and they need to make a copy. This > > API is available to external shell apps that use the shell protocol. > > > > Well, not entirely. The function takes EFI_DEVICE_PATH_PROTOCOL** not > CONST EFI_DEVICE_PATH_PROTOCOL**, and so one might argue that the > underlying object is modifiable by the callee. And similarly, that > shell code should not grab a EFI device path protocol pointer from the > database and pass it to a function that does not use a CONST qualified > EFI_DEVICE_PATH_PROTOCOL pointer to accept the argument. > > > We should get the shell owners to evaluate removing the destructive > > behavior. > > > > I suppose changing the prototypes is out of the question, as doing so > would require a new version of the shell protocol? > > > > -----Original Message----- > > > From: Ard Biesheuvel <ardb@kernel.org> > > > Sent: Thursday, December 8, 2022 10:45 AM > > > To: Kinney, Michael D <michael.d.kinney@intel.com> > > > Cc: devel@edk2.groups.io; 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 19:28, Kinney, Michael D > > > <michael.d.kinney@intel.com> wrote: > > > > > > > > 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); ? > > > > > > > > > > No, but that would be a functional change visible to all users of the > > > current API. > > > > > > And note that the calling code already has 'DevicePathCopy' variables, > > > it just doesn't bother using them, so the intent is clearly to pass a > > > copy, not the actual device path. > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97148): https://edk2.groups.io/g/devel/message/97148 Mute This Topic: https://groups.io/mt/95518373/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97149): https://edk2.groups.io/g/devel/message/97149 Mute This Topic: https://groups.io/mt/95518373/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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.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.
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97150): https://edk2.groups.io/g/devel/message/97150
Mute This Topic: https://groups.io/mt/95518373/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.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. > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97152): https://edk2.groups.io/g/devel/message/97152 Mute This Topic: https://groups.io/mt/95518373/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97153): https://edk2.groups.io/g/devel/message/97153 Mute This Topic: https://groups.io/mt/95518373/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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.kinney@intel.com>
> Sent: Thursday, December 8, 2022 1:58 PM
> To: devel@edk2.groups.io; ardb@kernel.org; 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
>
> 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.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.
> >
> >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.