[LLD][ELF] InX::BssRelRo should check section contents before marking relro

[peter.smith]

Summary

When a linker script is used with a pattern like { *(.bss .bss.*) } the InX::BssRelRo section will match against .bss.*. We want to make sure that when InX::BssRelRo hasn't created any .bss.rel.ro InputSections we don't report the .bss OutputSection as relro. We also want to give an error message if relro and non relro bss is placed within the same OutputSection.

This is an attempt at fixing the problem that motivated r318924 (D40364) in a way that doesn't need to check for empty or zero sized sections. The Synthetic Sections InX::BssRelRo and InX::Bss behave differently to other SyntheticSections in that they act as a proxy for creating .bss.rel.ro or .bss InputSections for copy relocations in their parent OutputSections. This means that they are always removed by removeUnusedSyntheticSections() and always have a zero size. This patch attempts to do the following:

• Move the name test before the InX::BssRelRo so that if it matches into an existing OutputSection that is already relro we are relro.
• If the OutputSection name is .bss.rel.ro then this is strong statement of intent like .data.rel.ro then we are relro.
• If all the InputSections in the parent OutputSection are .bss.rel.ro then we are relro.
• If none of the InputSections in the parent OutputSection are .bss.rel.ro then we are not relro.
• If there are both relro and non-relro InputSections then give an error message.

[peter.smith]

peter.smith created this revision.
Herald added a subscriber: emaste.

When a linker script is used with a pattern like { *(.bss .bss.*) } the InX::BssRelRo section will match against .bss.*. We want to make sure that when InX::BssRelRo hasn't created >>any .bss.rel.ro InputSections we don't report the .bss OutputSection as relro. We also want to give an error message if relro and non relro bss is placed within the same >>OutputSection.

I wonder if we need to do that much.

Any thoughts on the attached patch? It uses the idea of depending on the
output section name for .bss.rel.ro but doesn't issue an error message
if the user puts .bss.rel.ro in a rw section. That is similar to what
happens with .data.rel.ro, no?

I'm happy with Rafael's patch and have incorporated it into the review. This passes the important test case, I've removed the test case for the error message as this is less likely to happen as a user would have to explicitly name a .bss.rel.ro output section and then put .bss in it.

[jhenderson]

test/ELF/relro-copyrel-bss-script.s

4–5 ↗

(On Diff #125305)

Is there a different test already for the opposite case, where we have .bss.rel.ro (with or without any other .bss), but with the same script? If not, I think that it would be beneficial to add it.

[peter.smith]

Thanks for the comment, I've adapted the test to include parts of the previous error if mix .bss and .bss.rel.ro. I'm not sure if it adds much value in having two separate test cases, but do let me know if you'd like them split up?

[jhenderson]

LGTM, with one minor test comment inline.

To summarise the new behaviour as I understand it: "if linker script includes .bss.rel.ro in a section named something else and isn't otherwise relro, then don't treat it as relro". Assuming that's correct, I think that's fine. In my view, if you've written a linker script like that, you've done so deliberately, and don't want the linker preventing you having output as you requested. Yes, it means that there's a theoretical security hole, but you did ask for it, so it's not for the linker to judge.

test/ELF/relro-copyrel-bss-script.s

24–25 ↗

(On Diff #125331)

Does this bit do anything in the test? If not, please delete it. If it does, please comment on its purpose.

[peter.smith]

I agree with your summary, and I'll remove the .space pre-commit. Will wait till tomorrow to do so.

test/ELF/relro-copyrel-bss-script.s

24–25 ↗

(On Diff #125331)

I'll remove it. It is a hold over from a previous test where I wanted some non bss, non relro data after the relro for .dynamic etc, but that won't happen with this linker script.

lld/trunk/ELF/Writer.cpp

if (Sec == InX::GotPlt->getParent())
     return Config->ZNow;
 
   // .dynamic section contains data for the dynamic linker, and
   // there's no need to write to it at runtime, so it's better to put
   // it into RELRO.
   if (Sec == InX::Dynamic->getParent())
     return true;
 
-  // .bss.rel.ro is used for copy relocations for read-only symbols.
-  // Since the dynamic linker needs to process copy relocations, the
-  // section cannot be read-only, but once initialized, they shouldn't
-  // change.
-  if (Sec == InX::BssRelRo->getParent())
-    return true;
-
   // Sections with some special names are put into RELRO. This is a
   // bit unfortunate because section names shouldn't be significant in
   // ELF in spirit. But in reality many linker features depend on
   // magic section names.
   StringRef S = Sec->Name;
-  return S == ".data.rel.ro" || S == ".ctors" || S == ".dtors" || S == ".jcr" ||
-         S == ".eh_frame" || S == ".openbsd.randomdata";
+  return S == ".data.rel.ro" || S == ".bss.rel.ro" || S == ".ctors" ||
+         S == ".dtors" || S == ".jcr" || S == ".eh_frame" ||
+         S == ".openbsd.randomdata";
 }
 
 // We compute a rank for each section. The rank indicates where the
 // section should be placed in the file.  Instead of using simple
 // numbers (0,1,2...), we use a series of flags. One for each decision
 // point when placing the section.
 // Using flags has two key properties:
 // * It is easy to check if a give branch was taken.

lld/trunk/test/ELF/relro-copyrel-bss-script.s

+// REQUIRES: x86
+// 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
+
+// A linker script that will map .bss.rel.ro into .bss.
+// RUN: echo "SECTIONS { \
+// RUN: .bss : { *(.bss) *(.bss.*) } \
+// RUN: } " > %t.script
+
+// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t3.o
+// RUN: ld.lld %t3.o %t.so -z relro -o %t --script=%t.script 2>&1
+// RUN: llvm-readobj --program-headers %t | FileCheck %s
+        .section .text, "ax", @progbits
+        .global bar
+        .global foo
+        .global _start
+_start:
+        callq bar
+        // Will produce .bss.rel.ro that will match in .bss, this will lose
+        // the relro property of the copy relocation.
+        .quad foo
+
+        // Non relro bss
+        .bss
+        // make large enough to affect PT_GNU_RELRO MemSize if this was marked
+        // as relro.
+        .space 0x2000
+
+// CHECK:     Type: PT_GNU_RELRO (0x6474E552)
+// CHECK-NEXT:     Offset:
+// CHECK-NEXT:     VirtualAddress:
+// CHECK-NEXT:     PhysicalAddress:
+// CHECK-NEXT:     FileSize:
+// CHECK-NEXT:     MemSize: 4096
+// CHECK-NEXT:     Flags [ (0x4)
+// CHECK-NEXT:       PF_R (0x4)
+// CHECK-NEXT:     ]
+// CHECK-NEXT:     Alignment: 1
+// CHECK-NEXT:   }