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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel