public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
To: Neo Hsueh <hong-chihx.hsueh@intel.com>, edk2-devel@lists.01.org
Cc: Michael D Kinney <michael.d.kinney@intel.com>,
	Liming Gao <liming.gao@intel.com>,
	Dandan Bi <dandan.bi@intel.com>
Subject: Re: [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info is invalid
Date: Wed, 30 Jan 2019 15:42:09 +0100	[thread overview]
Message-ID: <fa042de2-2bbf-9971-5eac-0abf6755dc51@redhat.com> (raw)
In-Reply-To: <20190129185048.17500-1-hong-chihx.hsueh@intel.com>

On 01/29/19 19:50, 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 | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> index 1bd079ad6a..3a46e626e4 100644
> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> @@ -1002,7 +1002,7 @@ PeCoffLoaderRelocateImage (
>                                                                              RelocDir->VirtualAddress + RelocDir->Size - 1,
>                                                                              TeStrippedOffset
>                                                                              );
> -    if (RelocBase == NULL || RelocBaseEnd == NULL || RelocBaseEnd < RelocBase) {
> +    if (RelocBase == NULL || RelocBaseEnd == NULL || (UINTN) RelocBaseEnd < (UINTN) RelocBase) {
>        ImageContext->ImageError = IMAGE_ERROR_FAILED_RELOCATION;
>        return RETURN_LOAD_ERROR;
>      }
> @@ -1022,7 +1022,7 @@ PeCoffLoaderRelocateImage (
>      // Run the relocation information and apply the fixups
>      //
>      FixupData = ImageContext->FixupData;
> -    while (RelocBase < RelocBaseEnd) {
> +    while ((UINTN) RelocBase < (UINTN) RelocBaseEnd) {
>  
>        Reloc     = (UINT16 *) ((CHAR8 *) RelocBase + sizeof (EFI_IMAGE_BASE_RELOCATION));
>        //
> @@ -1051,7 +1051,7 @@ PeCoffLoaderRelocateImage (
>        //
>        // Run this relocation record
>        //
> -      while (Reloc < RelocEnd) {
> +      while ((UINTN) Reloc < (UINTN) RelocEnd) {
>          Fixup = PeCoffLoaderImageAddress (ImageContext, RelocBase->VirtualAddress + (*Reloc & 0xFFF), TeStrippedOffset);
>          if (Fixup == NULL) {
>            ImageContext->ImageError = IMAGE_ERROR_FAILED_RELOCATION;
> @@ -1741,11 +1741,19 @@ PeCoffLoaderRelocateImageForRuntime (
>    //
>    if (NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC) {
>      RelocDir      = DataDirectory + EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC;
> -    RelocBase     = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress (&ImageContext, RelocDir->VirtualAddress, 0);
> -    RelocBaseEnd  = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress (&ImageContext,
> -                                                                            RelocDir->VirtualAddress + RelocDir->Size - 1,
> -                                                                            0
> -                                                                            );
> +    if ((RelocDir != NULL) && (RelocDir->Size > 0)) {
> +      RelocBase     = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress (&ImageContext, RelocDir->VirtualAddress, 0);
> +      RelocBaseEnd  = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress (&ImageContext,
> +                                                                              RelocDir->VirtualAddress + RelocDir->Size - 1,
> +                                                                              0
> +                                                                              );
> +    }

If the above check fails, then RelocBase and RelocBaseEnd will not be
assigned, and the expressions below will check the previous values of
those variables. What guarantees that those values are valid / not
indeterminate?

Thanks
Laszlo

> +    if (RelocBase == NULL || RelocBaseEnd == NULL || (UINTN) RelocBaseEnd < (UINTN) RelocBase) {
> +      //
> +      // relocation block is not valid, just return
> +      //
> +      return;
> +    }
>    } else {
>      //
>      // Cannot find relocations, cannot continue to relocate the image, ASSERT for this invalid image.
> @@ -1769,7 +1777,7 @@ PeCoffLoaderRelocateImageForRuntime (
>      //
>      FixupData = RelocationData;
>      RelocBaseOrig = RelocBase;
> -    while (RelocBase < RelocBaseEnd) {
> +    while ((UINTN) RelocBase < (UINTN) RelocBaseEnd) {
>        //
>        // Add check for RelocBase->SizeOfBlock field.
>        //
> @@ -1794,7 +1802,7 @@ PeCoffLoaderRelocateImageForRuntime (
>        //
>        // Run this relocation record
>        //
> -      while (Reloc < RelocEnd) {
> +      while ((UINTN) Reloc < (UINTN) RelocEnd) {
>  
>          Fixup = PeCoffLoaderImageAddress (&ImageContext, RelocBase->VirtualAddress + (*Reloc & 0xFFF), 0);
>          if (Fixup == NULL) {
> 



  reply	other threads:[~2019-01-30 14:42 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-29 18:50 [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if reloc info is invalid Neo Hsueh
2019-01-30 14:42 ` Laszlo Ersek [this message]
  -- strict thread matches above, loose matches on Subject: below --
2019-01-30  1:19 Neo Hsueh
2019-01-30 14:45 ` Laszlo Ersek
2019-01-31  0:27 ` Bi, Dandan
2019-01-31  3:30   ` Gao, Liming
2019-01-31  3:38     ` Gao, Liming

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fa042de2-2bbf-9971-5eac-0abf6755dc51@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox