Cool. Thanks!
> -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Thursday, July 19, 2018 6:47 PM > To: Yao, Jiewen <[email protected]> > Cc: edk2-devel-01 <[email protected]>; Zhang, Chao B > <[email protected]>; Dong, Eric <[email protected]>; Carsey, Jaben > <[email protected]>; Wu, Jiaxin <[email protected]>; Gao, Liming > <[email protected]>; Kinney, Michael D <[email protected]>; > Roman Bacik <[email protected]>; Ni, Ruiyu <[email protected]>; > Fu, Siyuan <[email protected]>; Zeng, Star <[email protected]> > Subject: Re: [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath() > > On 07/19/18 01:10, Yao, Jiewen wrote: > > Thanks Laszlo. > > > > Would you please add one line comment on the FilePath, to describe if the > FilePath is internal input or external input? As such the API consumer can > know if caller’s responsibility to verify it or callee’s responsibility. > > Good point. The caller is responsible for ensuring the well-formedness > of the device path pointed-to by (*FilePath). The function uses > DevicePathLib APIs such as NextDevicePathNode(), and if e.g. some device > path node headers are invalid (containing bogus lengths, for example), > then the function won't work as expected. > > I'll add such a comment. > > > For example, if the caller gets path from a read write variable, and input > > it > directly, the this API need validate before use. > > If the caller already does the verification, then this API can use it > > directly. > > Right. > > (A separate question would be if edk2 offers facilities for verifying > the well-formedness of any given device path -- it looks like a complex > task, to implement everywhere.) > > In this series I'd just like to extract the duplicated code to a common > location, and fix its bugs. I didn't try to change the interface > contract (just to document it more precisely). If we'd like to "armor" > this function against maliciously formatted device paths, IMO that > should be done separately. > > So I agree that the comment you suggest should be added. > > > Sanity check is just for the format, not the content. > > The question I have is: Where should the sanity check be? > > At the moment: in the caller. > > Thanks! > Laszlo > > > > > thank you! > > Yao, Jiewen > > > > > >> 在 2018年7月19日，上午4:50，Laszlo Ersek <[email protected]> 写 > 道： > >> > >> The EfiOpenFileByDevicePath() function centralizes functionality from > >> > >> - MdeModulePkg/Universal/Disk/RamDiskDxe > >> - NetworkPkg/TlsAuthConfigDxe > >> - SecurityPkg/VariableAuthenticated/SecureBootConfigDxe > >> - ShellPkg/Library/UefiShellLib > >> > >> unifying the implementation and fixing various bugs. > >> > >> Cc: Chao Zhang <[email protected]> > >> Cc: Eric Dong <[email protected]> > >> Cc: Jaben Carsey <[email protected]> > >> Cc: Jiaxin Wu <[email protected]> > >> Cc: Jiewen Yao <[email protected]> > >> Cc: Liming Gao <[email protected]> > >> Cc: Michael D Kinney <[email protected]> > >> Cc: Roman Bacik <[email protected]> > >> Cc: Ruiyu Ni <[email protected]> > >> Cc: Siyuan Fu <[email protected]> > >> Cc: Star Zeng <[email protected]> > >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008 > >> Contributed-under: TianoCore Contribution Agreement 1.1 > >> Signed-off-by: Laszlo Ersek <[email protected]> > >> --- > >> MdePkg/Library/UefiLib/UefiLib.inf | 1 + > >> MdePkg/Include/Library/UefiLib.h | 86 ++++++++ > >> MdePkg/Library/UefiLib/UefiLib.c | 226 ++++++++++++++++++++ > >> 3 files changed, 313 insertions(+) > >> > >> diff --git a/MdePkg/Library/UefiLib/UefiLib.inf > b/MdePkg/Library/UefiLib/UefiLib.inf > >> index f69f0a43b576..a6c739ef3d6d 100644 > >> --- a/MdePkg/Library/UefiLib/UefiLib.inf > >> +++ b/MdePkg/Library/UefiLib/UefiLib.inf > >> @@ -68,6 +68,7 @@ [Protocols] > >> gEfiSimpleTextOutProtocolGuid ## > SOMETIMES_CONSUMES > >> gEfiGraphicsOutputProtocolGuid ## > SOMETIMES_CONSUMES > >> gEfiHiiFontProtocolGuid ## > SOMETIMES_CONSUMES > >> + gEfiSimpleFileSystemProtocolGuid ## > SOMETIMES_CONSUMES > >> gEfiUgaDrawProtocolGuid | > gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport ## > SOMETIMES_CONSUMES # Consumes if gEfiGraphicsOutputProtocolGuid > uninstalled > >> gEfiComponentNameProtocolGuid | NOT > gEfiMdePkgTokenSpaceGuid.PcdComponentNameDisable ## > SOMETIMES_PRODUCES # User chooses to produce it > >> gEfiComponentName2ProtocolGuid | NOT > gEfiMdePkgTokenSpaceGuid.PcdComponentName2Disable ## > SOMETIMES_PRODUCES # User chooses to produce it > >> diff --git a/MdePkg/Include/Library/UefiLib.h > b/MdePkg/Include/Library/UefiLib.h > >> index 7c6fde620c74..2468bf2aee80 100644 > >> --- a/MdePkg/Include/Library/UefiLib.h > >> +++ b/MdePkg/Include/Library/UefiLib.h > >> @@ -33,6 +33,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF > ANY KIND, EITHER EXPRESS OR IMPLIED. > >> #include <Protocol/DriverDiagnostics.h> > >> #include <Protocol/DriverDiagnostics2.h> > >> #include <Protocol/GraphicsOutput.h> > >> +#include <Protocol/DevicePath.h> > >> +#include <Protocol/SimpleFileSystem.h> > >> > >> #include <Library/BaseLib.h> > >> > >> @@ -1520,4 +1522,88 @@ EfiLocateProtocolBuffer ( > >> OUT UINTN *NoProtocols, > >> OUT VOID ***Buffer > >> ); > >> + > >> +/** > >> + Open or create a file or directory, possibly creating the chain of > >> + directories leading up to the directory. > >> + > >> + EfiOpenFileByDevicePath() first locates > EFI_SIMPLE_FILE_SYSTEM_PROTOCOL on > >> + FilePath, and opens the root directory of that filesystem with > >> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume(). > >> + > >> + On the remaining device path, the longest initial sequence of > >> + FILEPATH_DEVICE_PATH nodes is node-wise traversed with > >> + EFI_FILE_PROTOCOL.Open(). For the pathname fragment specified by > each > >> + traversed FILEPATH_DEVICE_PATH node, EfiOpenFileByDevicePath() first > masks > >> + EFI_FILE_MODE_CREATE out of OpenMode, and passes 0 for Attributes. > If > >> + EFI_FILE_PROTOCOL.Open() fails, and OpenMode includes > EFI_FILE_MODE_CREATE, > >> + then the operation is retried with the caller's OpenMode and Attributes > >> + unmodified. > >> + > >> + (As a consequence, if OpenMode includes EFI_FILE_MODE_CREATE, and > Attributes > >> + includes EFI_FILE_DIRECTORY, and each FILEPATH_DEVICE_PATH specifies > a single > >> + pathname component, then EfiOpenFileByDevicePath() ensures that the > specified > >> + series of subdirectories exist on return.) > >> + > >> + The EFI_FILE_PROTOCOL identified by the last FILEPATH_DEVICE_PATH > node is > >> + output to the caller; intermediate EFI_FILE_PROTOCOL instances are > closed. If > >> + there are no FILEPATH_DEVICE_PATH nodes past the node that identifies > the > >> + filesystem, then the EFI_FILE_PROTOCOL of the root directory of the > >> + filesystem is output to the caller. If a device path node that is > >> different > >> + from FILEPATH_DEVICE_PATH is encountered relative to the filesystem, > the > >> + traversal is stopped with an error, and a NULL EFI_FILE_PROTOCOL is > output. > >> + > >> + @param[in,out] FilePath On input, the device path to the file or > directory > >> + to open or create. On output, FilePath > points one > >> + past the last node in the original device > path that > >> + has been successfully processed. FilePath is > set on > >> + output even if EfiOpenFileByDevicePath() > returns an > >> + error. > >> + > >> + @param[out] File On error, File is set to NULL. On success, File > is > >> + set to the EFI_FILE_PROTOCOL of the root > directory > >> + of the filesystem, if there are no > >> + FILEPATH_DEVICE_PATH nodes in FilePath; > otherwise, > >> + File is set to the EFI_FILE_PROTOCOL > identified by > >> + the last node in FilePath. > >> + > >> + @param[in] OpenMode The OpenMode parameter to pass to > >> + EFI_FILE_PROTOCOL.Open(). For each > >> + FILEPATH_DEVICE_PATH node in FilePath, > >> + EfiOpenFileByDevicePath() first opens the > specified > >> + pathname fragment with > EFI_FILE_MODE_CREATE masked > >> + out of OpenMode and with Attributes set > to 0, and > >> + only retries the operation with > EFI_FILE_MODE_CREATE > >> + unmasked and Attributes propagated if the > first open > >> + attempt fails. > >> + > >> + @param[in] Attributes The Attributes parameter to pass to > >> + EFI_FILE_PROTOCOL.Open(), when > EFI_FILE_MODE_CREATE > >> + is propagated unmasked in OpenMode. > >> + > >> + @retval EFI_SUCCESS The file or directory has been > opened or > >> + created. > >> + > >> + @retval EFI_INVALID_PARAMETER FilePath is NULL; or File is NULL; or > FilePath > >> + contains a device path node, past > the node > >> + that identifies > >> + > EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, that is not a > >> + FILEPATH_DEVICE_PATH node. > >> + > >> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed. > >> + > >> + @return Error codes propagated from the > >> + LocateDevicePath() and > OpenProtocol() boot > >> + services, and from the > >> + > EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume() > >> + and EFI_FILE_PROTOCOL.Open() > member functions. > >> +**/ > >> +EFI_STATUS > >> +EFIAPI > >> +EfiOpenFileByDevicePath ( > >> + IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath, > >> + OUT EFI_FILE_PROTOCOL **File, > >> + IN UINT64 OpenMode, > >> + IN UINT64 Attributes > >> + ); > >> #endif > >> diff --git a/MdePkg/Library/UefiLib/UefiLib.c > b/MdePkg/Library/UefiLib/UefiLib.c > >> index 828a54ce7a97..d3e290178cd9 100644 > >> --- a/MdePkg/Library/UefiLib/UefiLib.c > >> +++ b/MdePkg/Library/UefiLib/UefiLib.c > >> @@ -1719,3 +1719,229 @@ EfiLocateProtocolBuffer ( > >> > >> return EFI_SUCCESS; > >> } > >> + > >> +/** > >> + Open or create a file or directory, possibly creating the chain of > >> + directories leading up to the directory. > >> + > >> + EfiOpenFileByDevicePath() first locates > EFI_SIMPLE_FILE_SYSTEM_PROTOCOL on > >> + FilePath, and opens the root directory of that filesystem with > >> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume(). > >> + > >> + On the remaining device path, the longest initial sequence of > >> + FILEPATH_DEVICE_PATH nodes is node-wise traversed with > >> + EFI_FILE_PROTOCOL.Open(). For the pathname fragment specified by > each > >> + traversed FILEPATH_DEVICE_PATH node, EfiOpenFileByDevicePath() first > masks > >> + EFI_FILE_MODE_CREATE out of OpenMode, and passes 0 for Attributes. > If > >> + EFI_FILE_PROTOCOL.Open() fails, and OpenMode includes > EFI_FILE_MODE_CREATE, > >> + then the operation is retried with the caller's OpenMode and Attributes > >> + unmodified. > >> + > >> + (As a consequence, if OpenMode includes EFI_FILE_MODE_CREATE, and > Attributes > >> + includes EFI_FILE_DIRECTORY, and each FILEPATH_DEVICE_PATH specifies > a single > >> + pathname component, then EfiOpenFileByDevicePath() ensures that the > specified > >> + series of subdirectories exist on return.) > >> + > >> + The EFI_FILE_PROTOCOL identified by the last FILEPATH_DEVICE_PATH > node is > >> + output to the caller; intermediate EFI_FILE_PROTOCOL instances are > closed. If > >> + there are no FILEPATH_DEVICE_PATH nodes past the node that identifies > the > >> + filesystem, then the EFI_FILE_PROTOCOL of the root directory of the > >> + filesystem is output to the caller. If a device path node that is > >> different > >> + from FILEPATH_DEVICE_PATH is encountered relative to the filesystem, > the > >> + traversal is stopped with an error, and a NULL EFI_FILE_PROTOCOL is > output. > >> + > >> + @param[in,out] FilePath On input, the device path to the file or > directory > >> + to open or create. On output, FilePath > points one > >> + past the last node in the original device > path that > >> + has been successfully processed. FilePath is > set on > >> + output even if EfiOpenFileByDevicePath() > returns an > >> + error. > >> + > >> + @param[out] File On error, File is set to NULL. On success, File > is > >> + set to the EFI_FILE_PROTOCOL of the root > directory > >> + of the filesystem, if there are no > >> + FILEPATH_DEVICE_PATH nodes in FilePath; > otherwise, > >> + File is set to the EFI_FILE_PROTOCOL > identified by > >> + the last node in FilePath. > >> + > >> + @param[in] OpenMode The OpenMode parameter to pass to > >> + EFI_FILE_PROTOCOL.Open(). For each > >> + FILEPATH_DEVICE_PATH node in FilePath, > >> + EfiOpenFileByDevicePath() first opens the > specified > >> + pathname fragment with > EFI_FILE_MODE_CREATE masked > >> + out of OpenMode and with Attributes set > to 0, and > >> + only retries the operation with > EFI_FILE_MODE_CREATE > >> + unmasked and Attributes propagated if the > first open > >> + attempt fails. > >> + > >> + @param[in] Attributes The Attributes parameter to pass to > >> + EFI_FILE_PROTOCOL.Open(), when > EFI_FILE_MODE_CREATE > >> + is propagated unmasked in OpenMode. > >> + > >> + @retval EFI_SUCCESS The file or directory has been > opened or > >> + created. > >> + > >> + @retval EFI_INVALID_PARAMETER FilePath is NULL; or File is NULL; or > FilePath > >> + contains a device path node, past > the node > >> + that identifies > >> + > EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, that is not a > >> + FILEPATH_DEVICE_PATH node. > >> + > >> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed. > >> + > >> + @return Error codes propagated from the > >> + LocateDevicePath() and > OpenProtocol() boot > >> + services, and from the > >> + > EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume() > >> + and EFI_FILE_PROTOCOL.Open() > member functions. > >> +**/ > >> +EFI_STATUS > >> +EFIAPI > >> +EfiOpenFileByDevicePath ( > >> + IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath, > >> + OUT EFI_FILE_PROTOCOL **File, > >> + IN UINT64 OpenMode, > >> + IN UINT64 Attributes > >> + ) > >> +{ > >> + EFI_STATUS Status; > >> + EFI_HANDLE FileSystemHandle; > >> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *FileSystem; > >> + EFI_FILE_PROTOCOL *LastFile; > >> + > >> + if (File == NULL) { > >> + return EFI_INVALID_PARAMETER; > >> + } > >> + *File = NULL; > >> + > >> + if (FilePath == NULL) { > >> + return EFI_INVALID_PARAMETER; > >> + } > >> + > >> + // > >> + // Look up the filesystem. > >> + // > >> + Status = gBS->LocateDevicePath ( > >> + &gEfiSimpleFileSystemProtocolGuid, > >> + FilePath, > >> + &FileSystemHandle > >> + ); > >> + if (EFI_ERROR (Status)) { > >> + return Status; > >> + } > >> + Status = gBS->OpenProtocol ( > >> + FileSystemHandle, > >> + &gEfiSimpleFileSystemProtocolGuid, > >> + (VOID **)&FileSystem, > >> + gImageHandle, > >> + NULL, > >> + EFI_OPEN_PROTOCOL_GET_PROTOCOL > >> + ); > >> + if (EFI_ERROR (Status)) { > >> + return Status; > >> + } > >> + > >> + // > >> + // Open the root directory of the filesystem. After this operation > succeeds, > >> + // we have to release LastFile on error. > >> + // > >> + Status = FileSystem->OpenVolume (FileSystem, &LastFile); > >> + if (EFI_ERROR (Status)) { > >> + return Status; > >> + } > >> + > >> + // > >> + // Traverse the device path nodes relative to the filesystem. > >> + // > >> + while (!IsDevicePathEnd (*FilePath)) { > >> + // > >> + // Keep local variables that relate to the current device path node > tightly > >> + // scoped. > >> + // > >> + FILEPATH_DEVICE_PATH *FilePathNode; > >> + CHAR16 *AlignedPathName; > >> + CHAR16 *PathName; > >> + EFI_FILE_PROTOCOL *NextFile; > >> + > >> + if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH || > >> + DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP) { > >> + Status = EFI_INVALID_PARAMETER; > >> + goto CloseLastFile; > >> + } > >> + FilePathNode = (FILEPATH_DEVICE_PATH *)*FilePath; > >> + > >> + // > >> + // FilePathNode->PathName may be unaligned, and the UEFI > specification > >> + // requires pointers that are passed to protocol member functions to > be > >> + // aligned. Create an aligned copy of the pathname if necessary. > >> + // > >> + if ((UINTN)FilePathNode->PathName % sizeof > *FilePathNode->PathName == 0) { > >> + AlignedPathName = NULL; > >> + PathName = FilePathNode->PathName; > >> + } else { > >> + AlignedPathName = AllocateCopyPool ( > >> + (DevicePathNodeLength (FilePathNode) - > >> + SIZE_OF_FILEPATH_DEVICE_PATH), > >> + FilePathNode->PathName > >> + ); > >> + if (AlignedPathName == NULL) { > >> + Status = EFI_OUT_OF_RESOURCES; > >> + goto CloseLastFile; > >> + } > >> + PathName = AlignedPathName; > >> + } > >> + > >> + // > >> + // Open the next pathname fragment with EFI_FILE_MODE_CREATE > masked out and > >> + // with Attributes set to 0. > >> + // > >> + Status = LastFile->Open ( > >> + LastFile, > >> + &NextFile, > >> + PathName, > >> + OpenMode & > ~(UINT64)EFI_FILE_MODE_CREATE, > >> + 0 > >> + ); > >> + > >> + // > >> + // Retry with EFI_FILE_MODE_CREATE and the original Attributes if the > first > >> + // attempt failed, and the caller specified EFI_FILE_MODE_CREATE. > >> + // > >> + if (EFI_ERROR (Status) && (OpenMode & EFI_FILE_MODE_CREATE) != 0) > { > >> + Status = LastFile->Open ( > >> + LastFile, > >> + &NextFile, > >> + PathName, > >> + OpenMode, > >> + Attributes > >> + ); > >> + } > >> + > >> + // > >> + // Release any AlignedPathName on both error and success paths; > PathName is > >> + // no longer needed. > >> + // > >> + if (AlignedPathName != NULL) { > >> + FreePool (AlignedPathName); > >> + } > >> + if (EFI_ERROR (Status)) { > >> + goto CloseLastFile; > >> + } > >> + > >> + // > >> + // Advance to the next device path node. > >> + // > >> + LastFile->Close (LastFile); > >> + LastFile = NextFile; > >> + *FilePath = NextDevicePathNode (FilePathNode); > >> + } > >> + > >> + *File = LastFile; > >> + return EFI_SUCCESS; > >> + > >> +CloseLastFile: > >> + LastFile->Close (LastFile); > >> + > >> + ASSERT (EFI_ERROR (Status)); > >> + return Status; > >> +} > >> -- > >> 2.14.1.3.gb7cf6e02401b > >> > >> _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
