From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org 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 6F6C6211BED6C for ; Wed, 30 Jan 2019 06:42:14 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 42C8180F7B; Wed, 30 Jan 2019 14:42:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-123-147.rdu2.redhat.com [10.10.123.147]) by smtp.corp.redhat.com (Postfix) with ESMTP id BDE2560C6E; Wed, 30 Jan 2019 14:42:10 +0000 (UTC) To: Neo Hsueh , edk2-devel@lists.01.org Cc: Michael D Kinney , Liming Gao , Dandan Bi References: <20190129185048.17500-1-hong-chihx.hsueh@intel.com> From: Laszlo Ersek Message-ID: Date: Wed, 30 Jan 2019 15:42:09 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190129185048.17500-1-hong-chihx.hsueh@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Wed, 30 Jan 2019 14:42:13 +0000 (UTC) Subject: Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info is invalid X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 30 Jan 2019 14:42:14 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/29/19 19:50, Neo Hsueh wrote: > Skip runtime relocation for PE images that provide invalid relocation > infomation (ex: RelocDir->Size = 0) to fix a hang observed while booting > Windows. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Neo Hsueh > Cc: Michael D Kinney > Cc: Liming Gao > Cc: Dandan Bi > Cc: Laszlo Ersek > --- > MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 28 ++++++++++++++++++---------- > 1 file changed, 18 insertions(+), 10 deletions(-) > > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > index 1bd079ad6a..3a46e626e4 100644 > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > @@ -1002,7 +1002,7 @@ PeCoffLoaderRelocateImage ( > RelocDir->VirtualAddress + RelocDir->Size - 1, > TeStrippedOffset > ); > - if (RelocBase == NULL || RelocBaseEnd == NULL || RelocBaseEnd < RelocBase) { > + if (RelocBase == NULL || RelocBaseEnd == NULL || (UINTN) RelocBaseEnd < (UINTN) RelocBase) { > ImageContext->ImageError = IMAGE_ERROR_FAILED_RELOCATION; > return RETURN_LOAD_ERROR; > } > @@ -1022,7 +1022,7 @@ PeCoffLoaderRelocateImage ( > // Run the relocation information and apply the fixups > // > FixupData = ImageContext->FixupData; > - while (RelocBase < RelocBaseEnd) { > + while ((UINTN) RelocBase < (UINTN) RelocBaseEnd) { > > Reloc = (UINT16 *) ((CHAR8 *) RelocBase + sizeof (EFI_IMAGE_BASE_RELOCATION)); > // > @@ -1051,7 +1051,7 @@ PeCoffLoaderRelocateImage ( > // > // Run this relocation record > // > - while (Reloc < RelocEnd) { > + while ((UINTN) Reloc < (UINTN) RelocEnd) { > Fixup = PeCoffLoaderImageAddress (ImageContext, RelocBase->VirtualAddress + (*Reloc & 0xFFF), TeStrippedOffset); > if (Fixup == NULL) { > ImageContext->ImageError = IMAGE_ERROR_FAILED_RELOCATION; > @@ -1741,11 +1741,19 @@ PeCoffLoaderRelocateImageForRuntime ( > // > if (NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC) { > RelocDir = DataDirectory + EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC; > - RelocBase = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress (&ImageContext, RelocDir->VirtualAddress, 0); > - RelocBaseEnd = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress (&ImageContext, > - RelocDir->VirtualAddress + RelocDir->Size - 1, > - 0 > - ); > + if ((RelocDir != NULL) && (RelocDir->Size > 0)) { > + RelocBase = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress (&ImageContext, RelocDir->VirtualAddress, 0); > + RelocBaseEnd = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress (&ImageContext, > + RelocDir->VirtualAddress + RelocDir->Size - 1, > + 0 > + ); > + } If the above check fails, then RelocBase and RelocBaseEnd will not be assigned, and the expressions below will check the previous values of those variables. What guarantees that those values are valid / not indeterminate? Thanks Laszlo > + if (RelocBase == NULL || RelocBaseEnd == NULL || (UINTN) RelocBaseEnd < (UINTN) RelocBase) { > + // > + // relocation block is not valid, just return > + // > + return; > + } > } else { > // > // Cannot find relocations, cannot continue to relocate the image, ASSERT for this invalid image. > @@ -1769,7 +1777,7 @@ PeCoffLoaderRelocateImageForRuntime ( > // > FixupData = RelocationData; > RelocBaseOrig = RelocBase; > - while (RelocBase < RelocBaseEnd) { > + while ((UINTN) RelocBase < (UINTN) RelocBaseEnd) { > // > // Add check for RelocBase->SizeOfBlock field. > // > @@ -1794,7 +1802,7 @@ PeCoffLoaderRelocateImageForRuntime ( > // > // Run this relocation record > // > - while (Reloc < RelocEnd) { > + while ((UINTN) Reloc < (UINTN) RelocEnd) { > > Fixup = PeCoffLoaderImageAddress (&ImageContext, RelocBase->VirtualAddress + (*Reloc & 0xFFF), 0); > if (Fixup == NULL) { >