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....@intel.com> > Reviewed-by: Jiewen Yao <jiewen....@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; > } > > /** > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel