Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if relocation info is invalid.

[Laszlo Ersek <lersek@redhat.com>]

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 <hong-chihx.hsueh@intel.com>; edk2-devel@lists.01.org
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; Bi, Dandan <dandan.bi@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.hsueh@intel.com>
>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>> Cc: Liming Gao <liming.gao@intel.com>
>>> Cc: Dandan Bi <dandan.bi@intel.com>
>>> Cc: Laszlo Ersek <lersek@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
>