[lld][ELF]Add option to make .dynamic read only

[jakehehrlich]

Summary

The .dynamic section of an ELF almost doesn't need to be written to with the exception of the DT_DEBUG entry. For several reasons having a read only .dynamic section would be useful. This change adds the -z keyword "rodynamic" which forces .dynamic to be read-only. In this case DT_DEBUG will not be emited.

[ruiu]

I have a couple of questions.

• Why can't .dynamic be read-only for all output?
• .dynamic is already covered by PT_GNU_RELRO segments. Does it make sense to make it read-only from beginning?

It seems that you are proposing a new command line option that even GNU linkers don't have, so this patch needs extra attention. Are you adding that option to GNU linkers too?

[ruiu]

On second thought, it isn't good to discuss new linker command line option in a code review thread, as most people do not read every review threads. Please start a thread in llvm-dev to propose a new option with an explanation why you want it.

[emaste]

Does it make sense to make it read-only from beginning?

For me I think having .dynamic relro is sufficient, and might actually like to have MIPS use the conventional DT_DEBUG scheme.

[pcc]

In D33251#756693, @emaste wrote:

Does it make sense to make it read-only from beginning?

For me I think having .dynamic relro is sufficient, and might actually like to have MIPS use the conventional DT_DEBUG scheme.

Agreed. It seems that this scheme exists only for historical reasons: looking through binutils history, DT_MIPS_RLD_MAP was introduced in 1994 [0], while PT_GNU_RELRO was introduced 10 years later [1].

[0] https://github.com/bminor/binutils-gdb/commit/4c040a649b610ab1a0b5aa30c01148bffa9e2f4b
[1] https://github.com/bminor/binutils-gdb/commit/8c37241be3b1baf394090269b4b67babceb83d61

[jakehehrlich]

After some discussion on llvm-dev it was decided that it would be best to not mess with DT_DEBUG_INDIRECT.

[ruiu]

I'm not convinced that we need this, so please don't submit.

If this is for your OS's vDSO which needs to be read by an in-kernel ELF loader which doesn't support the entire ELF spec, this patch is to work around your loader's limitation. If you don't want to implement a complete ELF loader, why do you want to "fix" the problem in the right way? It is odd that you do hack at one place and do the right thing at the other place. Your loader can ignore RW bit and map .dynamic sections to read-only segments.

[ruiu]

ELF/SyntheticSections.cpp

1028–1029

... on MIPS and on Fuchsia OS which passes -z rodynamic.

1032

Probably it is better to not add this comment as it is not informative but could be confusing (e.g. why users may want it on other systems?)

1077–1078

Add a space after // and a full stop at end of the line.

But you want to extend the comment to explain the background. I'd write something like:

DT_DEBUG points to a location containing some debug info so that debuggers can find debug info at runtime. We need it for each process, so we don't write it for DSOs.

DT_DEBUG is the only .dynamic entry to which the loader is expected to write. Some systems, such as Fuchsia, chose to make .dynamic sections completely read-only (and provides alternative mean to find debug info). If the target is such system, we don't write DT_DEBUG.

[jakehehrlich]

I made the changes Rui recommended.

[ruiu]

ELF/Config.h

147

These variables are named after their corresponding command line options, so ZRodynamic.

ELF/SyntheticSections.cpp

1080

You want to break the paragraph by adding a blank // line.

1081–1082

Let's avoid writing comments too abstractly. "Some systems (currently only Fuchsia)" and instead of "Such systemS", "Fuchsia".

[jakehehrlich]

Fixed a few more things.

[ruiu]

ELF/Config.h

147–148

Sort.

ELF/Driver.cpp

690–691

Sort

ELF/SyntheticSections.cpp

1084

What I wanted is this

// blah blah blah ...
// blah blah blah ...
// 
// blah blah blah ...
// blah blah blah ...

and not

// blah blah blah ...
// blah blah blah ...
// blah blah blah ...
// blah blah blah ...
//

(Please look around to find a local convention and follow it.)

[jakehehrlich]

Sorry about these last fixes. I shouldn't have made those mistakes. Should be fixed.

[ruiu]

test/ELF/rodynamic.s

3

This doesn't test the feature you've just added, since -shared is passed. You want to remove that flag.

9–14

This checks the presence of .dynamic section, which is not what you actually want to do. You want to use llvm-readobj like this.

RUN: ld.lld -o %t1 %t.o
RUN: llvm-readobj -dynamic-table %t1 | FileCheck -check-prefix=DEFAULT %s
DEFAULT: DEBUG

RUN: ld.lld -o %t2 %t.o -z rodynamic
RUN: llvm-readobj -dynamic-table %t2 | FileCheck %s
CHECK-NOT: DEBUG

[jakehehrlich]

Right! I forgot to check that DEBUG wasn't being emitted in the test. I also forgot to check the default behavior. The other part of the test ensures that the .dynamic section is in fact read-only and that in the default case it is read-write. I had to produce a shared library to make the executable have a .dynamic section as well so I added a simple file to Inputs.

[ruiu]

LGTM with these changes.

test/ELF/rodynamic.s

4

Break right here

8

and here to make it more comfortable to read. (It is a good practice to separate code blocks with blank lines so that you don't need to read an entire block of code without breathing.)

[jakehehrlich]

Made the final fix.

ELF/Config.h

struct Configuration {
   bool WarnMissingEntry;
   bool ZCombreloc;
   bool ZExecstack;
   bool ZNocopyreloc;
   bool ZNodelete;
   bool ZNodlopen;
   bool ZNow;
   bool ZOrigin;
+  bool ZReadOnlyDynamic;
   bool ZRelro;
   bool ZText;
   bool ExitEarly;
   bool ZWxneeded;
   DiscardPolicy Discard;
   SortSectionPolicy SortSection;
   StripPolicy Strip;
   UnresolvedPolicy UnresolvedSymbols;
   Target2Policy Target2;

ELF/Driver.cpp

void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   Config->WarnCommon = Args.hasArg(OPT_warn_common);
   Config->ZCombreloc = !hasZOption(Args, "nocombreloc");
   Config->ZExecstack = hasZOption(Args, "execstack");
   Config->ZNocopyreloc = hasZOption(Args, "nocopyreloc");
   Config->ZNodelete = hasZOption(Args, "nodelete");
   Config->ZNodlopen = hasZOption(Args, "nodlopen");
   Config->ZNow = hasZOption(Args, "now");
   Config->ZOrigin = hasZOption(Args, "origin");
+  Config->ZReadOnlyDynamic = hasZOption(Args, "rodynamic");
   Config->ZRelro = !hasZOption(Args, "norelro");
   Config->ZStackSize = getZOptionValue(Args, "stack-size", 0);
   Config->ZText = !hasZOption(Args, "notext");
   Config->ZWxneeded = hasZOption(Args, "wxneeded");
 
   if (Config->LTOO > 3)
     error("invalid optimization level for LTO: " + getString(Args, OPT_lto_O));
   if (Config->LTOPartitions == 0)
     error("--lto-partitions: number of threads must be > 0");
static void setConfigs() {
   Config->Is64 = (Kind == ELF64LEKind || Kind == ELF64BEKind);
   Config->IsLE = (Kind == ELF32LEKind || Kind == ELF64LEKind);
   Config->Endianness =
       Config->IsLE ? support::endianness::little : support::endianness::big;
   Config->IsMips64EL = (Kind == ELF64LEKind && Machine == EM_MIPS);
   Config->IsRela = Config->Is64 || IsX32 || Config->MipsN32Abi;
   Config->Pic = Config->Pie || Config->Shared;
   Config->Wordsize = Config->Is64 ? 8 : 4;
+  //MIPS must unconditionally set ZReadOnlyDynamic
+  Config->ZReadOnlyDynamic = Machine == EM_MIPS ? true : Config->ZReadOnlyDynamic;
 }
 
 // Returns a value of "-format" option.
 static bool getBinaryOption(StringRef S) {
   if (S == "binary")
     return true;
   if (S == "elf" || S == "default")
     return false;

ELF/SyntheticSections.h

 private:
   SymbolBody *Sym;
   const InputSectionBase *InputSec = nullptr;
   uint64_t OffsetInSec;
   bool UseSymVA;
   int64_t Addend;
 };
 
+// This is a section to hold a space within the data segment
+// of the executable file which is pointed to by the
+// DT_MIPS_RLD_MAP/DT_DEBUG_INDIRECT entry. This was a MIPS specific
+// section but it has other uses as well.
+// See "Dynamic section" in Chapter 5 in the following document:
+// ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/mipsabi.pdf
+class RldMapSection : public SyntheticSection {
+public:
+  RldMapSection();
+  size_t getSize() const override { return Config->Wordsize; }
+  void writeTo(uint8_t *Buf) override;
+};
+
 template <class ELFT> class DynamicSection final : public SyntheticSection {
   typedef typename ELFT::Dyn Elf_Dyn;
   typedef typename ELFT::Rel Elf_Rel;
   typedef typename ELFT::Rela Elf_Rela;
   typedef typename ELFT::Shdr Elf_Shdr;
   typedef typename ELFT::Sym Elf_Sym;
 
   // The .dynamic section contains information for the dynamic linker.
public:
   MipsReginfoSection(Elf_Mips_RegInfo Reginfo);
   size_t getSize() const override { return sizeof(Elf_Mips_RegInfo); }
   void writeTo(uint8_t *Buf) override;
 
 private:
   Elf_Mips_RegInfo Reginfo;
 };
 
-// This is a MIPS specific section to hold a space within the data segment
-// of executable file which is pointed to by the DT_MIPS_RLD_MAP entry.
-// See "Dynamic section" in Chapter 5 in the following document:
-// ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/mipsabi.pdf
-class MipsRldMapSection : public SyntheticSection {
-public:
-  MipsRldMapSection();
-  size_t getSize() const override { return Config->Wordsize; }
-  void writeTo(uint8_t *Buf) override;
-};
-
 class ARMExidxSentinelSection : public SyntheticSection {
 public:
   ARMExidxSentinelSection();
   size_t getSize() const override { return 8; }
   void writeTo(uint8_t *Buf) override;
 };
 
 // A container for one or more linker generated thunks. Instances of these
struct InX {
   static SyntheticSection *Dynamic;
   static StringTableSection *DynStrTab;
   static InputSection *Interp;
   static GdbIndexSection *GdbIndex;
   static GotBaseSection *Got;
   static GotPltSection *GotPlt;
   static IgotPltSection *IgotPlt;
   static MipsGotSection *MipsGot;
-  static MipsRldMapSection *MipsRldMap;
+  static RldMapSection *RldMap;
   static PltSection *Plt;
   static PltSection *Iplt;
   static StringTableSection *ShStrTab;
   static StringTableSection *StrTab;
 };
 
 template <class ELFT> struct In : public InX {
   static SymbolTableSection<ELFT> *DynSymTab;

ELF/SyntheticSections.cpp

 
 void StringTableSection::writeTo(uint8_t *Buf) {
   for (StringRef S : Strings) {
     memcpy(Buf, S.data(), S.size());
     Buf += S.size() + 1;
   }
 }
 
+RldMapSection::RldMapSection()
+    : SyntheticSection(SHF_ALLOC | SHF_WRITE, SHT_PROGBITS, Config->Wordsize,
+                       ".rld_map") {}
+
+void RldMapSection::writeTo(uint8_t *Buf) {
+  // Apply filler from linker script.
+  Optional<uint32_t> Fill = Script->getFiller(this->OutSec);
+  if (!Fill || *Fill == 0)
+    return;
+
+  uint64_t Filler = *Fill;
+  Filler = (Filler << 32) | Filler;
+  memcpy(Buf, &Filler, getSize());
+}
+
 // Returns the number of version definition entries. Because the first entry
 // is for the version definition itself, it is the number of versioned symbols
 // plus one. Note that we don't support multiple versions yet.
 static unsigned getVerDefNum() { return Config->VersionDefinitions.size() + 1; }
 
 template <class ELFT>
 DynamicSection<ELFT>::DynamicSection()
     : SyntheticSection(SHF_ALLOC | SHF_WRITE, SHT_DYNAMIC, Config->Wordsize,
                        ".dynamic") {
   this->Entsize = ELFT::Is64Bits ? 16 : 8;
 
-  // .dynamic section is not writable on MIPS.
+  // For security the user may want to make .dynamic read only.
+  // Additionally .dynamic section is not writable on MIPS.
   // See "Special Section" in Chapter 4 in the following document:
   // ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/mipsabi.pdf
-  if (Config->EMachine == EM_MIPS)
+  if (Config->ZReadOnlyDynamic)
     this->Flags = SHF_ALLOC;
 
   addEntries();
 }
 
 // There are some dynamic entries that don't depend on other sections.
 // Such entries can be set early.
 template <class ELFT> void DynamicSection<ELFT>::addEntries() {
if (Config->ZOrigin) {
     DtFlags1 |= DF_1_ORIGIN;
   }
 
   if (DtFlags)
     add({DT_FLAGS, DtFlags});
   if (DtFlags1)
     add({DT_FLAGS_1, DtFlags1});
 
-  if (!Config->Shared && !Config->Relocatable)
+  //If .dynamic is read only then DT_DEBUG will cause a segfault.
+  if (!Config->Shared && !Config->Relocatable && !Config->ZReadOnlyDynamic)
     add({DT_DEBUG, (uint64_t)0});
 }
 
 // Add remaining entries to complete .dynamic contents.
 template <class ELFT> void DynamicSection<ELFT>::finalizeContents() {
   if (this->Size)
     return; // Already finalized.
 
   this->Link = InX::DynStrTab->OutSec->SectionIndex;
   if (In<ELFT>::RelaDyn->OutSec->Size > 0) {
     bool IsRela = Config->IsRela;
     add({IsRela ? DT_RELA : DT_REL, In<ELFT>::RelaDyn});
     add({IsRela ? DT_RELASZ : DT_RELSZ, In<ELFT>::RelaDyn->OutSec->Size});
     add({IsRela ? DT_RELAENT : DT_RELENT,
if (Config->EMachine == EM_MIPS) {
     add({DT_MIPS_BASE_ADDRESS, Config->ImageBase});
     add({DT_MIPS_SYMTABNO, In<ELFT>::DynSymTab->getNumSymbols()});
     add({DT_MIPS_LOCAL_GOTNO, InX::MipsGot->getLocalEntriesNum()});
     if (const SymbolBody *B = InX::MipsGot->getFirstGlobalEntry())
       add({DT_MIPS_GOTSYM, B->DynsymIndex});
     else
       add({DT_MIPS_GOTSYM, In<ELFT>::DynSymTab->getNumSymbols()});
     add({DT_PLTGOT, InX::MipsGot});
-    if (InX::MipsRldMap)
-      add({DT_MIPS_RLD_MAP, InX::MipsRldMap});
   }
+  if (InX::RldMap)
+    add({DT_DEBUG_INDIRECT, InX::RldMap});
 
   this->OutSec->Link = this->Link;
 
   // +1 for DT_NULL
   this->Size = (Entries.size() + 1) * this->Entsize;
 }
 
 template <class ELFT> void DynamicSection<ELFT>::writeTo(uint8_t *Buf) {

 }
 
 size_t MergeSyntheticSection::getSize() const {
   // We should finalize string builder to know the size.
   const_cast<MergeSyntheticSection *>(this)->finalizeContents();
   return Builder.getSize();
 }
 
-MipsRldMapSection::MipsRldMapSection()
-    : SyntheticSection(SHF_ALLOC | SHF_WRITE, SHT_PROGBITS, Config->Wordsize,
-                       ".rld_map") {}
-
-void MipsRldMapSection::writeTo(uint8_t *Buf) {
-  // Apply filler from linker script.
-  Optional<uint32_t> Fill = Script->getFiller(this->OutSec);
-  if (!Fill || *Fill == 0)
-    return;
-
-  uint64_t Filler = *Fill;
-  Filler = (Filler << 32) | Filler;
-  memcpy(Buf, &Filler, getSize());
-}
-
 ARMExidxSentinelSection::ARMExidxSentinelSection()
     : SyntheticSection(SHF_ALLOC | SHF_LINK_ORDER, SHT_ARM_EXIDX,
                        Config->Wordsize, ".ARM.exidx") {}
 
 // Write a terminating sentinel entry to the end of the .ARM.exidx table.
 // This section will have been sorted last in the .ARM.exidx table.
 // This table entry will have the form:
 // | PREL31 upper bound of code that has exception tables | EXIDX_CANTUNWIND |

 SyntheticSection *InX::Dynamic;
 StringTableSection *InX::DynStrTab;
 InputSection *InX::Interp;
 GdbIndexSection *InX::GdbIndex;
 GotBaseSection *InX::Got;
 GotPltSection *InX::GotPlt;
 IgotPltSection *InX::IgotPlt;
 MipsGotSection *InX::MipsGot;
-MipsRldMapSection *InX::MipsRldMap;
+RldMapSection *InX::RldMap;
 PltSection *InX::Plt;
 PltSection *InX::Iplt;
 StringTableSection *InX::ShStrTab;
 StringTableSection *InX::StrTab;
 
 template void PltSection::addEntry<ELF32LE>(SymbolBody &Sym);
 template void PltSection::addEntry<ELF32BE>(SymbolBody &Sym);
 template void PltSection::addEntry<ELF64LE>(SymbolBody &Sym);

ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::createSyntheticSections() {
   Add(InX::Bss);
   InX::BssRelRo = make<BssSection>(".bss.rel.ro");
   Add(InX::BssRelRo);
 
   // Add MIPS-specific sections.
   bool HasDynSymTab = !Symtab<ELFT>::X->getSharedFiles().empty() ||
                       Config->Pic || Config->ExportDynamic;
   if (Config->EMachine == EM_MIPS) {
-    if (!Config->Shared && HasDynSymTab) {
-      InX::MipsRldMap = make<MipsRldMapSection>();
-      Add(InX::MipsRldMap);
-    }
     if (auto *Sec = MipsAbiFlagsSection<ELFT>::create())
       Add(Sec);
     if (auto *Sec = MipsOptionsSection<ELFT>::create())
       Add(Sec);
     if (auto *Sec = MipsReginfoSection<ELFT>::create())
       Add(Sec);
   }
 
+  //If ZReadOnlyDynamic is set then we're going to need to emit a
+  //RldMapSection for DT_DEBUG_INDIRECT/DT_MIPS_RLD_MAP to point to.
+  if(!Config->Shared && HasDynSymTab && Config->ZReadOnlyDynamic) {
+    In<ELFT>::RldMap = make<RldMapSection>();
+    Add(In<ELFT>::RldMap);
+  }
+
   if (HasDynSymTab) {
     In<ELFT>::DynSymTab = make<SymbolTableSection<ELFT>>(*InX::DynStrTab);
     Add(In<ELFT>::DynSymTab);
 
     In<ELFT>::VerSym = make<VersionTableSection<ELFT>>();
     Add(In<ELFT>::VerSym);
 
     if (!Config->VersionDefinitions.empty()) {

test/ELF/Inputs/rodynamic.s

+  .text
+  .globl _foo
+_foo:
+  ret

test/ELF/rodynamic.s

+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/rodynamic.s -o %td.o
+# RUN: ld.lld -shared %td.o -o %td.so
+# RUN: ld.lld -z rodynamic %t.o %td.so -o %t.exe
+# RUN: llvm-readobj -sections -dynamic-table %t.exe | FileCheck %s
+
+  .text
+  .globl __start,_foo
+  .type _foo,@function
+__start:
+  ret
+
+# CHECK:        Section {
+# CHECK:          Name: .dynamic
+# CHECK-NEXT:     Type: SHT_DYNAMIC
+# CHECK-NEXT:     Flags [
+# CHECK-NEXT:       SHF_ALLOC
+# CHECK-NEXT:     ]
+# CHECK:          Name: .rld_map
+# CHECK-NEXT:     Type: SHT_PROGBITS
+# CHECK-NEXT:     Flags [
+# CHECK-NEXT:       SHF_ALLOC
+# CHECK-NEXT:       SHF_WRITE
+# CHECK-NEXT:     ]
+# CHECK-NEXT:     Address: [[RLDMAPADDR:0x[0-9a-f]+]]
+# CHECK:        DynamicSection [
+# CHECK:          0x0000000070000016 unknown [[RLDMAPADDR]]