[PATCH v2] MdePkg/UefiFileHandleLib: Fix potential NULL dereference.

[PATCH v2] MdePkg/UefiFileHandleLib: Fix potential NULL dereference.

[Marvin Häuser]

Move the NULL-check in FileHandleGetInfo() to directly after the
allocation to prevent potential NULL dereferences.

V2:
  - Do not change the copyright date as requested.
  - Added R-bs from V1 as no functional changes have been made.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
---
 MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 28 +++++++++++---------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
index 57aad77bc135..5b3d39ef6103 100644
--- a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
+++ b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
@@ -74,19 +74,21 @@ FileHandleGetInfo (
     // error is expected.  getting size to allocate
     //
     FileInfo = AllocateZeroPool(FileInfoSize);
-    //
-    // now get the information
-    //
-    Status = FileHandle->GetInfo(FileHandle,
-                                 &gEfiFileInfoGuid,
-                                 &FileInfoSize,
-                                 FileInfo);
-    //
-    // if we got an error free the memory and return NULL
-    //
-    if (EFI_ERROR(Status) && (FileInfo != NULL)) {
-      FreePool(FileInfo);
-      FileInfo = NULL;
+    if (FileInfo != NULL) {
+      //
+      // now get the information
+      //
+      Status = FileHandle->GetInfo(FileHandle,
+                                   &gEfiFileInfoGuid,
+                                   &FileInfoSize,
+                                   FileInfo);
+      //
+      // if we got an error free the memory and return NULL
+      //
+      if (EFI_ERROR(Status)) {
+        FreePool(FileInfo);
+        FileInfo = NULL;
+      }
     }
   }
   return (FileInfo);
-- 
2.17.0.windows.1

[PATCH v2] MdePkg/UefiFileHandleLib: Fix the Root directory determination.

[Marvin Häuser]

The current implementation of the FileHandleGetFileName() function
assumes that the Root directory always has the FileName '\0'.
However, the only requirement the UEFI specification defines is that
a prepended '\\' must be supported to access files and folders
relative to the Root directory.
This patch removes this assumption and supports constructing valid
paths for any value of FileName for the Root Directory.

In praxis, this fixes compatibility issues with File System drivers
that report '\\' as the FileName of the Root directory, which
currently is both generating an invalid path ("\\\\") and resulting
in an EFI_NOT_FOUND result from the CurrentHandle->Open() call.

V2:
  - Do not change the copyright date as requested.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
---
 MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 27 ++++++++++++++------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
index 57aad77bc135..251cbc55edb5 100644
--- a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
+++ b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
@@ -820,10 +820,25 @@ FileHandleGetFileName (
         Status = EFI_OUT_OF_RESOURCES;
         break;
       } else {
+        //
+        // Prepare to move to the parent directory.
+        // Also determine whether CurrentHandle refers to the Root directory.
+        //
+        Status = CurrentHandle->Open (CurrentHandle, &NextHigherHandle, L"..", EFI_FILE_MODE_READ, 0);
         //
         // We got info... do we have a name? if yes precede the current path with it...
         //
-        if (StrLen (FileInfo->FileName) == 0) {
+        if ((StrLen (FileInfo->FileName) == 0) || EFI_ERROR (Status)) {
+          //
+          // Both FileInfo->FileName being '\0' and EFI_ERROR() suggest that
+          // CurrentHandle refers to the Root directory.  As this loop ensures
+          // FullFileName is starting with '\\' at all times, signal success
+          // and exit the loop.
+          // While FileInfo->FileName could theoretically be a value other than
+          // '\0' or '\\', '\\' is guaranteed to be supported by the
+          // specification and hence its value can safely be ignored.
+          //
+          Status = EFI_SUCCESS;
           if (*FullFileName == NULL) {
             ASSERT((*FullFileName == NULL && Size == 0) || (*FullFileName != NULL));
             *FullFileName = StrnCatGrowLeft(FullFileName, &Size, L"\\", 0);
@@ -841,15 +856,11 @@ FileHandleGetFileName (
           FreePool(FileInfo);
         }
       }
-      //
-      // Move to the parent directory
-      //
-      Status = CurrentHandle->Open (CurrentHandle, &NextHigherHandle, L"..", EFI_FILE_MODE_READ, 0);
-      if (EFI_ERROR (Status)) {
-        break;
-      }
 
       FileHandleClose(CurrentHandle);
+      //
+      // Move to the parent directory
+      //
       CurrentHandle = NextHigherHandle;
     }
   } else if (Status == EFI_NOT_FOUND) {
-- 
2.17.0.windows.1

[PATCH v2] MdePkg: Update MmSwDispatch.h's references to SmmSw2Dispatch.

[Marvin Häuser]

MmSwDispatch.h current refers to the deprecated SmmSw2Dispatch
protocol. Replace those references with the new MmSwDispatch name.

V2:
  - Do not change the copyright date as requested.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
---
 MdePkg/Include/Protocol/MmSwDispatch.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/MdePkg/Include/Protocol/MmSwDispatch.h b/MdePkg/Include/Protocol/MmSwDispatch.h
index d3acb64e6f3d..e412bb2e66c0 100644
--- a/MdePkg/Include/Protocol/MmSwDispatch.h
+++ b/MdePkg/Include/Protocol/MmSwDispatch.h
@@ -15,8 +15,8 @@
 
 **/
 
-#ifndef _MM_SW_DISPATCH2_H_
-#define _MM_SW_DISPATCH2_H_
+#ifndef _MM_SW_DISPATCH_H_
+#define _MM_SW_DISPATCH_H_
 
 #include <Pi/PiMmCis.h>
 
@@ -117,7 +117,7 @@ EFI_STATUS
 ///
 /// Interface structure for the MM Software MMI Dispatch Protocol.
 ///
-/// The EFI_MM_SW_DISPATCH2_PROTOCOL provides the ability to install child handlers for the
+/// The EFI_MM_SW_DISPATCH_PROTOCOL provides the ability to install child handlers for the
 /// given software.  These handlers will respond to software interrupts, and the maximum software
 /// interrupt in the EFI_MM_SW_REGISTER_CONTEXT is denoted by MaximumSwiValue.
 ///
-- 
2.17.0.windows.1

Re: [PATCH v2] MdePkg: Update MmSwDispatch.h's references to SmmSw2Dispatch.

[Gao, Liming]

Reviewed-by: Liming Gao <liming.gao@intel.com>

>-----Original Message-----
>From: Marvin Häuser [mailto:Marvin.Haeuser@outlook.com]
>Sent: Thursday, May 17, 2018 8:42 PM
>To: edk2-devel@lists.01.org
>Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
><liming.gao@intel.com>
>Subject: [PATCH v2] MdePkg: Update MmSwDispatch.h's references to
>SmmSw2Dispatch.
>
>MmSwDispatch.h current refers to the deprecated SmmSw2Dispatch
>protocol. Replace those references with the new MmSwDispatch name.
>
>V2:
>  - Do not change the copyright date as requested.
>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
>---
> MdePkg/Include/Protocol/MmSwDispatch.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/MdePkg/Include/Protocol/MmSwDispatch.h
>b/MdePkg/Include/Protocol/MmSwDispatch.h
>index d3acb64e6f3d..e412bb2e66c0 100644
>--- a/MdePkg/Include/Protocol/MmSwDispatch.h
>+++ b/MdePkg/Include/Protocol/MmSwDispatch.h
>@@ -15,8 +15,8 @@
>
> **/
>
>-#ifndef _MM_SW_DISPATCH2_H_
>-#define _MM_SW_DISPATCH2_H_
>+#ifndef _MM_SW_DISPATCH_H_
>+#define _MM_SW_DISPATCH_H_
>
> #include <Pi/PiMmCis.h>
>
>@@ -117,7 +117,7 @@ EFI_STATUS
> ///
> /// Interface structure for the MM Software MMI Dispatch Protocol.
> ///
>-/// The EFI_MM_SW_DISPATCH2_PROTOCOL provides the ability to install
>child handlers for the
>+/// The EFI_MM_SW_DISPATCH_PROTOCOL provides the ability to install
>child handlers for the
> /// given software.  These handlers will respond to software interrupts, and
>the maximum software
> /// interrupt in the EFI_MM_SW_REGISTER_CONTEXT is denoted by
>MaximumSwiValue.
> ///
>--
>2.17.0.windows.1

Re: [PATCH v2] MdePkg/UefiFileHandleLib: Fix the Root directory determination.

[Marvin Häuser]

Hey Liming and Mike,

Sorry, but I have seen this and another patch to UefiFileHandleLib have not been reviewed and/or merged yet.
Is there anything wrong with the patches?

Thanks,
Marvin.

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Marvin
> Häuser
> Sent: Thursday, May 17, 2018 2:42 PM
> To: edk2-devel@lists.01.org
> Cc: michael.d.kinney@intel.com; liming.gao@intel.com
> Subject: [edk2] [PATCH v2] MdePkg/UefiFileHandleLib: Fix the Root directory
> determination.
> 
> The current implementation of the FileHandleGetFileName() function
> assumes that the Root directory always has the FileName '\0'.
> However, the only requirement the UEFI specification defines is that a
> prepended '\\' must be supported to access files and folders relative to the
> Root directory.
> This patch removes this assumption and supports constructing valid paths for
> any value of FileName for the Root Directory.
> 
> In praxis, this fixes compatibility issues with File System drivers that report
> '\\' as the FileName of the Root directory, which currently is both generating
> an invalid path ("\\\\") and resulting in an EFI_NOT_FOUND result from the
> CurrentHandle->Open() call.
> 
> V2:
>   - Do not change the copyright date as requested.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> ---
>  MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 27
> ++++++++++++++------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> index 57aad77bc135..251cbc55edb5 100644
> --- a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> +++ b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> @@ -820,10 +820,25 @@ FileHandleGetFileName (
>          Status = EFI_OUT_OF_RESOURCES;
>          break;
>        } else {
> +        //
> +        // Prepare to move to the parent directory.
> +        // Also determine whether CurrentHandle refers to the Root directory.
> +        //
> +        Status = CurrentHandle->Open (CurrentHandle, &NextHigherHandle,
> + L"..", EFI_FILE_MODE_READ, 0);
>          //
>          // We got info... do we have a name? if yes precede the current path
> with it...
>          //
> -        if (StrLen (FileInfo->FileName) == 0) {
> +        if ((StrLen (FileInfo->FileName) == 0) || EFI_ERROR (Status)) {
> +          //
> +          // Both FileInfo->FileName being '\0' and EFI_ERROR() suggest that
> +          // CurrentHandle refers to the Root directory.  As this loop ensures
> +          // FullFileName is starting with '\\' at all times, signal success
> +          // and exit the loop.
> +          // While FileInfo->FileName could theoretically be a value other than
> +          // '\0' or '\\', '\\' is guaranteed to be supported by the
> +          // specification and hence its value can safely be ignored.
> +          //
> +          Status = EFI_SUCCESS;
>            if (*FullFileName == NULL) {
>              ASSERT((*FullFileName == NULL && Size == 0) || (*FullFileName !=
> NULL));
>              *FullFileName = StrnCatGrowLeft(FullFileName, &Size, L"\\", 0); @@ -
> 841,15 +856,11 @@ FileHandleGetFileName (
>            FreePool(FileInfo);
>          }
>        }
> -      //
> -      // Move to the parent directory
> -      //
> -      Status = CurrentHandle->Open (CurrentHandle, &NextHigherHandle,
> L"..", EFI_FILE_MODE_READ, 0);
> -      if (EFI_ERROR (Status)) {
> -        break;
> -      }
> 
>        FileHandleClose(CurrentHandle);
> +      //
> +      // Move to the parent directory
> +      //
>        CurrentHandle = NextHigherHandle;
>      }
>    } else if (Status == EFI_NOT_FOUND) {
> --
> 2.17.0.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v2] MdePkg/UefiFileHandleLib: Fix the Root directory determination.

[Ni, Ruiyu]

Thanks/Ray

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Marvin
> Häuser
> Sent: Thursday, May 17, 2018 8:42 PM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: [edk2] [PATCH v2] MdePkg/UefiFileHandleLib: Fix the Root directory
> determination.
> 
> The current implementation of the FileHandleGetFileName() function
> assumes that the Root directory always has the FileName '\0'.
> However, the only requirement the UEFI specification defines is that
> a prepended '\\' must be supported to access files and folders
> relative to the Root directory.
> This patch removes this assumption and supports constructing valid
> paths for any value of FileName for the Root Directory.
> 
> In praxis, this fixes compatibility issues with File System drivers
> that report '\\' as the FileName of the Root directory, which
> currently is both generating an invalid path ("\\\\") and resulting
> in an EFI_NOT_FOUND result from the CurrentHandle->Open() call.
> 
> V2:
>   - Do not change the copyright date as requested.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> ---
>  MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 27
> ++++++++++++++------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> index 57aad77bc135..251cbc55edb5 100644
> --- a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> +++ b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> @@ -820,10 +820,25 @@ FileHandleGetFileName (
>          Status = EFI_OUT_OF_RESOURCES;
>          break;
>        } else {
> +        //
> +        // Prepare to move to the parent directory.
> +        // Also determine whether CurrentHandle refers to the Root directory.
> +        //
> +        Status = CurrentHandle->Open (CurrentHandle, &NextHigherHandle,
> L"..", EFI_FILE_MODE_READ, 0);
>          //
>          // We got info... do we have a name? if yes precede the current path
> with it...
>          //
> -        if (StrLen (FileInfo->FileName) == 0) {
> +        if ((StrLen (FileInfo->FileName) == 0) || EFI_ERROR (Status)) {
> +          //
> +          // Both FileInfo->FileName being '\0' and EFI_ERROR() suggest that
> +          // CurrentHandle refers to the Root directory.  As this loop ensures
> +          // FullFileName is starting with '\\' at all times, signal success
> +          // and exit the loop.
> +          // While FileInfo->FileName could theoretically be a value other than
> +          // '\0' or '\\', '\\' is guaranteed to be supported by the
> +          // specification and hence its value can safely be ignored.
> +          //

Per spec, Open("..") request on root directory should return error status.
So I think to keep code simple enough, we could remove the "StrLen(..) == 0"
check.

UEFI Spec 2.7:
".."   Opens the parent directory for the current location. If the location is the
root directory the request will return an error, as there is no parent directory
for the root directory.


> +          Status = EFI_SUCCESS;
>            if (*FullFileName == NULL) {
>              ASSERT((*FullFileName == NULL && Size == 0) || (*FullFileName !=
> NULL));
>              *FullFileName = StrnCatGrowLeft(FullFileName, &Size, L"\\", 0);
> @@ -841,15 +856,11 @@ FileHandleGetFileName (
>            FreePool(FileInfo);
>          }
>        }
> -      //
> -      // Move to the parent directory
> -      //
> -      Status = CurrentHandle->Open (CurrentHandle, &NextHigherHandle,
> L"..", EFI_FILE_MODE_READ, 0);
> -      if (EFI_ERROR (Status)) {
> -        break;
> -      }
> 
>        FileHandleClose(CurrentHandle);
> +      //
> +      // Move to the parent directory
> +      //
>        CurrentHandle = NextHigherHandle;
>      }
>    } else if (Status == EFI_NOT_FOUND) {
> --
> 2.17.0.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel