From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 7E0FB1A1EC4 for ; Fri, 14 Oct 2016 05:18:53 -0700 (PDT) Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0B300C04B928; Fri, 14 Oct 2016 12:18:53 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-51.phx2.redhat.com [10.3.116.51]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9ECIpgY014897; Fri, 14 Oct 2016 08:18:52 -0400 To: Liming Gao , edk2-devel@ml01.01.org References: <1476427794-7140-1-git-send-email-liming.gao@intel.com> From: Laszlo Ersek Message-ID: <2d7ee43f-bf5d-0c9f-0391-7fce3552d334@redhat.com> Date: Fri, 14 Oct 2016 14:18:51 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1476427794-7140-1-git-send-email-liming.gao@intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Fri, 14 Oct 2016 12:18:53 +0000 (UTC) Subject: Re: [Patch] MdeModulePkg FileExplorerLib: Fix potential Integer Overflow. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 14 Oct 2016 12:18:53 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 > Reviewed-by: Jiewen Yao > --- > .../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; > } > > /** >