[ELF] Align the first section of a PT_LOAD even if its type is SHT_NOBITS

[MaskRay]

Summary

Reported at https://reviews.llvm.org/D64930#1642223

If the only section of a PT_LOAD is a SHT_NOBITS section (e.g. .bss), we
may not align its sh_offset. p_offset of the PT_LOAD will be set to
sh_offset, and we will get p_offset!=p_vaddr (mod p_align). If such
executable is mapped by the Linux kernel, it will segfault.

After D64906, this may happen the non-linker script case.

The linker script case has had this issue for a long time.
This was fixed by rL321657 (but the test linkerscript/nobits-offset.s
failed to test a SHT_NOBITS section), but broken by rL345154.

[peter.smith]

LGTM. I've tested this on the example and in qemu-aarch64 the p_offset is aligned and the ELF file runs in the same way as when -znow isn't used.

[pcc]

Thanks @MaskRay and @peter.smith for investigating this problem, I can confirm that this change fixes it for me.

ELF/Writer.cpp

2277 ↗

(On Diff #216857)

Maybe it would be better to reorder the if statements here to make the logic slightly easier to understand? This seems to work:

diff --git a/lld/ELF/Writer.cpp b/lld/ELF/Writer.cpp
index fde253796a1..f14951f403b 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -2269,8 +2269,16 @@ template <class ELFT> void Writer<ELFT>::fixSectionAlignments() {
 // same with its virtual address modulo the page size, so that the loader can
 // load executables without any address adjustment.
 static uint64_t computeFileOffset(OutputSection *os, uint64_t off) {
-  // File offsets are not significant for .bss sections. By convention, we keep
-  // section offsets monotonically increasing rather than setting to zero.
+  // The first section in a PT_LOAD has to have congruent offset and address
+  // module the page size.
+  if (os->ptLoad && os->ptLoad->firstSec == os) {
+    uint64_t alignment = std::max<uint64_t>(os->alignment, config->maxPageSize);
+    return alignTo(off, alignment, os->addr);
+  }
+
+  // File offsets are not significant for .bss sections other than the first one
+  // in a PT_LOAD. By convention, we keep section offsets monotonically
+  // increasing rather than setting to zero.
   if (os->type == SHT_NOBITS)
     return off;
 
@@ -2278,16 +2286,9 @@ static uint64_t computeFileOffset(OutputSection *os, uint64_t off) {
   if (!os->ptLoad)
     return alignTo(off, os->alignment);
 
-  // The first section in a PT_LOAD has to have congruent offset and address
-  // module the page size.
-  OutputSection *first = os->ptLoad->firstSec;
-  if (os == first) {
-    uint64_t alignment = std::max<uint64_t>(os->alignment, config->maxPageSize);
-    return alignTo(off, alignment, os->addr);
-  }
-
   // If two sections share the same PT_LOAD the file offset is calculated
   // using this formula: Off2 = Off1 + (VA2 - VA1).
+  OutputSection *first = os->ptLoad->firstSec;
   return first->offset + os->addr - first->addr;
 }

[MaskRay]

Restore the logic to be similar to r321657 as pcc suggested

lld/trunk/ELF/Writer.cpp

for (const PhdrEntry *p : part.phdrs)
       }
   }
 }
 
 // Compute an in-file position for a given section. The file offset must be the
 // same with its virtual address modulo the page size, so that the loader can
 // load executables without any address adjustment.
 static uint64_t computeFileOffset(OutputSection *os, uint64_t off) {
-  // File offsets are not significant for .bss sections. By convention, we keep
-  // section offsets monotonically increasing rather than setting to zero.
+  // The first section in a PT_LOAD has to have congruent offset and address
+  // module the page size.
+  if (os->ptLoad && os->ptLoad->firstSec == os) {
+    uint64_t alignment = std::max<uint64_t>(os->alignment, config->maxPageSize);
+    return alignTo(off, alignment, os->addr);
+  }
+
+  // File offsets are not significant for .bss sections other than the first one
+  // in a PT_LOAD. By convention, we keep section offsets monotonically
+  // increasing rather than setting to zero.
    if (os->type == SHT_NOBITS)
      return off;
 
   // If the section is not in a PT_LOAD, we just have to align it.
   if (!os->ptLoad)
     return alignTo(off, os->alignment);
 
-  // The first section in a PT_LOAD has to have congruent offset and address
-  // module the page size.
-  OutputSection *first = os->ptLoad->firstSec;
-  if (os == first) {
-    uint64_t alignment = std::max<uint64_t>(os->alignment, config->maxPageSize);
-    return alignTo(off, alignment, os->addr);
-  }
-
   // If two sections share the same PT_LOAD the file offset is calculated
   // using this formula: Off2 = Off1 + (VA2 - VA1).
+  OutputSection *first = os->ptLoad->firstSec;
   return first->offset + os->addr - first->addr;
 }
 
 // Set an in-file position to a given section and returns the end position of
 // the section.
 static uint64_t setFileOffset(OutputSection *os, uint64_t off) {
   off = computeFileOffset(os, off);
   os->offset = off;

lld/trunk/test/ELF/linkerscript/nobits-offset.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
 # RUN: echo "SECTIONS { \
 # RUN:         .sec1 (NOLOAD) : { . += 1; } \
-# RUN:         .text          : { *(.text) } \
+# RUN:         .bss           : { *(.bss) } \
 # RUN:       };" > %t.script
 # RUN: ld.lld %t.o -T %t.script -o %t
-# RUN: llvm-readelf --sections %t | FileCheck %s
+# RUN: llvm-readelf -S -l %t | FileCheck %s
 
-# We used to misalign section offsets if the first section in a
-# PT_LOAD was SHT_NOBITS.
+## If a SHT_NOBITS section is the only section of a PT_LOAD segment,
+## p_offset will be set to the sh_offset field of the section. Check we align
+## sh_offset to sh_addr modulo max-page-size, so that p_vaddr=p_offset (mod
+## p_align).
 
-# CHECK: [ 2] .text  PROGBITS  0000000000000010 001010 000010 00  AX  0   0 16
+# CHECK: Name Type   Address          Off     Size   ES Flg Lk Inf Al
+# CHECK: .bss NOBITS 0000000000000400 001400  000001 00  WA  0   0 1024
 
-.global _start
-_start:
-  nop
-.p2align 4
+# CHECK: Type Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
+# CHECK: LOAD 0x001400 0x0000000000000400 0x0000000000000400 0x000000 0x000001 RW  0x1000
+
+# CHECK: 00 .bss
+
+.bss
+.p2align 10
+.byte 0

lld/trunk/test/ELF/nobits-offset.s

+# REQUIRES: aarch64
+# RUN: llvm-mc -filetype=obj -triple=aarch64 %s -o %t.o
+# RUN: ld.lld %t.o -o %t
+# RUN: llvm-readelf -S -l %t | FileCheck %s
+
+## If a SHT_NOBITS section is the only section of a PT_LOAD segment,
+## p_offset will be set to the sh_offset field of the section. Check we align
+## sh_offset to sh_addr modulo max-page-size, so that p_vaddr=p_offset (mod
+## p_align).
+
+# CHECK: Name Type   Address          Off     Size   ES Flg Lk Inf Al
+# CHECK: .bss NOBITS 0000000000221000 001000  000001 00  WA  0   0 4096
+
+# CHECK: Type Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
+# CHECK: LOAD 0x001000 0x0000000000221000 0x0000000000221000 0x000000 0x000001 RW  0x10000
+
+# CHECK: 02 .bss
+
+.bss
+.p2align 12
+.byte 0

lld/trunk/test/ELF/relocatable.s

 # CHECKEXE-NEXT:      ABIVersion: 0
 # CHECKEXE-NEXT:      Unused: (00 00 00 00 00 00 00)
 # CHECKEXE-NEXT:    }
 # CHECKEXE-NEXT:    Type: Executable
 # CHECKEXE-NEXT:    Machine: EM_X86_64
 # CHECKEXE-NEXT:    Version: 1
 # CHECKEXE-NEXT:    Entry: 0x201000
 # CHECKEXE-NEXT:    ProgramHeaderOffset: 0x40
-# CHECKEXE-NEXT:    SectionHeaderOffset: 0x11F8
+# CHECKEXE-NEXT:    SectionHeaderOffset: 0x21A0
 # CHECKEXE-NEXT:    Flags [
 # CHECKEXE-NEXT:    ]
 # CHECKEXE-NEXT:    HeaderSize: 64
 # CHECKEXE-NEXT:    ProgramHeaderEntrySize: 56
 # CHECKEXE-NEXT:    ProgramHeaderCount: 5
 # CHECKEXE-NEXT:    SectionHeaderEntrySize: 64
 # CHECKEXE-NEXT:    SectionHeaderCount: 7
 # CHECKEXE-NEXT:    StringTableSectionIndex: 5

lld/trunk/test/ELF/relocation-copy-align-common.s

 # CHECK:        Index:
 # CHECK:        Name: .bss
 # CHECK-NEXT:   Type: SHT_NOBITS
 # CHECK-NEXT:   Flags [
 # CHECK-NEXT:     SHF_ALLOC
 # CHECK-NEXT:     SHF_WRITE
 # CHECK-NEXT:   ]
 # CHECK-NEXT:   Address: 0x203000
-# CHECK-NEXT:   Offset: 0x20B0
+# CHECK-NEXT:   Offset: 0x3000
 # CHECK-NEXT:   Size: 16
 # CHECK-NEXT:   Link: 0
 # CHECK-NEXT:   Info: 0
 # CHECK-NEXT:   AddressAlignment: 8
 # CHECK-NEXT:   EntrySize: 0
 # CHECK-NEXT: }
 
 # CHECK:      Relocations [