From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web11.20872.1670520179598248754 for ; Thu, 08 Dec 2022 09:23:00 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=PhUvyaW3; spf=pass (domain: kernel.org, ip: 145.40.68.75, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 833D1B825A6 for ; Thu, 8 Dec 2022 17:22:57 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2EC7EC433EF for ; Thu, 8 Dec 2022 17:22:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1670520176; bh=b06NV1JfLJKYae+NtQSUZRV2kAdzKJB3C7XFiLsQVw4=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=PhUvyaW3FFof5ka5I7r75aqX08979k3qZcAyb4U0lpTrYaYXTPQ+GeCCXt7X/2y9l mvUqRt51C4KoguT3Ey+GuRoHVEacxM+1x/X2zks8MNxqKq7wcMMhM/oocAhv0yoUVY qGXvTqI+zKtMlS9uLFMRuUTFC3ZZoR41tXBbHeF7iIil5EeoNjQVLPEKnYNtjl3ffC jg4iN1wjTXtCuXN5tgIrtQiI58YwTKWs1y18nUrtFGNlxZdlwZNcA1wRlQDjKcDe4B qKiRY0zNnaRPK8GERiQGQnO1gXGw9OUN1LuIfdDX2PajKTSdMFjT4IzTPkXMK6bKXD 5HknZtpXSlPTA== Received: by mail-lf1-f50.google.com with SMTP id cf42so3096459lfb.1 for ; Thu, 08 Dec 2022 09:22:56 -0800 (PST) X-Gm-Message-State: ANoB5plDoRmWhG7KCj9XUHFCrHgsiKw4n/ZBxGgDBLsklY0dWMfRgEGo FhduiApIeLlmot4eof98CpUAVF3vkiRKYqCF74s= X-Google-Smtp-Source: AA0mqf6mcF/UdHlGEXB0h86PW8sTlmAWGGyJfBXvJrFdgqZ2b8QiCyVZ3enT/00QJilUXM7qbv2/T19Sp8PiHWxZpx8= X-Received: by 2002:a19:6b19:0:b0:4a2:740b:5b02 with SMTP id d25-20020a196b19000000b004a2740b5b02mr29792318lfa.122.1670520174098; Thu, 08 Dec 2022 09:22:54 -0800 (PST) MIME-Version: 1.0 References: <20221207161245.2554193-1-ardb@kernel.org> In-Reply-To: From: "Ard Biesheuvel" Date: Thu, 8 Dec 2022 18:22:42 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH] ShellPkg: Avoid corrupting installed device path protocols To: devel@edk2.groups.io, michael.d.kinney@intel.com Cc: "Ni, Ray" , "Gao, Zhichao" Content-Type: text/plain; charset="UTF-8" On Thu, 8 Dec 2022 at 17:51, Michael D Kinney 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 On Behalf Of Ard Biesheuvel > > Sent: Wednesday, December 7, 2022 8:13 AM > > To: devel@edk2.groups.io > > Cc: Ni, Ray ; Gao, Zhichao ; Ard Biesheuvel > > 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 > > --- > > 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] > > -=-=-=-=-=-= > > > > > > > >