Re: [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath()

["Yao, Jiewen" <jiewen.yao@intel.com>]

Cool. Thanks!


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, July 19, 2018 6:47 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Dong, Eric <eric.dong@intel.com>; Carsey, Jaben
> <jaben.carsey@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Roman Bacik <roman.bacik@broadcom.com>; Ni, Ruiyu <ruiyu.ni@intel.com>;
> Fu, Siyuan <siyuan.fu@intel.com>; Zeng, Star <star.zeng@intel.com>
> 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 <lersek@redhat.com> 写
> 道：
> >>
> >> 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 <chao.b.zhang@intel.com>
> >> Cc: Eric Dong <eric.dong@intel.com>
> >> Cc: Jaben Carsey <jaben.carsey@intel.com>
> >> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >> Cc: Liming Gao <liming.gao@intel.com>
> >> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >> Cc: Roman Bacik <roman.bacik@broadcom.com>
> >> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >> Cc: Siyuan Fu <siyuan.fu@intel.com>
> >> Cc: Star Zeng <star.zeng@intel.com>
> >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >> ---
> >> 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
> >>
> >>