This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by MaskRay on Aug 23 2019, 8:19 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Aug 23 2019, 8:19 AM
peter.smith accepted this revision.Aug 23 2019, 9:52 AM

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.

This revision is now accepted and ready to land.Aug 23 2019, 9:52 AM
pcc added a comment.Aug 23 2019, 9:53 AM

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 updated this revision to Diff 216984.Aug 23 2019, 5:39 PM

Restore the logic to be similar to r321657 as pcc suggested

This revision was automatically updated to reflect the committed changes.