[PATCH] D40359: [ELF] Give error message when relro sections are not contiguous.

[Rafael Avila de Espindola via llvm-commits]

LGTM

Peter Smith via Phabricator <reviews at reviews.llvm.org> writes:

> peter.smith created this revision.
> Herald added subscribers: arichardson, emaste.
>
> This is patch 1 of 3 spun out from https://reviews.llvm.org/D40029 covering : Give an error if relro is not contiguous.
>
> If a linker script is used that names linker generated synthetic sections it is possible that the OutputSections for which isRelroSection() is true are not contiguous. When the relro sections are not contiguous we cannot describe them with a single PT_GNU_RELRO PHDR. Unfortunately at least one contemporary dynamic loader only supports one PT_GNU_RELRO PHDR so we cannot output more than one of these PHDRs. As not including relro sections in the PHDR will lead to security sensitive sections being
> writeable we choose to give an error message instead.
>
> Patches 2 and 3 will cover:
>
> - Relax handling of empty sections.
> - Name change when linker scripts are used.
>
>
> https://reviews.llvm.org/D40359
>
> Files:
>   ELF/Writer.cpp
>   test/ELF/relro-non-contiguous.s
>
>
> Index: test/ELF/relro-non-contiguous.s
> ===================================================================
> --- /dev/null
> +++ test/ELF/relro-non-contiguous.s
> @@ -0,0 +1,28 @@
> +// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/shared.s -o %t.o
> +// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/copy-in-shared.s -o %t2.o
> +// RUN: ld.lld -shared %t.o %t2.o -o %t.so
> +
> +// Place the .got.plt (non relro) immediately after .dynamic. This is the
> +// reverse order of the non-linker script case. The linker created .bss.rel.ro
> +// section will be placed after .got.plt causing the relro to be non-contiguous.
> +// RUN: echo "SECTIONS { \
> +// RUN: .dynamic : { *(.dynamic) } \
> +// RUN: .got.plt : { *(.got.plt) } \
> +// RUN: } " > %t.script
> +// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t3.o
> +
> +// Expect error for non-contiguous relro
> +// RUN: not ld.lld %t3.o %t.so -z relro -o %t --script=%t.script 2>&1 | FileCheck %s
> +// No error when we do not request relro.
> +// RUN: ld.lld %t3.o %t.so -z norelro -o %t --script=%t.script
> +// REQUIRES: x86
> +
> +// CHECK: error: section: .bss.rel.ro is not contiguous with other relro sections
> +        .section .text, "ax", @progbits
> +        .global _start
> +        .global bar
> +        .global foo
> +_start:
> +        .quad bar
> +        .quad foo
> +
> Index: ELF/Writer.cpp
> ===================================================================
> --- ELF/Writer.cpp
> +++ ELF/Writer.cpp
> @@ -1511,10 +1511,26 @@
>  
>    // PT_GNU_RELRO includes all sections that should be marked as
>    // read-only by dynamic linker after proccessing relocations.
> +  // Current dynamic loaders only support one PT_GNU_RELRO PHDR, give
> +  // an error message if more than one PT_GNU_RELRO PHDR is required.
>    PhdrEntry *RelRo = make<PhdrEntry>(PT_GNU_RELRO, PF_R);
> -  for (OutputSection *Sec : OutputSections)
> -    if (needsPtLoad(Sec) && isRelroSection(Sec))
> -      RelRo->add(Sec);
> +  bool InRelroPhdr = false;
> +  bool IsRelroFinished = false;
> +  for (OutputSection *Sec : OutputSections) {
> +    if (!needsPtLoad(Sec))
> +      continue;
> +    if (isRelroSection(Sec)) {
> +      InRelroPhdr = true;
> +      if (!IsRelroFinished)
> +        RelRo->add(Sec);
> +      else
> +        error("section: " + Sec->Name + " is not contiguous with other relro" +
> +              " sections");
> +    } else if (InRelroPhdr) {
> +      InRelroPhdr = false;
> +      IsRelroFinished = true;
> +    }
> +  }
>    if (RelRo->FirstSec)
>      Ret.push_back(RelRo);
>  
>
>
> Index: test/ELF/relro-non-contiguous.s
> ===================================================================
> --- /dev/null
> +++ test/ELF/relro-non-contiguous.s
> @@ -0,0 +1,28 @@
> +// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/shared.s -o %t.o
> +// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/copy-in-shared.s -o %t2.o
> +// RUN: ld.lld -shared %t.o %t2.o -o %t.so
> +
> +// Place the .got.plt (non relro) immediately after .dynamic. This is the
> +// reverse order of the non-linker script case. The linker created .bss.rel.ro
> +// section will be placed after .got.plt causing the relro to be non-contiguous.
> +// RUN: echo "SECTIONS { \
> +// RUN: .dynamic : { *(.dynamic) } \
> +// RUN: .got.plt : { *(.got.plt) } \
> +// RUN: } " > %t.script
> +// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t3.o
> +
> +// Expect error for non-contiguous relro
> +// RUN: not ld.lld %t3.o %t.so -z relro -o %t --script=%t.script 2>&1 | FileCheck %s
> +// No error when we do not request relro.
> +// RUN: ld.lld %t3.o %t.so -z norelro -o %t --script=%t.script
> +// REQUIRES: x86
> +
> +// CHECK: error: section: .bss.rel.ro is not contiguous with other relro sections
> +        .section .text, "ax", @progbits
> +        .global _start
> +        .global bar
> +        .global foo
> +_start:
> +        .quad bar
> +        .quad foo
> +
> Index: ELF/Writer.cpp
> ===================================================================
> --- ELF/Writer.cpp
> +++ ELF/Writer.cpp
> @@ -1511,10 +1511,26 @@
>  
>    // PT_GNU_RELRO includes all sections that should be marked as
>    // read-only by dynamic linker after proccessing relocations.
> +  // Current dynamic loaders only support one PT_GNU_RELRO PHDR, give
> +  // an error message if more than one PT_GNU_RELRO PHDR is required.
>    PhdrEntry *RelRo = make<PhdrEntry>(PT_GNU_RELRO, PF_R);
> -  for (OutputSection *Sec : OutputSections)
> -    if (needsPtLoad(Sec) && isRelroSection(Sec))
> -      RelRo->add(Sec);
> +  bool InRelroPhdr = false;
> +  bool IsRelroFinished = false;
> +  for (OutputSection *Sec : OutputSections) {
> +    if (!needsPtLoad(Sec))
> +      continue;
> +    if (isRelroSection(Sec)) {
> +      InRelroPhdr = true;
> +      if (!IsRelroFinished)
> +        RelRo->add(Sec);
> +      else
> +        error("section: " + Sec->Name + " is not contiguous with other relro" +
> +              " sections");
> +    } else if (InRelroPhdr) {
> +      InRelroPhdr = false;
> +      IsRelroFinished = true;
> +    }
> +  }
>    if (RelRo->FirstSec)
>      Ret.push_back(RelRo);
>