> -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Monday, January 28, 2019 2:17 PM > To: Hsueh, Hong-chihX <hong-chihx.hs...@intel.com>; edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Gao, Liming > <liming....@intel.com>; Bi, Dandan <dandan...@intel.com> > 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 <hong-chihx.hs...@intel.com> > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > Cc: Liming Gao <liming....@intel.com> > > Cc: Dandan Bi <dandan...@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > --- > > 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. Here is the updated patch : https://lists.01.org/pipermail/edk2-devel/2019-January/035810.html Regards, Neo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
