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 7508F211BB8B9 for ; Tue, 29 Jan 2019 02:55:14 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 002DF58E4B; Tue, 29 Jan 2019 10:55:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-45.rdu2.redhat.com [10.10.121.45]) by smtp.corp.redhat.com (Postfix) with ESMTP id B58465D717; Tue, 29 Jan 2019 10:55:12 +0000 (UTC) To: "Bi, Dandan" , "Hsueh, Hong-chihX" , "edk2-devel@lists.01.org" Cc: "Kinney, Michael D" , "Gao, Liming" References: <20190128184047.20792-1-hong-chihx.hsueh@intel.com> <3C0D5C461C9E904E8F62152F6274C0BB40B8E138@SHSMSX104.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: Date: Tue, 29 Jan 2019 11:55:11 +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: <3C0D5C461C9E904E8F62152F6274C0BB40B8E138@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Tue, 29 Jan 2019 10:55:14 +0000 (UTC) Subject: Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation 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: Tue, 29 Jan 2019 10:55:14 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/29/19 06:13, Bi, Dandan wrote: > Hi Neo, > > Thank you very much for the patch. > > Some minor comments > 1) Besides the skip check in this patch, could you help to add additional check for RelocDir->Size before calling PeCoffLoaderImageAddress to calculate the RelocBase and RelocBaseEnd? > Since when RelocDir->Size==0, we can just return, don't need to do the calculation. I agree, checking Size against 0 before calling PeCoffLoaderImageAddress() is best; we do the same in PeCoffLoaderRelocateImage(): if ((RelocDir != NULL) && (RelocDir->Size > 0)) { Thanks, Laszlo > > 2) Please use the PatchCheck.py in edk2\BaseTools\Scripts to check the patch format before committing the patch. > Can refer following link for more info: > https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format > > > Thanks, > Dandan > >> -----Original Message----- >> From: Hsueh, Hong-chihX >> Sent: Tuesday, January 29, 2019 2:41 AM >> To: edk2-devel@lists.01.org >> Cc: Kinney, Michael D ; Gao, Liming >> ; Bi, Dandan ; Laszlo Ersek >> >> Subject: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if >> relocation info is invalid. >> >> 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 | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c >> b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c >> index 1bd079ad6a..f01c691dea 100644 >> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c >> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c >> @@ -1746,6 +1746,12 @@ PeCoffLoaderRelocateImageForRuntime ( >> RelocDir->VirtualAddress + RelocDir- >>> Size - 1, >> 0 >> ); >> + if (RelocBase == NULL || RelocBaseEnd == NULL || RelocBaseEnd < >> RelocBase) { >> + // >> + // relocation block is not valid, just return >> + // >> + return; >> + } >> } else { >> // >> // Cannot find relocations, cannot continue to relocate the image, ASSERT >> for this invalid image. >> -- >> 2.16.2.windows.1 >