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 5635C21962301 for ; Mon, 28 Jan 2019 14:16:51 -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 5E8BC8AE40; Mon, 28 Jan 2019 22:16:51 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-122-66.rdu2.redhat.com [10.10.122.66]) by smtp.corp.redhat.com (Postfix) with ESMTP id E50FB600D7; Mon, 28 Jan 2019 22:16:49 +0000 (UTC) To: Neo Hsueh , edk2-devel@lists.01.org Cc: Michael D Kinney , Liming Gao , Dandan Bi References: <20190128184047.20792-1-hong-chihx.hsueh@intel.com> From: Laszlo Ersek Message-ID: <0313d273-69f2-af63-bbff-8d561aaf8bbd@redhat.com> Date: Mon, 28 Jan 2019 23:16:48 +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: <20190128184047.20792-1-hong-chihx.hsueh@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.25]); Mon, 28 Jan 2019 22:16:51 +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: Mon, 28 Jan 2019 22:16:52 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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