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 B1622211BB8B9 for ; Tue, 29 Jan 2019 02:52:44 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E4E6A2D4B7A; Tue, 29 Jan 2019 10:52:43 +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 7B89B600C6; Tue, 29 Jan 2019 10:52:42 +0000 (UTC) To: "Hsueh, Hong-chihX" , "edk2-devel@lists.01.org" Cc: "Kinney, Michael D" , "Gao, Liming" , "Bi, Dandan" References: <20190128184047.20792-1-hong-chihx.hsueh@intel.com> <0313d273-69f2-af63-bbff-8d561aaf8bbd@redhat.com> <1F8AF101AC6B5D47B4348ACCED214431CA59DADB@fmsmsx107.amr.corp.intel.com> From: Laszlo Ersek Message-ID: <2cc91c1c-e1cf-6a00-3944-db6415d66961@redhat.com> Date: Tue, 29 Jan 2019 11:52:41 +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: <1F8AF101AC6B5D47B4348ACCED214431CA59DADB@fmsmsx107.amr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Tue, 29 Jan 2019 10:52:44 +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:52:46 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/29/19 00:40, Hsueh, Hong-chihX wrote: >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Monday, January 28, 2019 2:17 PM >> To: Hsueh, Hong-chihX ; edk2-devel@lists.01.org >> Cc: Kinney, Michael D ; Gao, Liming >> ; Bi, Dandan >> Subject: Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if >> relocation info is invalid. >> >> On 01/28/19 19:40, 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 | 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. >>> >> >> Thank you for the update. >> >> ... Originally I meant to respond with an Acked-by (purely from a formal point- >> of-view); however I figured the patch wasn't large and I could check it for a >> Reviewed-by as well. >> >> I'm noticing the comparison (RelocBaseEnd < RelocBase) is supposed to catch >> invalid relocation info. These variables are pointers, declared as >> follows: >> >> EFI_IMAGE_BASE_RELOCATION *RelocBase; >> EFI_IMAGE_BASE_RELOCATION *RelocBaseEnd; >> >> According to the C standard, the relational operators can only be applied to a >> pair of pointers if each of those points into the same array, or one past the last >> element. In this case, given that you intend to catch invalid relocation info, >> that's exactly *not* the case. In other words, in the only case when the >> relational operator would evaluate to true, it would also invoke undefined >> behavior. Furthermore, the byte distance between the pointed-to-objects might >> not even be a whole multiple of sizeof (EFI_IMAGE_BASE_RELOCATION). >> >> Normally I would suggest changing the return type of >> PeCoffLoaderImageAddress() to UINTN -- that would be fitting because the >> internal computation is already performed in UINTN, and only cast to >> (CHAR8 *) as last step. This way we could move the cast to the callers, and >> perform the sanity checks before the conversion to (VOID*) (or to other pointer >> types). >> >> I do see the function is called from many places, so this change might be too >> costly. Can we at least write in this patch, >> >> if (RelocBase == NULL || >> RelocBaseEnd == NULL || >> (UINTN)RelocBaseEnd < (UINTN)RelocBase || >> (((UINTN)RelocBaseEnd - (UINTN)RelocBase) % >> sizeof (EFI_IMAGE_BASE_RELOCATION) != 0)) { >> return; >> } >> >> ? >> >> Perhaps we should even extract this logic to a helper function, because I see >> another spot with the same condition. That's in PeCoffLoaderRelocateImage(), >> from the top of commit a8d8d430510d ("Support load 64 bit image from 32 bit >> core. Add more enhancement to check invalid PE format.", 2014-03-25). >> >> I'm sorry that I didn't manage to make these suggestions under the v1 posting. >> >> Thanks, >> Laszlo > > Hi Laszlo, > Thank you. I agree the pointer comparison is not optimal especially in this case. > However I didn't add multiple of size (EFI_IMAGE_BASE_RELOCATION) check because from the commit eb76b762, we actually check the address range between Base to RelocDir->Size - 1. Thank you for pointing that out. I think that patch is not correct. We have: EFI_IMAGE_BASE_RELOCATION *RelocBase; EFI_IMAGE_BASE_RELOCATION *RelocBaseEnd; and RelocBase = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress (ImageContext, RelocDir->VirtualAddress, TeStrippedOffset); RelocBaseEnd = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress (ImageContext, RelocDir->VirtualAddress + RelocDir->Size - 1, TeStrippedOffset ); It is fine to make RelocBaseEnd an *inclusive* end pointer (if that is our goal -- I'm not sure why though), but in that case, we should not cast the result to (EFI_IMAGE_BASE_RELOCATION*), and we certainly shouldn't compare (RelocBase < RelocBaseEnd), when we know that RelocBaseEnd can never point to an EFI_IMAGE_BASE_RELOCATION, or precisely one past it. Thanks Laszlo > > Here is the updated patch : > https://lists.01.org/pipermail/edk2-devel/2019-January/035810.html > > Regards, > Neo >