* [PATCH 0/2] Fix bugs in ELF loader logic @ 2021-06-29 6:27 Ni, Ray 2021-06-29 6:27 ` [PATCH 1/2] UefiPayloadPkg/PayloadLoader: Fix bug in locating relocation section Ni, Ray 2021-06-29 6:27 ` [PATCH 2/2] UefiPayloadPkg/PayloadLoader: Remove assertion Ni, Ray 0 siblings, 2 replies; 5+ messages in thread From: Ni, Ray @ 2021-06-29 6:27 UTC (permalink / raw) To: devel Ray Ni (2): UefiPayloadPkg/PayloadLoader: Fix bug in locating relocation section UefiPayloadPkg/PayloadLoader: Remove assertion .../PayloadLoaderPeim/ElfLib/Elf32Lib.c | 23 ++++++++++++------- .../PayloadLoaderPeim/ElfLib/Elf64Lib.c | 23 ++++++++++++------- 2 files changed, 30 insertions(+), 16 deletions(-) -- 2.31.1.windows.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] UefiPayloadPkg/PayloadLoader: Fix bug in locating relocation section 2021-06-29 6:27 [PATCH 0/2] Fix bugs in ELF loader logic Ni, Ray @ 2021-06-29 6:27 ` Ni, Ray 2021-06-30 4:29 ` Guo Dong 2021-06-29 6:27 ` [PATCH 2/2] UefiPayloadPkg/PayloadLoader: Remove assertion Ni, Ray 1 sibling, 1 reply; 5+ messages in thread From: Ni, Ray @ 2021-06-29 6:27 UTC (permalink / raw) To: devel; +Cc: Maurice Ma, Guo Dong, Benjamin You 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] UefiPayloadPkg/PayloadLoader: Fix bug in locating relocation section 2021-06-29 6:27 ` [PATCH 1/2] UefiPayloadPkg/PayloadLoader: Fix bug in locating relocation section Ni, Ray @ 2021-06-30 4:29 ` Guo Dong 0 siblings, 0 replies; 5+ messages in thread From: Guo Dong @ 2021-06-30 4:29 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io; +Cc: Ma, Maurice, You, Benjamin 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] UefiPayloadPkg/PayloadLoader: Remove assertion 2021-06-29 6:27 [PATCH 0/2] Fix bugs in ELF loader logic Ni, Ray 2021-06-29 6:27 ` [PATCH 1/2] UefiPayloadPkg/PayloadLoader: Fix bug in locating relocation section Ni, Ray @ 2021-06-29 6:27 ` Ni, Ray 2021-06-30 4:29 ` Guo Dong 1 sibling, 1 reply; 5+ messages in thread From: Ni, Ray @ 2021-06-29 6:27 UTC (permalink / raw) To: devel; +Cc: Maurice Ma, Guo Dong, Benjamin You For R_386_RELATIVE and R_X86_64_RELATIVE, today's logic assumes that the content pointed by the Rela->r_offset is 0 but it's not always TRUE. We observed that linker may set the content to Rela->r_addend. The patch removes the assertion. There is no functionality impact for this patch. 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> --- UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf32Lib.c | 1 - UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf64Lib.c | 1 - 2 files changed, 2 deletions(-) diff --git a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf32Lib.c b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf32Lib.c index dd27d3ce59..780f2d9507 100644 --- a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf32Lib.c +++ b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf32Lib.c @@ -164,7 +164,6 @@ ProcessRelocation32 ( // Calculation: B + A // if (RelaType == SHT_RELA) { - ASSERT (*Ptr == 0); *Ptr = (UINT32) Delta + Rela->r_addend; } else { // diff --git a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf64Lib.c b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf64Lib.c index 3f4f12903c..0f1b06e8cc 100644 --- a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf64Lib.c +++ b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf64Lib.c @@ -173,7 +173,6 @@ ProcessRelocation64 ( // Calculation: B + A // if (RelaType == SHT_RELA) { - ASSERT (*Ptr == 0); *Ptr = Delta + Rela->r_addend; } else { // -- 2.31.1.windows.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] UefiPayloadPkg/PayloadLoader: Remove assertion 2021-06-29 6:27 ` [PATCH 2/2] UefiPayloadPkg/PayloadLoader: Remove assertion Ni, Ray @ 2021-06-30 4:29 ` Guo Dong 0 siblings, 0 replies; 5+ messages in thread From: Guo Dong @ 2021-06-30 4:29 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io; +Cc: Ma, Maurice, You, Benjamin 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 2/2] UefiPayloadPkg/PayloadLoader: Remove assertion > > For R_386_RELATIVE and R_X86_64_RELATIVE, today's logic assumes that > the content pointed by the Rela->r_offset is 0 but it's not always > TRUE. We observed that linker may set the content to Rela->r_addend. > > The patch removes the assertion. > There is no functionality impact for this patch. > > 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> > --- > UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf32Lib.c | 1 - > UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf64Lib.c | 1 - > 2 files changed, 2 deletions(-) > > diff --git a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf32Lib.c > b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf32Lib.c > index dd27d3ce59..780f2d9507 100644 > --- a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf32Lib.c > +++ b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf32Lib.c > @@ -164,7 +164,6 @@ ProcessRelocation32 ( > // Calculation: B + A > > // > > if (RelaType == SHT_RELA) { > > - ASSERT (*Ptr == 0); > > *Ptr = (UINT32) Delta + Rela->r_addend; > > } else { > > // > > diff --git a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf64Lib.c > b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf64Lib.c > index 3f4f12903c..0f1b06e8cc 100644 > --- a/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf64Lib.c > +++ b/UefiPayloadPkg/PayloadLoaderPeim/ElfLib/Elf64Lib.c > @@ -173,7 +173,6 @@ ProcessRelocation64 ( > // Calculation: B + A > > // > > if (RelaType == SHT_RELA) { > > - ASSERT (*Ptr == 0); > > *Ptr = Delta + Rela->r_addend; > > } else { > > // > > -- > 2.31.1.windows.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-30 4:30 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-06-29 6:27 [PATCH 0/2] Fix bugs in ELF loader logic Ni, Ray 2021-06-29 6:27 ` [PATCH 1/2] UefiPayloadPkg/PayloadLoader: Fix bug in locating relocation section Ni, Ray 2021-06-30 4:29 ` Guo Dong 2021-06-29 6:27 ` [PATCH 2/2] UefiPayloadPkg/PayloadLoader: Remove assertion Ni, Ray 2021-06-30 4:29 ` Guo Dong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox