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

[Laszlo Ersek <lersek@redhat.com>]

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