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

[Rafael Avila de Espindola via llvm-commits]

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).
>  //