On 01/29/19 00:40, Hsueh, Hong-chihX wrote:
>> -----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.
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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
