[lld|ELF] Fix virtual address of the first PT_LOAD segment when linker script is being used

[evgeny777]

Summary

When linker script is used base address of PT_PHDR is set to 0 (zero). When you try to run linked image, Linux kernel tries to map executable segment starting from zero VA, which can't be used by userspace process. This causes exec() to return -1 with errno = EPERM (not a single instruction is executed)

[ruiu]

ELF/Writer.cpp

70–71

I think function overloading is used too casually here. You can always pass an argument explicitly instead of defining two functions with the same name.

1195

assignFileOffsets should set file offsets only as the name implies, but this function call will change section VAs. This should be moved to assignAddresses.

[evgeny777]

Review updated according to comments from Rui Ueyama

[ruiu]

ELF/LinkerScript.cpp

37

Target.h contains this declaration, so please remove.

232–233

It is not clear for me as to what you are trying to do with this code. Can you elaborate?

ELF/Writer.cpp

1183–1184

I'm wondering why you need to update this function. File offsets are basically orthogonal to section addresses, no?

[evgeny777]

ELF/LinkerScript.cpp

232–233

Ok, what I need to do is to calculate VA of the first PT_LOAD segment, which contains PT_PHDR
To do this I calculate suitable page aligned VA, which is less than first section address and FirstSectionVA - BaseVA >= (ELF_hdr size + PHDR size)
Does this make sense?

ELF/Writer.cpp

1183–1184

File offsets and section VA should be equal modulo page size, shouldn't they?
Looking at kernel sources I don't see any relocation, only mmap'ing

[ruiu]

ELF/LinkerScript.cpp

232–233

After applying this patch and poking around, I think I understand what this is for. So you are trying to place ELF and Program headers right before the first section.

I think this code is a bit tricky and has room for improvement.

Instead of computing BaseVA in the loop, I'd just record the minimum VA.

254

... by adding MinVA = std::min(MinVA, Dot) here.

260

... and assign MinVA - Out<ELFT>::ElfHeader->getSize() - Out<ELFT>::ProgramHeaders->getSize() to ElfHeader and MinVA - Out<ELFT>::ProgramHeaders->getSize() to ProgramHeaders.

[evgeny777]

ELF/LinkerScript.cpp

254

MinVA isn't necessary VA of the very first section. Technically speaking if you take MinVA here, you should also do the same in assignFileOffsets
Meanwhile first PT_LOAD segment isn't obliged to have lowest VA, it can be placed anywhere.

[evgeny777]

Review updated: now minimum VA of section is calculated (it turned out that gold also does this).
I had to update assignFileOffsets as well to reflect this change

[ruiu]

This is looking better.

ELF/LinkerScript.cpp

228

This is a cool way to write a largest number, but is it only me who thinks uintX_t(-1) is more conventional?

260

I'm wondering if ELF header needs to start at beginning of a page. What if you remove alignDown?

[evgeny777]

ELF/LinkerScript.cpp

228

Who knows :). I don't mind replacing it to uintX_t(-1), if you like.

260

Sorry, but this will not work

(FileOffset mod PageSize) should be equal to (VA mod PageSize)
ELF header has file offset equal to zero (0), so

VA mod PageSize = 0, means VA should be page aligned

[ruiu]

ELF/LinkerScript.cpp

228

Well, on second thought, I think numeric_limits is probably better.

260

Got it. Please add a comment to describe this piece of code.

// ELF and Program headers need to be right before the first section in memory.
// Set their addresses accordingly.

ELF/Writer.cpp

1186

getFileAlignment is the function to take care of this. Doesn't it work?

[evgeny777]

ELF/LinkerScript.cpp

260

Sure. May I commit this patch after that?

ELF/Writer.cpp

1186

It is not used in case section has SHT_NOBITS type and linker script may put such section (for example .bss) first
This can also be fixed calling getFileAlignment() for such sections, in case they go first. Let me know your thoughts

[ruiu]

ELF/Writer.cpp

1186

I prefer just calling getFileAlignment for all sections because it's simpler.

[evgeny777]

ELF/Writer.cpp

1186

Ok current version has this code

for (OutputSectionBase<ELFT> *Sec : OutputSections) {
   if (Sec->getType() == SHT_NOBITS) {
     Sec->setFileOffset(Off);
     continue;
   }

   Off = getFileAlignment<ELFT>(Off, Sec);

As you may have noticed getFileAlignment is not called for SHT_NOBITS sections. Doing otherways breaks unit tests (for example ELF/edata-etext.s). The problem is that SHT_NOBITS section *may* be the very first section with VA set explicitly when linker script is used. For example:

SECTIONS
 {
   . = 0x10000000;
   .bss : { *(.bss) }
   .data : { *(.data) }
   . = 0x0F000000;
   .text : { *(.text) }
 }

In such case I see two ways to fix:

a) Explicitly check for script mode (as I'm doing currently)
b) If SHT_NOBITS section goes first call getFileAlignment() to calculate its offset

Hope this makes sense

[ruiu]

ELF/Writer.cpp

1134–1135

This is not your fault, but this code does not make sense. We want to fix only VAs in this function and let assignFileOffsets to fix file offsets. So please remove setFileOffset from this function.

1186

We don't have to handle linker scripts in a special manner here. Define the following helper in this function,

uintX_t Off = 0;

auto Set = [&](OutputSectionBase<ELFT> *Sec) {
  if (Sec->getType() == SHT_NOBITS) {
    Sec->setFileOffset(Off);
    return;
  }
  Off = getFileAlignment<ELFT>(Off, Sec);
  Sec->setFileOffset(Off);
  Off += Sec->getSize();
};

and use it as

Set(Out<ELFT>::ElfHeader);
Set(Out<ELFT>::ProgramHeaders);
for (OutputSectionBase<ELFT> *Sec : OutputSections)
  Set(Sec);

The above code should work both for both with and without linker scripts.

[evgeny777]

Review updated.

[evgeny777]

Rui, your proposed changes don't work correctly in case .bss (SHT_NOBITS) section is mentioned firrst in the linker script. I used this script:

SECTIONS {
   . = 0x10000000;
   .bss : { *(.bss) }
   .data : { *(.data) }
   . = 0x0F000000;
   .text : { *(.text) }
 }

with a very basic C program

This is what your patch produces (look at offset 0x2e0 and VA - they aren't equal modulo page size):

LOAD           0x000000 0x000000000efff000 0x000000000efff000 0x0002e0 0x0002e0 R   0x1000
LOAD           0x0002e0 0x0000000010000000 0x0000000010000000 0x000d90 0x000070 RW  0x1000

And this is correct result:

LOAD           0x000000 0x000000000efff000 0x000000000efff000 0x0002e0 0x0002e0 R   0x1000
LOAD           0x001000 0x0000000010000000 0x0000000010000000 0x000070 0x000070 RW  0x1000

[evgeny777]

May be add unit test for above?

[evgeny777]

Bug found - diff updated

[ruiu]

ELF/Writer.cpp

1188

Why don't you call this function unconditionally for all sections? Why only the first?

[evgeny777]

ELF/Writer.cpp

1188

This was a "hack" to make script case work whan .bss is first segment in program.
Using getFileAlignment for all sections will break following tests:

Failing Tests (5):
    lld :: ELF/edata-etext.s
    lld :: ELF/relocatable.s
    lld :: ELF/relocation-copy-flags.s
    lld :: ELF/tls-offset.s
    lld :: ELF/tls.s

For some reason SHT_NOBITS section alignments are not respected in assignFileOffsets.
Not sure why and whether this is a bug or feature. I suggest reverting the change above (2 lines)
and make a separate patch for this later

[ruiu]

I'd just update the tests.

[evgeny777]

Review updated.

It turned out that current implementation treats NOBITS sections correctly. In general file offsets for those sections should not be aligned, because they don't occupy any space in ELF.
What really should be done (IMHO) is aligning file offsets of PT_LOAD segments containing those sections in case file size of such segment is not zero. In case non-zero sized PT_LOAD segment file offset and VA are not properly aligned vm_mmap() kernel function returns -EINVAL and image is not loaded.

All lld test cases pass and linker scripts work fine as well

[ruiu]

Your patch contains space characters at end of lines. Please remove them.

ELF/Writer.cpp

1222–1227

I spent more than 20 minutes to understand this piece of code without success. Do you really need this?

test/ELF/linkerscript-phdr-check.s

17

Remove trailing newline.

[ruiu]

ELF/Writer.cpp

1222–1227

Your new test passed without this code, so I suspect you don't need it.

[evgeny777]

Removed the newly added code - will make a separate patch for the issue
Can this be committed?

[ruiu]

Is there an issue in the first place?

[ruiu]

Anyway, this change LGTM. Please commit after the following change.

ELF/LinkerScript.cpp

262

You have trailing whitespace in this line and other lines. Please remove them before committing.

[Hektve87]

In D21750#471235, @evgeny777 wrote:

Review updated.

It turned out that current implementation treats NOBITS sections correctly. In general file offsets for those sections should not be aligned, because they don't occupy any space in ELF.
What really should be done (IMHO) is aligning file offsets of PT_LOAD segments containing those sections in case file size of such segment is not zero. In case non-zero sized PT_LOAD segment file offset and VA are not properly aligned vm_mmap() kernel function returns -EINVAL and image is not loaded.

All lld test cases pass and linker scripts work fine as well

(~~~~)

ELF/LinkerScript.cpp

 
 #include "LinkerScript.h"
 #include "Config.h"
 #include "Driver.h"
 #include "InputSection.h"
 #include "OutputSections.h"
 #include "ScriptParser.h"
 #include "SymbolTable.h"
+#include "Target.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/ELF.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/StringSaver.h"
 
 using namespace llvm;
 using namespace llvm::ELF;
 using namespace llvm::object;
 using namespace lld;
 using namespace lld::elf;
 
 ScriptConfiguration *elf::ScriptConfig;
 
 static bool matchStr(StringRef S, StringRef T);
 
 // This is an operator-precedence parser to parse and evaluate
 // a linker script expression. For each linker script arithmetic
 // expression (e.g. ". = . + 0x1000"), a new instance of ExprParser
 // is created and ran.
 namespace {
void LinkerScript<ELFT>::assignAddresses(
   for (OutputSectionBase<ELFT> *Sec : Sections) {
     StringRef Name = Sec->getName();
     if (getSectionIndex(Name) == INT_MAX)
       Opt.Commands.push_back({SectionKind, {}, Name});
   }
 
   // Assign addresses as instructed by linker script SECTIONS sub-commands.
   Dot = Out<ELFT>::ElfHeader->getSize() + Out<ELFT>::ProgramHeaders->getSize();
+  uintX_t MinVA = std::numeric_limits<uintX_t>::max();
   uintX_t ThreadBssOffset = 0;
 
   for (SectionsCommand &Cmd : Opt.Commands) {
     if (Cmd.Kind == ExprKind) {
       Dot = evalExpr(Cmd.Expr, Dot);
       continue;
     }
 
     // Find all the sections with required name. There can be more than
     // ont section with such name, if the alignment, flags or type
     // attribute differs.
     for (OutputSectionBase<ELFT> *Sec : Sections) {
       if (Sec->getName() != Cmd.SectionName)
         continue;
 
       if ((Sec->getFlags() & SHF_TLS) && Sec->getType() == SHT_NOBITS) {
         uintX_t TVA = Dot + ThreadBssOffset;
         TVA = alignTo(TVA, Sec->getAlignment());
         Sec->setVA(TVA);
         ThreadBssOffset = TVA - Dot + Sec->getSize();
         continue;
       }
 
       if (Sec->getFlags() & SHF_ALLOC) {
         Dot = alignTo(Dot, Sec->getAlignment());
         Sec->setVA(Dot);
+        MinVA = std::min(MinVA, Dot);
         Dot += Sec->getSize();
         continue;
       }
     }
   }
+  
+  // ELF and Program headers need to be right before the first section in memory.
+  // Set their addresses accordingly.
+  MinVA = alignDown(MinVA - Out<ELFT>::ElfHeader->getSize() - 
+                    Out<ELFT>::ProgramHeaders->getSize(), Target->PageSize);
+  Out<ELFT>::ElfHeader->setVA(MinVA);
+  Out<ELFT>::ProgramHeaders->setVA(Out<ELFT>::ElfHeader->getSize() + MinVA);  
 }
 
 template <class ELFT>
 ArrayRef<uint8_t> LinkerScript<ELFT>::getFiller(StringRef Name) {
   auto I = Opt.Filler.find(Name);
   if (I == Opt.Filler.end())
     return {};
   return I->second;

ELF/Writer.cpp

private:
   void createSections();
   void addPredefinedSections();
   bool needsGot();
 
   void createPhdrs();
   void assignAddresses();
   void assignFileOffsets();
   void setPhdrs();
   void fixHeaders();
   void fixSectionAlignments();
   void fixAbsoluteSymbols();
   void openFile();
   void writeHeader();
   void writeSections();
   void writeBuildId();
   bool isDiscarded(InputSectionBase<ELFT> *IS) const;
   StringRef getOutputSectionName(InputSectionBase<ELFT> *S) const;
   bool needsInterpSection() const {
template <class ELFT> void Writer<ELFT>::fixSectionAlignments() {
   }
 }
 
 // We should set file offsets and VAs for elf header and program headers
 // sections. These are special, we do not include them into output sections
 // list, but have them to simplify the code.
 template <class ELFT> void Writer<ELFT>::fixHeaders() {
   uintX_t BaseVA = ScriptConfig->DoLayout ? 0 : Target->getVAStart();
   Out<ELFT>::ElfHeader->setVA(BaseVA);
-  Out<ELFT>::ElfHeader->setFileOffset(0);
   uintX_t Off = Out<ELFT>::ElfHeader->getSize();
   Out<ELFT>::ProgramHeaders->setVA(Off + BaseVA);
-  Out<ELFT>::ProgramHeaders->setFileOffset(Off);
 }
 
 // Assign VAs (addresses at run-time) to output sections.
 template <class ELFT> void Writer<ELFT>::assignAddresses() {
   uintX_t VA = Target->getVAStart() + Out<ELFT>::ElfHeader->getSize() +
                Out<ELFT>::ProgramHeaders->getSize();
 
   uintX_t ThreadBssOffset = 0;
static uintX_t getFileAlignment(uintX_t Off, OutputSectionBase<ELFT> *Sec) {
   // Relocatable output does not have program headers
   // and does not need any other offset adjusting.
   if (Config->Relocatable || !(Sec->getFlags() & SHF_ALLOC))
     return Off;
   return alignTo(Off, Target->PageSize, Sec->getVA());
 }
 
 // Assign file offsets to output sections.
 template <class ELFT> void Writer<ELFT>::assignFileOffsets() {
-  uintX_t Off =
-      Out<ELFT>::ElfHeader->getSize() + Out<ELFT>::ProgramHeaders->getSize();
+  uintX_t Off = 0;
 
-  for (OutputSectionBase<ELFT> *Sec : OutputSections) {
+  auto Set = [&](OutputSectionBase<ELFT> *Sec){
     if (Sec->getType() == SHT_NOBITS) {
       Sec->setFileOffset(Off);
-      continue;
+      return;
     }
-
     Off = getFileAlignment<ELFT>(Off, Sec);
     Sec->setFileOffset(Off);
     Off += Sec->getSize();
-  }
+  };
+
+  Set(Out<ELFT>::ElfHeader);
+  Set(Out<ELFT>::ProgramHeaders);
+  for (OutputSectionBase<ELFT> *Sec : OutputSections)
+    Set(Sec);
+ 
   SectionHeaderOff = alignTo(Off, sizeof(uintX_t));
   FileSize = SectionHeaderOff + (OutputSections.size() + 1) * sizeof(Elf_Shdr);
 }
 
 // Finalize the program headers. We call this function after we assign
 // file offsets and VAs to all sections.
 template <class ELFT> void Writer<ELFT>::setPhdrs() {
   for (Phdr &P : Phdrs) {
     Elf_Phdr &H = P.H;
     OutputSectionBase<ELFT> *First = P.First;
     OutputSectionBase<ELFT> *Last = P.Last;
     if (First) {
       H.p_filesz = Last->getFileOff() - First->getFileOff();
       if (Last->getType() != SHT_NOBITS)
         H.p_filesz += Last->getSize();
       H.p_memsz = Last->getVA() + Last->getSize() - First->getVA();
       H.p_offset = First->getFileOff();
       H.p_vaddr = First->getVA();
     }
     if (H.p_type == PT_LOAD)
       H.p_align = Target->PageSize;
     else if (H.p_type == PT_GNU_RELRO)
       H.p_align = 1;
     H.p_paddr = H.p_vaddr;
 
     // The TLS pointer goes after PT_TLS. At least glibc will align it,
     // so round up the size to make sure the offsets are correct.
     if (H.p_type == PT_TLS) {
       Out<ELFT>::TlsPhdr = &H;
       H.p_memsz = alignTo(H.p_memsz, H.p_align);
     }
   }
 }
 
 static uint32_t getMipsEFlags(bool Is64Bits) {

test/ELF/linkerscript-phdr-check.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+
+# RUN: echo "SECTIONS { . = 0x10000000; .text : {*(.text.*)} }" > %t.script
+# RUN: ld.lld -o %t1 --script %t.script %t
+# RUN: llvm-readobj -program-headers %t1 | FileCheck %s
+# CHECK:      ProgramHeaders [
+# CHECK-NEXT:  ProgramHeader {
+# CHECK-NEXT:    Type: PT_PHDR (0x6)
+# CHECK-NEXT:    Offset: 0x40
+# CHECK-NEXT:    VirtualAddress: 0xFFFF040
+
+.global _start
+_start:
+ nop