[ELF] - Fill executable segments with trap instructions.

[grimar]

Summary

LLD currently fills the last page of each executable segment with
trap instructions. It seems to be not enough, see PR36853.

Patch changes logic to fill the all paddings within a segment.
So that any zero areas which exist because of alignment
paddings are also covered.

[ruiu]

This change is a bit worrisome from the performance point of view because it significantly increases the number of bytes we write to mmap'ed file. I believe this is still fine because writing bytes to contiguous memory is extremely fast and the size of the executable part of an executable is relatively small compared to the size of an entire executable, but we need to verify that first.

[ruiu]

When I tried this patch against an internal program of size 2 GiB, the overhead almost 2% (i.e. the link time is 2% longer than before), which is I think not negligible. We need a better way of filling executable segments with trap instructions.

[grimar]

In D44775#1046138, @ruiu wrote:

When I tried this patch against an internal program of size 2 GiB, the overhead almost 2% (i.e. the link time is 2% longer than before), which is I think not negligible. We need a better way of filling executable segments with trap instructions.

Thanks for the information. Indeed I did not expect 2% slowdown. I'll try to implement and benchmark a different approach.

[grimar]

• Reimplemented.

[grimar]

• Included forgotten test case.

[ruiu]

ELF/Arch/X86.cpp

445 ↗

(On Diff #139589)

These instructions won't change, so let's just append 0xcc instructions at end of each array, instead of using memset.

[grimar]

• Addressed review comment.

[ruiu]

ELF/Arch/X86.cpp

446 ↗

(On Diff #139710)

Please remove ; .align 48 because that doesn't really explain what this code does. (Also we want to align it not to 48 but 16).

466 ↗

(On Diff #139710)

Ditto

ELF/Arch/X86_64.cpp

503–504 ↗

(On Diff #139710)

Do this

0xcc, 0xcc, ... // int3
0xcc, 0xcc, ... // int3

so that the comments are aligned vertically.

552 ↗

(On Diff #139710)

Ditto

[grimar]

ELF/Arch/X86.cpp

446 ↗

(On Diff #139710)

Actually, we want to align to PltHeaderSize I think, which is 48 here. Because if you align 32 to 16, you'll get
32 instead of 48 and that would not work.

[grimar]

Abandoning in favour of D44943

[ruiu]

ELF/Arch/X86.cpp

446 ↗

(On Diff #139710)

That's wrong. ".align 48" doesn't mean that the size of this thingy is 48 byte. It just says that the thin that follows this thing is aligned to 48 bytes. So that comment is wrong.

[ruiu]

You shouldn't have dropped this patch because mine doesn't include the code to add 0xcc to end of each PLT entry. We still need that for PLT entries that's larger than one page.

[• espindola]

ELF/Writer.cpp

2195

Why not static?

2212

I wonder if we could write this as we write the sections.

[grimar]

In D44775#1050890, @ruiu wrote:

You shouldn't have dropped this patch because mine doesn't include the code to add 0xcc to end of each PLT entry. We still need that for PLT entries that's larger than one page.

I think we have D44682 for PLT entries posted earlier than this one on review and it do all necessary job for PLT entries + has the test case.

[grimar]

ELF/Arch/X86.cpp

446 ↗

(On Diff #139710)

Ok, seems I overthinking the meaning of "0xcc, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc, 0xcc, // 19: int3; .align 16" above then,
thanks for the explanation.

ELF/Writer.cpp

if (Sec->Flags & SHF_ALLOC)
       Sec->writeTo<ELFT>(Buf + Sec->Offset);
 }
 
 static void fillTrap(uint8_t *I, uint8_t *End) {
   for (; I + 4 <= End; I += 4)
     memcpy(I, &Target->TrapInstr, 4);
 }
 
-// Fill the last page of executable segments with trap instructions
-// instead of leaving them as zero. Even though it is not required by any
-// standard, it is in general a good thing to do for security reasons.
-//
-// We'll leave other pages in segments as-is because the rest will be
-// overwritten by output sections.
+// Fill the executable segments with trap instructions instead of leaving them
+// as zero. Even though it is not required by any standard, it is in general a
+// good thing to do for security reasons. We have to fill all pages to properly
+// fill paddings created during applying alignment to sections.
 template <class ELFT> void Writer<ELFT>::writeTrapInstr() {
   if (Script->HasSectionsCommand)
     return;
 
-  // Fill the last page.
+  // Fill executable segments with trap instructions.
   uint8_t *Buf = Buffer->getBufferStart();
   for (PhdrEntry *P : Phdrs)
     if (P->p_type == PT_LOAD && (P->p_flags & PF_X))
-      fillTrap(Buf + alignDown(P->p_offset + P->p_filesz, Target->PageSize),
+      fillTrap(Buf + P->p_offset,
                Buf + alignTo(P->p_offset + P->p_filesz, Target->PageSize));
 
   // Round up the file size of the last segment to the page boundary iff it is
   // an executable segment to ensure that other tools don't accidentally
   // trim the instruction padding (e.g. when stripping the file).
   PhdrEntry *Last = nullptr;
   for (PhdrEntry *P : Phdrs)
     if (P->p_type == PT_LOAD)
       Last = P;
 
   if (Last && (Last->p_flags & PF_X))
     Last->p_memsz = Last->p_filesz = alignTo(Last->p_filesz, Target->PageSize);
 }
 
 // Write section contents to a mmap'ed file.
 template <class ELFT> void Writer<ELFT>::writeSections() {

test/ELF/fill-trap2.s

+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+# RUN: ld.lld %t -o %t2
+# RUN: llvm-readobj -program-headers %t2 | FileCheck %s
+# RUN: od -Ax -t x1 -N16 -j0x1000 %t2 | FileCheck %s -check-prefix=FILL
+
+# CHECK: ProgramHeader {
+# CHECK:   Type: PT_LOAD
+# CHECK:   Offset: 0x1000
+# CHECK-NEXT:   VirtualAddress:
+# CHECK-NEXT:   PhysicalAddress:
+# CHECK-NEXT:   FileSize: 8192
+# CHECK-NEXT:   MemSize:
+# CHECK-NEXT:   Flags [
+# CHECK-NEXT:     PF_R
+# CHECK-NEXT:     PF_X
+# CHECK-NEXT:   ]
+
+# FILL: 001000 90 cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc
+
+nop
+
+.section .foo,"ax"
+.align 16
+nop
+.zero 0x1000