[LLD] Ensure explicit PT_GNU_RELRO has correct p_align

[bd1976bris]

PT_GNU_RELRO was introduced by Jakub Jelinek in https://sourceware.org/legacy-ml/binutils/2004-01/msg00070.html (Jakub Jelinek - [RFC PATCH] Little hardening DSOs/executables against exploits). p_align for PT_GNU_RELRO has always been set to 1.

There was a regression in behavior for the value of p_align for explicitly defined PT_GNU_RELRO phdrs in:
5a58e98.

Before the above change, p_align was set to 1: https://godbolt.org/z/vdEPne7Y7.
After the change, it was set to the largest alignment of any of the assigned sections: https://godbolt.org/z/E7r3xa7qj.

Fix this regression by forcing p_align to 1.

[llvmbot]

@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-lld

Author: bd1976bris (bd1976bris)

Changes

PT_GNU_RELRO was introduced by Jakub Jelinek in https://sourceware.org/legacy-ml/binutils/2004-01/msg00070.html (Jakub Jelinek - [RFC PATCH] Little hardening DSOs/executables against exploits). p_align for PT_GNU_RELRO has always been set to 1.

There was a regression in behavior for the value of p_align for explicitly defined PT_GNU_RELRO phdrs in:
5a58e98.

Before the above change, p_align was set to 1: https://godbolt.org/z/vdEPne7Y7.
After the change, it was set to the largest alignment of any of the assigned sections: https://godbolt.org/z/E7r3xa7qj.

Fix this regression by forcing p_align to 1.

---

Full diff: https://github.com/llvm/llvm-project/pull/133022.diff

2 Files Affected:

• (modified) lld/ELF/LinkerScript.cpp (+8-1)
• (added) lld/test/ELF/linkerscript/explicit-relro-phdr.test (+21)

diff --git a/lld/ELF/LinkerScript.cpp b/lld/ELF/LinkerScript.cpp
index 120f5271cf229..bc2b1e8eef618 100644
--- a/lld/ELF/LinkerScript.cpp
+++ b/lld/ELF/LinkerScript.cpp
@@ -1676,13 +1676,20 @@ SmallVector<std::unique_ptr<PhdrEntry>, 0> LinkerScript::createPhdrs() {
 
   // Add output sections to program headers.
   for (OutputSection *sec : ctx.outputSections) {
-    // Assign headers specified by linker script
+    // Assign headers specified by linker script.
     for (size_t id : getPhdrIndices(sec)) {
       ret[id]->add(sec);
       if (!phdrsCommands[id].flags)
         ret[id]->p_flags |= sec->getPhdrFlags();
     }
   }
+
+  // LLD forces the p_align to 1 for implicitly created PT_GNU_RELRO headers.
+  // Ensure this also happens when the PT_GNU_RELRO phdr is explicitly created
+  // via a linker script PHDRS command.
+  for (auto &phdr : ret)
+    if (phdr->p_type == PT_GNU_RELRO)
+      phdr->p_align = 1;
   return ret;
 }
 
diff --git a/lld/test/ELF/linkerscript/explicit-relro-phdr.test b/lld/test/ELF/linkerscript/explicit-relro-phdr.test
new file mode 100644
index 0000000000000..274d2b20694f4
--- /dev/null
+++ b/lld/test/ELF/linkerscript/explicit-relro-phdr.test
@@ -0,0 +1,21 @@
+## Test that a PT_GNU_RELRO phdr explicitly specified via a linker script
+## PHDRS command has the expected fags and alignment.
+
+# REQUIRES: x86
+# RUN: rm -rf %t && split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/relro.s -o %t.o
+
+# RUN: ld.lld %t.o -T %t/test.script -o %t.elf
+# RUN: llvm-readelf -l %t.elf | FileCheck %s
+
+##       Type      Offset  VirtAddr PhysAddr FileSiz MemSiz  Flg Align
+# CHECK: GNU_RELRO 0x[[#]] 0x[[#]]  0x[[#]]  0x[[#]] 0x[[#]] R   0x1
+
+#--- relro.s
+.section .data.rel.ro.test, "aw"
+.align 8
+.quad 0
+
+#--- test.script
+PHDRS { ph_gnu_relro PT_GNU_RELRO FLAGS (0x4); }
+SECTIONS { .data.rel.ro : { *(.data.rel.ro.*) } : ph_gnu_relro }

[MaskRay]

Thanks for sharing this intriguing case! However, I’m not convinced this is a regression related to PHDRS specifying PT_GNU_RELRO. In both musl and glibc, the p_align value of PT_GNU_RELRO is simply ignored.

For situations like this, where the correct behavior isn’t entirely clear, I lean toward simplifying the code. The alignment of PT_GNU_RELRO, when defined in PHDRS, should match that of other program headers. There doesn’t seem to be a compelling reason to handle it differently.

[bd1976bris]

Thanks for sharing this intriguing case! However, I’m not convinced this is a regression related to PHDRS specifying PT_GNU_RELRO. In both musl and glibc, the p_align value of PT_GNU_RELRO is simply ignored.

For situations like this, where the correct behavior isn’t entirely clear, I lean toward simplifying the code. The alignment of PT_GNU_RELRO, when defined in PHDRS, should match that of other program headers. There doesn’t seem to be a compelling reason to handle it differently.

Thanks @MaskRay - however, I see the situation differently:

With gnu-ld, p_align for PT_GNU_RELRO has always been set to 1, dating back to the original introduction of this feature in 2004. That's also the case when the PT_GNU_RELRO is specified explicitly in a PHDRS command for gnu-ld. So, p_align = 1 can reasonably be considered the de facto standard. Moreover, it seems wrong that explicitly specifying PT_GNU_RELRO through a PHDRS command would result in behavior different from when the linker implicitly creates it.

Linkers don't offer a mechanism to set the p_align value, so forcing it to 1 requires a linker patch.

In the interest of transparency, I would like to note that I care about this issue because we have an internal spec where PT_GNU_RELRO.p_align is 1, based on LLD's behaviour at the time. I would like to apologize that this issue wasn't raised at the time the change was made.

Perhaps one solution would be to unify the explicit and implicit code paths for PT_GNU_RELRO? A more consistent way to handle PT_GNU_RELRO with a PHDRS command (IMO) would be to allow users to specify it with the following restrictions:

1. Only one PT_GNU_RELRO phdr is allowed.
2. Explicit assignment to this phdr is disallowed; instead, just as in the implicitly created case, any sections between DATA_SEGMENT_ALIGN and DATA_SEGMENT_RELRO_END would be implicitly assigned to it.

If we want to do the above, then fixing the p_align value for PT_GNU_RELRO now would ensure no behavior change when we implement the unification.