Re: [Patch] MdeModulePkg FileExplorerLib: Fix potential Integer Overflow.

[Laszlo Ersek <lersek@redhat.com>]

On 10/14/16 08:49, Liming Gao wrote:
> In function 'LibAppendFileName' of 'FileExplorer.c':
> "
> MaxLen = (Size1 + Size2 + sizeof (CHAR16))/ sizeof (CHAR16);
> "
> Overflow may happen here. MaxLen might become a very small number.
> This patch adds integer overflow checker.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Liming Gao <liming.gao@intel.com>
> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  .../Library/FileExplorerLib/FileExplorer.c         | 28 ++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)

The first hunk of the patch looks good to me.

The rest of the patch may also be good, but what it does is logically
separate -- it eliminates resource leaks and fixes return values on
error paths. For that reason, it belongs to a different patch, I think;
something like:

  MdeModulePkg FileExplorerLib: free resources and set retvals on errors

Thanks
Laszlo

> diff --git a/MdeModulePkg/Library/FileExplorerLib/FileExplorer.c b/MdeModulePkg/Library/FileExplorerLib/FileExplorer.c
> index 59c851b..41a22aa 100644
> --- a/MdeModulePkg/Library/FileExplorerLib/FileExplorer.c
> +++ b/MdeModulePkg/Library/FileExplorerLib/FileExplorer.c
> @@ -620,6 +620,14 @@ LibAppendFileName (
>  
>    Size1 = StrSize (Str1);
>    Size2 = StrSize (Str2);
> +  
> +  //
> +  // Check overflow
> +  //
> +  if (((MAX_UINTN - Size1) < Size2) || ((MAX_UINTN - Size1 - Size2) < sizeof(CHAR16))) {
> +    return NULL;
> +  }
> +  
>    MaxLen = (Size1 + Size2 + sizeof (CHAR16))/ sizeof (CHAR16);
>    Str   = AllocateZeroPool (Size1 + Size2 + sizeof (CHAR16));
>    ASSERT (Str != NULL);
> @@ -963,6 +971,7 @@ LibGetFileHandleFromDevicePath (
>      // the file system support below to be skipped.
>      //
>      Status = EFI_OUT_OF_RESOURCES;
> +    goto Done;
>    }
>          
>    //
> @@ -992,6 +1001,11 @@ LibGetFileHandleFromDevicePath (
>        *ParentFileName = AllocateCopyPool (StrSize (((FILEPATH_DEVICE_PATH *) DevicePathNode)->PathName), ((FILEPATH_DEVICE_PATH *) DevicePathNode)->PathName);
>      } else {
>        TempPath = LibAppendFileName (*ParentFileName, ((FILEPATH_DEVICE_PATH *) DevicePathNode)->PathName);
> +      if (TempPath == NULL) {
> +        LastHandle->Close (LastHandle);
> +        Status = EFI_OUT_OF_RESOURCES;
> +        goto Done;
> +      }
>        FreePool (*ParentFileName);
>        *ParentFileName = TempPath;
>      }
> @@ -1067,12 +1081,14 @@ LibFindFiles (
>    // Pass 1 to get Directories
>    // Pass 2 to get files that are EFI images
>    //
> +  Status = EFI_SUCCESS;
>    for (Pass = 1; Pass <= 2; Pass++) {
>      FileHandle->SetPosition (FileHandle, 0);
>      for (;;) {
>        BufferSize  = DirBufferSize;
>        Status      = FileHandle->Read (FileHandle, &BufferSize, DirInfo);
>        if (EFI_ERROR (Status) || BufferSize == 0) {
> +        Status = EFI_SUCCESS;
>          break;
>        }
>  
> @@ -1095,12 +1111,18 @@ LibFindFiles (
>  
>        NewMenuEntry = LibCreateMenuEntry ();
>        if (NULL == NewMenuEntry) {
> -        return EFI_OUT_OF_RESOURCES;
> +        Status = EFI_OUT_OF_RESOURCES;
> +        goto Done;
>        }
>  
>        NewFileContext = (FILE_CONTEXT *) NewMenuEntry->VariableContext;
>        NewFileContext->DeviceHandle = DeviceHandle;
>        NewFileContext->FileName = LibAppendFileName (FileName, DirInfo->FileName);
> +      if  (NewFileContext->FileName == NULL) {
> +        LibDestroyMenuEntry (NewMenuEntry);
> +        Status = EFI_OUT_OF_RESOURCES;
> +        goto Done;
> +      }
>        NewFileContext->FileHandle = FileHandle;
>        NewFileContext->DevicePath = FileDevicePath (NewFileContext->DeviceHandle, NewFileContext->FileName);
>        NewMenuEntry->HelpString = NULL;
> @@ -1135,9 +1157,11 @@ LibFindFiles (
>  
>    gFileExplorerPrivate.FsOptionMenu->MenuNumber = OptionNumber;
>  
> +Done:
> +
>    FreePool (DirInfo);
>  
> -  return EFI_SUCCESS;
> +  return Status;
>  }
>  
>  /**
>