[ELF] --no-rosegment: don't mark read-only PT_LOAD segments executable

[MaskRay]

Once we move .lrodata after .bss (#78521), or if we use SECTIONS
commands, certain read-only sections may be in their own PT_LOAD, not in
the traditional "text segment". Current --no-rosegment code may
unnecessarily mark read-only PT_LOAD executable. Fix it.

[llvmbot]

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-elf

Author: Fangrui Song (MaskRay)

Changes

Once we move .lrodata after .bss (#78521), or if we use SECTIONS
commands, certain read-only sections may be in their own PT_LOAD, not in
the traditional "text segment". Current --no-rosegment code may
unnecessarily mark read-only PT_LOAD executable. Fix it.

---

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

2 Files Affected:

• (modified) lld/ELF/Writer.cpp (+16-12)
• (modified) lld/test/ELF/segments.s (+1-1)

diff --git a/lld/ELF/Writer.cpp b/lld/ELF/Writer.cpp
index 6df43a34be013a..53ca70b59076fd 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -2353,17 +2353,12 @@ static bool needsPtLoad(OutputSection *sec) {
   return true;
 }
 
-// Linker scripts are responsible for aligning addresses. Unfortunately, most
-// linker scripts are designed for creating two PT_LOADs only, one RX and one
-// RW. This means that there is no alignment in the RO to RX transition and we
-// cannot create a PT_LOAD there.
+// Adjust phdr flags according to certain options.
 static uint64_t computeFlags(uint64_t flags) {
   if (config->omagic)
     return PF_R | PF_W | PF_X;
   if (config->executeOnly && (flags & PF_X))
     return flags & ~PF_R;
-  if (config->singleRoRx && !(flags & PF_W))
-    return flags | PF_X;
   return flags;
 }
 
@@ -2451,7 +2446,7 @@ SmallVector<PhdrEntry *, 0> Writer<ELFT>::createPhdrs(Partition &part) {
     // Segments are contiguous memory regions that has the same attributes
     // (e.g. executable or writable). There is one phdr for each segment.
     // Therefore, we need to create a new phdr when the next section has
-    // different flags or is loaded at a discontiguous address or memory region
+    // compatible flags or is loaded at a discontiguous address or memory region
     // using AT or AT> linker script command, respectively.
     //
     // As an exception, we don't create a separate load segment for the ELF
@@ -2465,13 +2460,22 @@ SmallVector<PhdrEntry *, 0> Writer<ELFT>::createPhdrs(Partition &part) {
     // so when hasSectionsCommand, since we cannot introduce the extra alignment
     // needed to create a new LOAD)
     uint64_t newFlags = computeFlags(sec->getPhdrFlags());
+    // When --no-rosegment is specified, RO and RX sections are compatible.
+    uint32_t diff = flags ^ newFlags;
+    if (config->singleRoRx && !(newFlags & PF_W))
+      diff &= ~PF_X;
+    if (diff)
+      load = nullptr;
+
     bool sameLMARegion =
         load && !sec->lmaExpr && sec->lmaRegion == load->firstSec->lmaRegion;
-    if (!(load && newFlags == flags && sec != relroEnd &&
-          sec->memRegion == load->firstSec->memRegion &&
-          (sameLMARegion || load->lastSec == Out::programHeaders) &&
-          (script->hasSectionsCommand || sec->type == SHT_NOBITS ||
-           load->lastSec->type != SHT_NOBITS))) {
+    if (load && sec != relroEnd &&
+        sec->memRegion == load->firstSec->memRegion &&
+        (sameLMARegion || load->lastSec == Out::programHeaders) &&
+        (script->hasSectionsCommand || sec->type == SHT_NOBITS ||
+         load->lastSec->type != SHT_NOBITS)) {
+      load->p_flags |= newFlags;
+    } else {
       load = addHdr(PT_LOAD, newFlags);
       flags = newFlags;
     }
diff --git a/lld/test/ELF/segments.s b/lld/test/ELF/segments.s
index ee171174ac7ca4..1fe248afa88480 100644
--- a/lld/test/ELF/segments.s
+++ b/lld/test/ELF/segments.s
@@ -44,7 +44,7 @@
 # NOROSEGMENT1-NEXT:  LOAD           0x001006 0x0000000000000006 0x0000000000000006 0x000001 0x000001 RW  0x1000
 # NOROSEGMENT1-NEXT:  LOAD           0x001007 0x0000000000000007 0x0000000000000007 0x000002 0x000002 R E 0x1000
 # NOROSEGMENT1-NEXT:  LOAD           0x001009 0x0000000000000009 0x0000000000000009 0x000001 0x000001 RW  0x1000
-# NOROSEGMENT1-NEXT:  LOAD           0x00100a 0x000000000000000a 0x000000000000000a 0x000001 0x000001 R E 0x1000
+# NOROSEGMENT1-NEXT:  LOAD           0x00100a 0x000000000000000a 0x000000000000000a 0x000001 0x000001 R   0x1000
 # NOROSEGMENT1-NEXT:  GNU_STACK      0x000000 0x0000000000000000 0x0000000000000000 0x000000 0x000000 RW  0
 
 # RUN: ld.lld -N a.o -o omagic

[smithp35]

I've got some small comments, but no objections should anyone else want to approve.

[smithp35]

Did you mean "create a new phdr when the next section has incompatible flags"?

[MaskRay]

Thanks for the catch. Fixed the typo

[smithp35]

Could be worth calling diff incompatible not quite as precise as diff, but it matches the earlier comment about incompatible flags. Works quite well with

if (incompatible)
  load = nullptr;

Not got a strong opinion, could go with either.

[MaskRay]

Renamed

[smithp35]

There's an interesting conflict here between singleRoRx and execute-only, could be worth making these options incompatible in Driver.cpp as singleRoRx breaks execute-only.

This isn't a regression caused by this patch though so could be done in a different patch.

[MaskRay]

The initial --execute-only patch (https://reviews.llvm.org/D49456) checks for the incompatibility: it has an error --execute-only does not support intermingling data and code.

It also has this error "--execute-only and -no-rosegment cannot be used together", which is untested. (The !Script->HasSectionsCommand condition is redundant as hasSectionsCommand is not set in Driver.cpp yet)

[smithp35]

LGTM on my side, thanks for the updates.