Re: [PATCH 1/2] UefiPayloadPkg/PayloadLoader: Fix bug in locating relocation section

["Guo Dong" <guo.dong@intel.com>]

Reviewed-by: Guo Dong <guo.dong@intel.com>

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Monday, June 28, 2021 11:27 PM
> To: devel@edk2.groups.io
> Cc: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
> <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> Subject: [PATCH 1/2] UefiPayloadPkg/PayloadLoader: Fix bug in locating
> relocation section
> 
> Per ELF spec, the DT_REL/DT_RELA tag in dynamic section stores the
> virtual address of the relocation section.
> 
> But today's code logic treats it as the section offset and finds
> the relocation section whose offset equals to DT_REL/DT_RELA.
> 
> The logic can work when the section offset equals to the section
> virtual address. But when the ELF is generated from the link script
> that reserves a sizeof(pe_header) in the file beginning, the section
> offset doesn't equal to section virtual address. Such logic can
> not find the relocation section.
> 
> The patch fixes this bug.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Maurice Ma <maurice.ma@intel.com>
> Cc: Guo Dong <guo.dong@intel.com>
> Cc: Benjamin You <benjamin.you@intel.com>
> ---
>  .../PayloadLoaderPeim/ElfLib/Elf32Lib.c       | 22 +++++++++++++------
>  .../PayloadLoaderPeim/ElfLib/Elf64Lib.c       | 22 +++++++++++++------
>  2 files changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf32Lib.c
> b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf32Lib.c
> index 3fa100ce4a..dd27d3ce59 100644
> --- a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf32Lib.c
> +++ b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf32Lib.c
> @@ -206,7 +206,7 @@ RelocateElf32Dynamic (
>    Elf32_Shdr                   *DynShdr;
> 
>    Elf32_Shdr                   *RelShdr;
> 
>    Elf32_Dyn                    *Dyn;
> 
> -  UINT32                       RelaOffset;
> 
> +  UINT32                       RelaAddress;
> 
>    UINT32                       RelaCount;
> 
>    UINT32                       RelaSize;
> 
>    UINT32                       RelaEntrySize;
> 
> @@ -246,7 +246,7 @@ RelocateElf32Dynamic (
>    //
> 
>    // 2. Locate the relocation section from the dynamic section.
> 
>    //
> 
> -  RelaOffset    = MAX_UINT32;
> 
> +  RelaAddress   = MAX_UINT32;
> 
>    RelaSize      = 0;
> 
>    RelaCount     = 0;
> 
>    RelaEntrySize = 0;
> 
> @@ -265,8 +265,8 @@ RelocateElf32Dynamic (
>          // based on the original file value and the memory base address.
> 
>          // For consistency, files do not contain relocation entries to ``correct''
> addresses in the dynamic structure.
> 
>          //
> 
> -        RelaOffset = Dyn->d_un.d_ptr - (UINT32) (UINTN) ElfCt-
> >PreferredImageAddress;
> 
> -        RelaType   = (Dyn->d_tag == DT_RELA) ? SHT_RELA: SHT_REL;
> 
> +        RelaAddress = Dyn->d_un.d_ptr;
> 
> +        RelaType    = (Dyn->d_tag == DT_RELA) ? SHT_RELA: SHT_REL;
> 
>          break;
> 
>        case DT_RELACOUNT:
> 
>        case DT_RELCOUNT:
> 
> @@ -285,7 +285,7 @@ RelocateElf32Dynamic (
>      }
> 
>    }
> 
> 
> 
> -  if (RelaOffset == MAX_UINT64) {
> 
> +  if (RelaAddress == MAX_UINT64) {
> 
>      ASSERT (RelaCount     == 0);
> 
>      ASSERT (RelaEntrySize == 0);
> 
>      ASSERT (RelaSize      == 0);
> 
> @@ -298,8 +298,16 @@ RelocateElf32Dynamic (
>    //
> 
>    // Verify the existence of the relocation section.
> 
>    //
> 
> -  RelShdr = GetElf32SectionByRange (ElfCt->FileBase, RelaOffset, RelaSize);
> 
> -  ASSERT (RelShdr != NULL);
> 
> +  RelShdr = NULL;
> 
> +  for (Index = 0; Index < ElfCt->ShNum; Index++) {
> 
> +    RelShdr = GetElf32SectionByIndex (ElfCt->FileBase, Index);
> 
> +    ASSERT (RelShdr != NULL);
> 
> +    if ((RelShdr->sh_addr == RelaAddress) && (RelShdr->sh_size == RelaSize))
> {
> 
> +      break;
> 
> +    }
> 
> +    RelShdr = NULL;
> 
> +  }
> 
> +
> 
>    if (RelShdr == NULL) {
> 
>      return EFI_UNSUPPORTED;
> 
>    }
> 
> diff --git a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf64Lib.c
> b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf64Lib.c
> index e364807007..3f4f12903c 100644
> --- a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf64Lib.c
> +++ b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf64Lib.c
> @@ -215,7 +215,7 @@ RelocateElf64Dynamic (
>    Elf64_Shdr                   *DynShdr;
> 
>    Elf64_Shdr                   *RelShdr;
> 
>    Elf64_Dyn                    *Dyn;
> 
> -  UINT64                       RelaOffset;
> 
> +  UINT64                       RelaAddress;
> 
>    UINT64                       RelaCount;
> 
>    UINT64                       RelaSize;
> 
>    UINT64                       RelaEntrySize;
> 
> @@ -255,7 +255,7 @@ RelocateElf64Dynamic (
>    //
> 
>    // 2. Locate the relocation section from the dynamic section.
> 
>    //
> 
> -  RelaOffset    = MAX_UINT64;
> 
> +  RelaAddress    = MAX_UINT64;
> 
>    RelaSize      = 0;
> 
>    RelaCount     = 0;
> 
>    RelaEntrySize = 0;
> 
> @@ -274,8 +274,8 @@ RelocateElf64Dynamic (
>          // based on the original file value and the memory base address.
> 
>          // For consistency, files do not contain relocation entries to ``correct''
> addresses in the dynamic structure.
> 
>          //
> 
> -        RelaOffset = Dyn->d_un.d_ptr - (UINTN) ElfCt->PreferredImageAddress;
> 
> -        RelaType   = (Dyn->d_tag == DT_RELA) ? SHT_RELA: SHT_REL;
> 
> +        RelaAddress = Dyn->d_un.d_ptr;
> 
> +        RelaType    = (Dyn->d_tag == DT_RELA) ? SHT_RELA: SHT_REL;
> 
>          break;
> 
>        case DT_RELACOUNT:
> 
>        case DT_RELCOUNT:
> 
> @@ -294,7 +294,7 @@ RelocateElf64Dynamic (
>      }
> 
>    }
> 
> 
> 
> -  if (RelaOffset == MAX_UINT64) {
> 
> +  if (RelaAddress == MAX_UINT64) {
> 
>      ASSERT (RelaCount     == 0);
> 
>      ASSERT (RelaEntrySize == 0);
> 
>      ASSERT (RelaSize      == 0);
> 
> @@ -307,8 +307,16 @@ RelocateElf64Dynamic (
>    //
> 
>    // Verify the existence of the relocation section.
> 
>    //
> 
> -  RelShdr = GetElf64SectionByRange (ElfCt->FileBase, RelaOffset, RelaSize);
> 
> -  ASSERT (RelShdr != NULL);
> 
> +  RelShdr = NULL;
> 
> +  for (Index = 0; Index < ElfCt->ShNum; Index++) {
> 
> +    RelShdr = GetElf64SectionByIndex (ElfCt->FileBase, Index);
> 
> +    ASSERT (RelShdr != NULL);
> 
> +    if ((RelShdr->sh_addr == RelaAddress) && (RelShdr->sh_size == RelaSize))
> {
> 
> +      break;
> 
> +    }
> 
> +    RelShdr = NULL;
> 
> +  }
> 
> +
> 
>    if (RelShdr == NULL) {
> 
>      return EFI_UNSUPPORTED;
> 
>    }
> 
> --
> 2.31.1.windows.1