[PATCH] D43819: [ELF] - Restrict section offsets that exceeds file size.

[Rafael Ávila de Espíndola via Phabricator via llvm-commits]

rafael added a comment.

LGTM.

I think the comment can probably be simplified, but that can be a
followup.

Cheers,
Rafael

George Rimar via Phabricator <reviews at reviews.llvm.org> writes:

> grimar updated this revision to Diff 137976.
>  grimar added a comment.
> 
> - Addressed review comment.
> 
> https://reviews.llvm.org/D43819
> 
> Files:
> 
>   ELF/Writer.cpp
>   test/ELF/linkerscript/Inputs/sections-va-overflow.s
>   test/ELF/linkerscript/sections-va-overflow.test
> 
> Index: test/ELF/linkerscript/sections-va-overflow.test
>  ===================================================================
> 
>   - test/ELF/linkerscript/sections-va-overflow.test +++ test/ELF/linkerscript/sections-va-overflow.test @@ -0,0 +1,22 @@ +# REQUIRES: x86 +# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/sections-va-overflow.s -o %t.o +# RUN: not ld.lld -o /dev/null --script %s %t.o 2>&1 | FileCheck %s -check-prefix=ERR + +PHDRS { +  ph_headers PT_PHDR PHDRS; +  ph_text PT_LOAD FILEHDR PHDRS FLAGS (0x1 | 0x4); +} + +SECTIONS { + . = 0xffffffff20000000; + .text : { *(.text*) } : ph_text + .test 0x1000 : { BYTE(0) } + .bss : { *(.bss*) } +} + +## Section .test has VA 0x1000 and placed in the same segment as section .text +## with VA 0xffffffff20000000. That might be technically correct, but most probably +## is a result of a broken script file and causes file offset calculation overflow. +## It seems we do not have to support it, so we don't and we report an error in this case. +# ERR: error: unable to place section .text at file offset [0xFFFFFFFF20000000, 0xFFFFFFFE40000000]; check your linker script for overflows +# ERR-NOT: unable to place section .bss Index: test/ELF/linkerscript/Inputs/sections-va-overflow.s ===================================================================
>   - test/ELF/linkerscript/Inputs/sections-va-overflow.s +++ test/ELF/linkerscript/Inputs/sections-va-overflow.s @@ -0,0 +1,6 @@ +.global _start +_start: +  retq + +.bss +.space 0x2000 Index: ELF/Writer.cpp ===================================================================
>   - ELF/Writer.cpp +++ ELF/Writer.cpp @@ -1873,6 +1873,12 @@ FileSize = alignTo(Off, Config->Wordsize); }
> 
>     +static std::string rangeToString(uint64_t Addr, uint64_t Len) { +  if (Len == 0) +    return "<empty range at 0x" + utohexstr(Addr) + ">"; +  return "[0x" + utohexstr(Addr) + ", 0x" + utohexstr(Addr + Len - 1) + "]"; +} + // Assign file offsets to output sections. template <class ELFT> void Writer<ELFT>::assignFileOffsets() { uint64_t Off = 0; @@ -1897,6 +1903,25 @@
> 
>     SectionHeaderOff = alignTo(Off, Config->Wordsize); FileSize = SectionHeaderOff + (OutputSections.size() + 1) * sizeof(Elf_Shdr); + +  // Our logic assumes that sections have rising VA within the same segment. +  // With use of linker scripts it is possible to violate this rule and get file +  // offset overlaps or overflows. That should never happen with a valid script +  // which does not move the location counter backwards and usually scripts do +  // not do that. Unfortunately, there are apps in the wild, for example, Linux +  // kernel, which control segment distribution explicitly and move the counter +  // backwards, so we have to allow doing that to support linking them. We +  // perform non-critical checks for overlaps in checkNoOverlappingSections(), +  // but here we want to prevent file size overflows because it would crash the +  // linker. +  for (OutputSection *Sec : OutputSections) { +    if (Sec->Type == SHT_NOBITS) +      continue; +    if ((Sec->Offset > FileSize) || (Sec->Offset + Sec->Size > FileSize)) +      error("unable to place section " + Sec->Name + " at file offset " + +            rangeToString(Sec->Offset, Sec->Offset + Sec->Size) + +            "; check your linker script for overflows"); +  } }
> 
>     // Finalize the program headers. We call this function after we assign @@ -1935,12 +1960,6 @@ } }
> 
>     -static std::string rangeToString(uint64_t Addr, uint64_t Len) {
> - if (Len == 0)
> - return "<empty range at 0x" + utohexstr(Addr) + ">";
> - return "[0x" + utohexstr(Addr) + ", 0x" + utohexstr(Addr + Len - 1) + "]"; -} - // Check whether sections overlap for a specific address range (file offsets, // load and virtual adresses). //
> 
> Index: test/ELF/linkerscript/sections-va-overflow.test
>  ===================================================================
> 
>   - test/ELF/linkerscript/sections-va-overflow.test +++ test/ELF/linkerscript/sections-va-overflow.test @@ -0,0 +1,22 @@ +# REQUIRES: x86 +# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/sections-va-overflow.s -o %t.o +# RUN: not ld.lld -o /dev/null --script %s %t.o 2>&1 | FileCheck %s -check-prefix=ERR + +PHDRS { +  ph_headers PT_PHDR PHDRS; +  ph_text PT_LOAD FILEHDR PHDRS FLAGS (0x1 | 0x4); +} + +SECTIONS { + . = 0xffffffff20000000; + .text : { *(.text*) } : ph_text + .test 0x1000 : { BYTE(0) } + .bss : { *(.bss*) } +} + +## Section .test has VA 0x1000 and placed in the same segment as section .text +## with VA 0xffffffff20000000. That might be technically correct, but most probably +## is a result of a broken script file and causes file offset calculation overflow. +## It seems we do not have to support it, so we don't and we report an error in this case. +# ERR: error: unable to place section .text at file offset [0xFFFFFFFF20000000, 0xFFFFFFFE40000000]; check your linker script for overflows +# ERR-NOT: unable to place section .bss Index: test/ELF/linkerscript/Inputs/sections-va-overflow.s ===================================================================
>   - test/ELF/linkerscript/Inputs/sections-va-overflow.s +++ test/ELF/linkerscript/Inputs/sections-va-overflow.s @@ -0,0 +1,6 @@ +.global _start +_start: +  retq + +.bss +.space 0x2000 Index: ELF/Writer.cpp ===================================================================
>   - ELF/Writer.cpp +++ ELF/Writer.cpp @@ -1873,6 +1873,12 @@ FileSize = alignTo(Off, Config->Wordsize); }
> 
>     +static std::string rangeToString(uint64_t Addr, uint64_t Len) { +  if (Len == 0) +    return "<empty range at 0x" + utohexstr(Addr) + ">"; +  return "[0x" + utohexstr(Addr) + ", 0x" + utohexstr(Addr + Len - 1) + "]"; +} + // Assign file offsets to output sections. template <class ELFT> void Writer<ELFT>::assignFileOffsets() { uint64_t Off = 0; @@ -1897,6 +1903,25 @@
> 
>     SectionHeaderOff = alignTo(Off, Config->Wordsize); FileSize = SectionHeaderOff + (OutputSections.size() + 1) * sizeof(Elf_Shdr); + +  // Our logic assumes that sections have rising VA within the same segment. +  // With use of linker scripts it is possible to violate this rule and get file +  // offset overlaps or overflows. That should never happen with a valid script +  // which does not move the location counter backwards and usually scripts do +  // not do that. Unfortunately, there are apps in the wild, for example, Linux +  // kernel, which control segment distribution explicitly and move the counter +  // backwards, so we have to allow doing that to support linking them. We +  // perform non-critical checks for overlaps in checkNoOverlappingSections(), +  // but here we want to prevent file size overflows because it would crash the +  // linker. +  for (OutputSection *Sec : OutputSections) { +    if (Sec->Type == SHT_NOBITS) +      continue; +    if ((Sec->Offset > FileSize) || (Sec->Offset + Sec->Size > FileSize)) +      error("unable to place section " + Sec->Name + " at file offset " + +            rangeToString(Sec->Offset, Sec->Offset + Sec->Size) + +            "; check your linker script for overflows"); +  } }
> 
>     // Finalize the program headers. We call this function after we assign @@ -1935,12 +1960,6 @@ } }
> 
>     -static std::string rangeToString(uint64_t Addr, uint64_t Len) {
> - if (Len == 0)
> - return "<empty range at 0x" + utohexstr(Addr) + ">";
> - return "[0x" + utohexstr(Addr) + ", 0x" + utohexstr(Addr + Len - 1) + "]"; -} - // Check whether sections overlap for a specific address range (file offsets, // load and virtual adresses). //


https://reviews.llvm.org/D43819