ELF: Reserve space for copy relocations of read-only symbols in relro.

[pcc]

Summary

When reserving copy relocation space for a shared symbol, scan the DSO's
program headers to see if the symbol is in a read-only segment. If so,
reserve space for that symbol in a new synthetic section named .bss.rel.ro
which will be covered by the relro program header.

This fixes the security issue disclosed on the binutils mailing list at:
https://sourceware.org/ml/libc-alpha/2016-12/msg00914.html

[ruiu]

Looks like ld.bfd and ld.gold have something similar already. Are they using the same section name, .bss.rel.ro?

lld/ELF/Relocations.cpp

419–426 ↗

(On Diff #82988)

Can you factor the new code as a function taking a SharedSymbol and returning a boolean value?

[pcc]

In D28272#636529, @ruiu wrote:

Looks like ld.bfd and ld.gold have something similar already. Are they using the same section name, .bss.rel.ro?

It looks like they are both using the name .data.rel.ro.

Relatedly, I discovered a bug in this patch. We can't use SHT_NOBITS for the new section because we need to lay out all NOBITS sections contiguously at the end of the LOAD. RELRO also needs to be contiguous (there can only be one RELRO header), so we can't have another RELRO covering part of the bss.

In principle, we could teach the linker to emit two r/w LOAD headers, one for RELRO and the other for non-RELRO, each with its own bss, but it's not clear that that would be worth it. So we might as well use SHT_PROGBITS and rename the section to .data.rel.ro to match the type. I'll upload a new patch that does that.

lld/ELF/Relocations.cpp

419–426 ↗

(On Diff #82988)

Will do

[pcc]

• Change section ordering as follows: non-RELRO non-NOBITS, TLS non-NOBITS, TLS NOBITS, RELRO non-NOBITS, RELRO NOBITS, non-RELRO NOBITS

[ruiu]

LGTM, but you may want to wait for a while to give Rafael to take another look.

lld/ELF/Symbols.h

297–300 ↗

(On Diff #83332)

Do you have to inline this function? I think you added #include "OutputSections.h" for this function, but I don't want to add a new #include to this file to avoid cyclic inclusion.

[pcc]

I committed the section ordering changes separately in r291523.

lld/trunk/ELF/OutputSections.h

 // until Writer is initialized.
 template <class ELFT> struct Out {
   typedef typename ELFT::uint uintX_t;
   typedef typename ELFT::Phdr Elf_Phdr;
 
   static uint8_t First;
   static EhOutputSection<ELFT> *EhFrame;
   static OutputSection<ELFT> *Bss;
+  static OutputSection<ELFT> *BssRelRo;
   static OutputSectionBase *Opd;
   static uint8_t *OpdBuf;
   static PhdrEntry *TlsPhdr;
   static OutputSectionBase *DebugInfo;
   static OutputSectionBase *ElfHeader;
   static OutputSectionBase *ProgramHeaders;
   static OutputSectionBase *PreinitArray;
   static OutputSectionBase *InitArray;
template <class ELFT> uint64_t getHeaderSize() {
   if (Config->OFormatBinary)
     return 0;
   return Out<ELFT>::ElfHeader->Size + Out<ELFT>::ProgramHeaders->Size;
 }
 
 template <class ELFT> uint8_t Out<ELFT>::First;
 template <class ELFT> EhOutputSection<ELFT> *Out<ELFT>::EhFrame;
 template <class ELFT> OutputSection<ELFT> *Out<ELFT>::Bss;
+template <class ELFT> OutputSection<ELFT> *Out<ELFT>::BssRelRo;
 template <class ELFT> OutputSectionBase *Out<ELFT>::Opd;
 template <class ELFT> uint8_t *Out<ELFT>::OpdBuf;
 template <class ELFT> PhdrEntry *Out<ELFT>::TlsPhdr;
 template <class ELFT> OutputSectionBase *Out<ELFT>::DebugInfo;
 template <class ELFT> OutputSectionBase *Out<ELFT>::ElfHeader;
 template <class ELFT> OutputSectionBase *Out<ELFT>::ProgramHeaders;
 template <class ELFT> OutputSectionBase *Out<ELFT>::PreinitArray;
 template <class ELFT> OutputSectionBase *Out<ELFT>::InitArray;
 template <class ELFT> OutputSectionBase *Out<ELFT>::FiniArray;
 } // namespace elf
 } // namespace lld
 
 
 #endif

lld/trunk/ELF/Relocations.cpp

template <class ELFT> static uint32_t getAlignment(SharedSymbol<ELFT> *SS) {
 
   uintX_t SecAlign = SS->file()->getSection(SS->Sym)->sh_addralign;
   uintX_t SymValue = SS->Sym.st_value;
   int TrailingZeros =
       std::min(countTrailingZeros(SecAlign), countTrailingZeros(SymValue));
   return 1 << TrailingZeros;
 }
 
-// Reserve space in .bss for copy relocation.
+template <class ELFT> static bool isReadOnly(SharedSymbol<ELFT> *SS) {
+  typedef typename ELFT::uint uintX_t;
+  typedef typename ELFT::Phdr Elf_Phdr;
+
+  // Determine if the symbol is read-only by scanning the DSO's program headers.
+  uintX_t Value = SS->Sym.st_value;
+  for (const Elf_Phdr &Phdr : check(SS->file()->getObj().program_headers()))
+    if ((Phdr.p_type == ELF::PT_LOAD || Phdr.p_type == ELF::PT_GNU_RELRO) &&
+        !(Phdr.p_flags & ELF::PF_W) && Value >= Phdr.p_vaddr &&
+        Value < Phdr.p_vaddr + Phdr.p_memsz)
+      return true;
+  return false;
+}
+
+// Reserve space in .bss or .bss.rel.ro for copy relocation.
 template <class ELFT> static void addCopyRelSymbol(SharedSymbol<ELFT> *SS) {
   typedef typename ELFT::uint uintX_t;
   typedef typename ELFT::Sym Elf_Sym;
 
   // Copy relocation against zero-sized symbol doesn't make sense.
   uintX_t SymSize = SS->template getSize<ELFT>();
   if (SymSize == 0)
     fatal("cannot create a copy relocation for symbol " + toString(*SS));
 
+  // See if this symbol is in a read-only segment. If so, preserve the symbol's
+  // memory protection by reserving space in the .bss.rel.ro section.
+  bool IsReadOnly = isReadOnly(SS);
+  OutputSection<ELFT> *CopySec =
+      IsReadOnly ? Out<ELFT>::BssRelRo : Out<ELFT>::Bss;
+
   uintX_t Alignment = getAlignment(SS);
-  uintX_t Off = alignTo(Out<ELFT>::Bss->Size, Alignment);
-  Out<ELFT>::Bss->Size = Off + SymSize;
-  Out<ELFT>::Bss->updateAlignment(Alignment);
+  uintX_t Off = alignTo(CopySec->Size, Alignment);
+  CopySec->Size = Off + SymSize;
+  CopySec->updateAlignment(Alignment);
   uintX_t Shndx = SS->Sym.st_shndx;
   uintX_t Value = SS->Sym.st_value;
   // Look through the DSO's dynamic symbol table for aliases and create a
   // dynamic symbol for each one. This causes the copy relocation to correctly
   // interpose any aliases.
   for (const Elf_Sym &S : SS->file()->getGlobalSymbols()) {
     if (S.st_shndx != Shndx || S.st_value != Value)
       continue;
     auto *Alias = dyn_cast_or_null<SharedSymbol<ELFT>>(
         Symtab<ELFT>::X->find(check(S.getName(SS->file()->getStringTable()))));
     if (!Alias)
       continue;
-    Alias->OffsetInBss = Off;
+    Alias->CopyIsInBssRelRo = IsReadOnly;
+    Alias->CopyOffset = Off;
     Alias->NeedsCopyOrPltAddr = true;
     Alias->symbol()->IsUsedInRegularObj = true;
   }
-  In<ELFT>::RelaDyn->addReloc(
-      {Target->CopyRel, Out<ELFT>::Bss, SS->OffsetInBss, false, SS, 0});
+  In<ELFT>::RelaDyn->addReloc({Target->CopyRel, CopySec, Off, false, SS, 0});
 }
 
 template <class ELFT>
 static RelExpr adjustExpr(const elf::ObjectFile<ELFT> &File, SymbolBody &Body,
                           bool IsWrite, RelExpr Expr, uint32_t Type,
                           const uint8_t *Data, InputSectionBase<ELFT> &S,
                           typename ELFT::uint RelOff) {
   bool Preemptible = isPreemptible(Body, Type);

lld/trunk/ELF/Symbols.h

public:
   unsigned Is32BitMipsGot : 1;
 
   // True if this symbol is in the Iplt sub-section of the Plt.
   unsigned IsInIplt : 1;
 
   // True if this symbol is in the Igot sub-section of the .got.plt or .got.
   unsigned IsInIgot : 1;
 
+  // True if this is a shared symbol in a read-only segment which requires a
+  // copy relocation. This causes space for the symbol to be allocated in the
+  // .bss.rel.ro section.
+  unsigned CopyIsInBssRelRo : 1;
+
   // The following fields have the same meaning as the ELF symbol attributes.
   uint8_t Type;    // symbol type
   uint8_t StOther; // st_other field value
 
   // The Type field may also have this value. It means that we have not yet seen
   // a non-Lazy symbol with this name, so we don't know what its type is. The
   // Type field is normally set to this value for Lazy symbols unless we saw a
   // weak undefined symbol first, in which case we need to remember the original
public:
 
   SharedFile<ELFT> *file() { return (SharedFile<ELFT> *)this->File; }
 
   const Elf_Sym &Sym;
 
   // This field is a pointer to the symbol's version definition.
   const Elf_Verdef *Verdef;
 
-  // OffsetInBss is significant only when needsCopy() is true.
-  uintX_t OffsetInBss = 0;
+  // CopyOffset is significant only when needsCopy() is true.
+  uintX_t CopyOffset = 0;
 
   // If non-null the symbol has a Thunk that may be used as an alternative
   // destination for callers of this Symbol.
   Thunk<ELFT> *ThunkData = nullptr;
   bool needsCopy() const { return this->NeedsCopyOrPltAddr && !this->isFunc(); }
+
+  OutputSection<ELFT> *getBssSectionForCopy() const;
 };
 
 // This class represents a symbol defined in an archive file. It is
 // created from an archive file header, and it knows how to load an
 // object file from an archive to replace itself with a defined
 // symbol. If the resolver finds both Undefined and Lazy for
 // the same name, it will ask the Lazy to load a file.
 class Lazy : public SymbolBody {

lld/trunk/ELF/Symbols.cpp

case SymbolBody::DefinedCommonKind:
     return In<ELFT>::Common->OutSec->Addr + In<ELFT>::Common->OutSecOff +
            cast<DefinedCommon>(Body).Offset;
   case SymbolBody::SharedKind: {
     auto &SS = cast<SharedSymbol<ELFT>>(Body);
     if (!SS.NeedsCopyOrPltAddr)
       return 0;
     if (SS.isFunc())
       return Body.getPltVA<ELFT>();
-    return Out<ELFT>::Bss->Addr + SS.OffsetInBss;
+    return SS.getBssSectionForCopy()->Addr + SS.CopyOffset;
   }
   case SymbolBody::UndefinedKind:
     return 0;
   case SymbolBody::LazyArchiveKind:
   case SymbolBody::LazyObjectKind:
     assert(Body.symbol()->IsUsedInRegularObj && "lazy symbol reached writer");
     return 0;
   }
   llvm_unreachable("invalid symbol kind");
 }
 
 SymbolBody::SymbolBody(Kind K, StringRefZ Name, bool IsLocal, uint8_t StOther,
                        uint8_t Type)
     : SymbolKind(K), NeedsCopyOrPltAddr(false), IsLocal(IsLocal),
       IsInGlobalMipsGot(false), Is32BitMipsGot(false), IsInIplt(false),
-      IsInIgot(false), Type(Type), StOther(StOther), Name(Name) {}
+      IsInIgot(false), CopyIsInBssRelRo(false), Type(Type), StOther(StOther),
+      Name(Name) {}
 
 // Returns true if a symbol can be replaced at load-time by a symbol
 // with the same name defined in other ELF executable or DSO.
 bool SymbolBody::isPreemptible() const {
   if (isLocal())
     return false;
 
   // Shared symbols resolve to the definition in the DSO. The exceptions are

 
 template <typename ELFT>
 Undefined<ELFT>::Undefined(StringRefZ Name, bool IsLocal, uint8_t StOther,
                            uint8_t Type, InputFile *File)
     : SymbolBody(SymbolBody::UndefinedKind, Name, IsLocal, StOther, Type) {
   this->File = File;
 }
 
+template <typename ELFT>
+OutputSection<ELFT> *SharedSymbol<ELFT>::getBssSectionForCopy() const {
+  assert(needsCopy());
+  return CopyIsInBssRelRo ? Out<ELFT>::BssRelRo : Out<ELFT>::Bss;
+}
+
 DefinedCommon::DefinedCommon(StringRef Name, uint64_t Size, uint64_t Alignment,
                              uint8_t StOther, uint8_t Type, InputFile *File)
     : Defined(SymbolBody::DefinedCommonKind, Name, /*IsLocal=*/false, StOther,
               Type),
       Alignment(Alignment), Size(Size) {
   this->File = File;
 }
 

 template uint64_t SymbolBody::template getSize<ELF64LE>() const;
 template uint64_t SymbolBody::template getSize<ELF64BE>() const;
 
 template class elf::Undefined<ELF32LE>;
 template class elf::Undefined<ELF32BE>;
 template class elf::Undefined<ELF64LE>;
 template class elf::Undefined<ELF64BE>;
 
+template class elf::SharedSymbol<ELF32LE>;
+template class elf::SharedSymbol<ELF32BE>;
+template class elf::SharedSymbol<ELF64LE>;
+template class elf::SharedSymbol<ELF64BE>;
+
 template class elf::DefinedRegular<ELF32LE>;
 template class elf::DefinedRegular<ELF32BE>;
 template class elf::DefinedRegular<ELF64LE>;
 template class elf::DefinedRegular<ELF64BE>;

lld/trunk/ELF/SyntheticSections.cpp

SymbolTableSection<ELFT>::getOutputSection(SymbolBody *Sym) {
   case SymbolBody::DefinedRegularKind: {
     auto &D = cast<DefinedRegular<ELFT>>(*Sym);
     if (D.Section)
       return D.Section->OutSec;
     break;
   }
   case SymbolBody::DefinedCommonKind:
     return In<ELFT>::Common->OutSec;
-  case SymbolBody::SharedKind:
-    if (cast<SharedSymbol<ELFT>>(Sym)->needsCopy())
-      return Out<ELFT>::Bss;
+  case SymbolBody::SharedKind: {
+    auto &SS = cast<SharedSymbol<ELFT>>(*Sym);
+    if (SS.needsCopy())
+      return SS.getBssSectionForCopy();
     break;
+  }
   case SymbolBody::UndefinedKind:
   case SymbolBody::LazyArchiveKind:
   case SymbolBody::LazyObjectKind:
     break;
   }
   return nullptr;
 }
 

lld/trunk/ELF/Writer.cpp

 template <class ELFT> void Writer<ELFT>::createSyntheticSections() {
   // Initialize all pointers with NULL. This is needed because
   // you can call lld::elf::main more than once as a library.
   memset(&Out<ELFT>::First, 0, sizeof(Out<ELFT>));
 
   // Create singleton output sections.
   Out<ELFT>::Bss =
       make<OutputSection<ELFT>>(".bss", SHT_NOBITS, SHF_ALLOC | SHF_WRITE);
+  Out<ELFT>::BssRelRo = make<OutputSection<ELFT>>(".bss.rel.ro", SHT_NOBITS,
+                                                  SHF_ALLOC | SHF_WRITE);
   In<ELFT>::DynStrTab = make<StringTableSection<ELFT>>(".dynstr", true);
   In<ELFT>::Dynamic = make<DynamicSection<ELFT>>();
   Out<ELFT>::EhFrame = make<EhOutputSection<ELFT>>();
   In<ELFT>::RelaDyn = make<RelocationSection<ELFT>>(
       Config->Rela ? ".rela.dyn" : ".rel.dyn", Config->ZCombreloc);
   In<ELFT>::ShStrTab = make<StringTableSection<ELFT>>(".shstrtab", false);
 
   Out<ELFT>::ElfHeader = make<OutputSectionBase>("", 0, SHF_ALLOC);
template <class ELFT> bool elf::isRelroSection(const OutputSectionBase *Sec) {
   if (Sec == In<ELFT>::GotPlt->OutSec)
     return Config->ZNow;
   if (Sec == In<ELFT>::Dynamic->OutSec)
     return true;
   if (In<ELFT>::Got && Sec == In<ELFT>::Got->OutSec)
     return true;
   if (In<ELFT>::MipsGot && Sec == In<ELFT>::MipsGot->OutSec)
     return true;
+  if (Sec == Out<ELFT>::BssRelRo)
+    return true;
   StringRef S = Sec->getName();
   return S == ".data.rel.ro" || S == ".ctors" || S == ".dtors" || S == ".jcr" ||
          S == ".eh_frame" || S == ".openbsd.randomdata";
 }
 
 template <class ELFT>
 static bool compareSectionsNonScript(const OutputSectionBase *A,
                                      const OutputSectionBase *B) {
finalizeSynthetic<ELFT>(
        In<ELFT>::RelaPlt,   In<ELFT>::Plt,        In<ELFT>::Iplt,
        In<ELFT>::Plt,       In<ELFT>::EhFrameHdr, In<ELFT>::VerSym,
        In<ELFT>::VerNeed,   In<ELFT>::Dynamic});
 }
 
 template <class ELFT> void Writer<ELFT>::addPredefinedSections() {
   if (Out<ELFT>::Bss->Size > 0)
     OutputSections.push_back(Out<ELFT>::Bss);
+  if (Out<ELFT>::BssRelRo->Size > 0)
+    OutputSections.push_back(Out<ELFT>::BssRelRo);
 
   auto OS = dyn_cast_or_null<OutputSection<ELFT>>(findSection(".ARM.exidx"));
   if (OS && !OS->Sections.empty() && !Config->Relocatable)
     OS->addSection(make<ARMExidxSentinelSection<ELFT>>());
 
   addInputSec(In<ELFT>::SymTab);
   addInputSec(In<ELFT>::ShStrTab);
   addInputSec(In<ELFT>::StrTab);

lld/trunk/test/ELF/Inputs/copy-rel-pie.s

+.data
 .global foo
 .type foo, @object
 .size foo, 4
 foo:
 .long 0
 
+.text
 .global bar
 .type bar, @function
 bar:

lld/trunk/test/ELF/Inputs/relocation-copy-relro.s

+.rodata
+.globl a
+.size a, 4
+.type a, @object
+a:
+.word 1
+
+.section .data.rel.ro,"aw",%progbits
+.globl b
+.size b, 4
+.type b, @object
+b:
+.word 2

lld/trunk/test/ELF/relocation-copy-relro.s

+// REQUIRES: x86
+// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %p/Inputs/relocation-copy-relro.s -o %t2.o
+// RUN: ld.lld -shared %t2.o -o %t.so
+// RUN: ld.lld %t.o %t.so -o %t3
+// RUN: llvm-readobj -program-headers -s -r %t3 | FileCheck %s
+
+// CHECK:        Name: .bss.rel.ro (48)
+// CHECK-NEXT:   Type: SHT_NOBITS (0x8)
+// CHECK-NEXT:   Flags [ (0x3)
+// CHECK-NEXT:     SHF_ALLOC (0x2)
+// CHECK-NEXT:     SHF_WRITE (0x1)
+// CHECK-NEXT:   ]
+// CHECK-NEXT:   Address: 0x2020B0
+// CHECK-NEXT:   Offset: 0x20B0
+// CHECK-NEXT:   Size: 8
+
+// CHECK: 0x2020B0 R_X86_64_COPY a 0x0
+// CHECK: 0x2020B4 R_X86_64_COPY b 0x0
+
+// CHECK:      Type: PT_GNU_RELRO (0x6474E552)
+// CHECK-NEXT: Offset: 0x2000
+// CHECK-NEXT: VirtualAddress: 0x202000
+// CHECK-NEXT: PhysicalAddress: 0x202000
+// CHECK-NEXT: FileSize: 176
+// CHECK-NEXT: MemSize: 4096
+
+.text
+.global _start
+_start:
+movl $1, a
+movl $2, b