Fill the last page of each executable section with 0xcc or equivalent.

[ruiu]

Summary

I think this patch is better than https://reviews.llvm.org/D44775.

[grimar]

lld/ELF/Writer.cpp

2210

Does it really make sense to optimize here? If so, then probably my patch was better,
since it writes fewer bytes and is more straightforward.
If no (and I expect it is the case), then I would remove the check to simplify the code,
writing few more pages should not be slow.

2212

I do not think it is the correct assertion.
memset has int value argument, though the description is:
`The value is passed as an int, but the function fills the block of memory
using the unsigned char conversion of this value.`

So I think what you want to check here that all bytes of TrapInstr are the same.
Btw, that is true for all targets atm. What about landing D44979 then and removing the assert?

[ruiu]

lld/ELF/Writer.cpp

2210

Writing to a single page is extremely fast, so no. And your patch is not straightforward because the code is more complicated. The point of this patch is that you don't need to iterate over Phdrs. You should just iterate over OutputSections.

[ruiu]

• do not use memset

[• espindola]

lld/ELF/Writer.cpp

2211

Iterating over OutputSections seems like a good idea, but couldn't this fill just the area between two sections?

[ruiu]

lld/ELF/Writer.cpp

2211

We can. The reason why I'm doing this is because this way, trap instructions are always 4 byte aligned even if the last segment is not 4 byte aligned. But I can fix it some way.

[• espindola]

lld/ELF/Writer.cpp

2211

Interesting. We have the same latent bug in OutputSection::writeTo. I guess we have to add nop for ppc (where not all bytes are the same) to test a fix.

[grimar]

LGTM + nit/suggestion.

lld/ELF/Writer.cpp

2209

So, should this check be removed for simplicity of code?
(basing on comments, it makes no sense to have it, right?)

Then, btw, code could be:

for (OutputSection *Sec : OutputSections)
  if (Sec->PtLoad && Sec->PtLoad == PT_LOAD && (Sec->PtLoad->p_flags & PF_X))
    fillTrap(Buf + alignDown(Sec->Offset + Sec->Size, Target->PageSize));

[ruiu]

lld/ELF/Writer.cpp

2209

No it can't. If a section ends exactly at a page boundary, no padding is needed.

2211

I thought about how to fill only gaps between sections, but there seems no easy way of doing it. It's doable , but it's perhaps not worth writing more code for it. So I think I'd prefer the current code.

[grimar]

lld/ELF/Writer.cpp

2209

Ah, I see now, it would fill the next page with traps without this check.

[aaronpuchert]

This has been accepted for quite some time, any reason why we're not landing it?

The bug is now #36201.

lld/ELF/Writer.cpp

 
 template <class ELFT> void Writer<ELFT>::writeSectionsBinary() {
   uint8_t *Buf = Buffer->getBufferStart();
   for (OutputSection *Sec : OutputSections)
     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);
+static void fillTrap(uint8_t *Buf) {
+  assert(sizeof(Target->TrapInstr) == 4);
+
+  auto *P = reinterpret_cast<uint32_t *>(Buf);
+  for (size_t I = 0; I < Target->PageSize / 4; ++I)
+    *P++ = Target->TrapInstr;
 }
 
-// Fill the last page of executable segments with trap instructions
+// Fill the last pages of executable sections 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.
 template <class ELFT> void Writer<ELFT>::writeTrapInstr() {
   if (Script->HasSectionsCommand)
     return;
 
-  // Fill the last page.
+  // Fill the last page of each executable section.
   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),
-               Buf + alignTo(P->p_offset + P->p_filesz, Target->PageSize));
+  for (OutputSection *Sec : OutputSections) {
+    PhdrEntry *P = Sec->PtLoad;
+    if (!P || P->p_type != PT_LOAD || !(P->p_flags & PF_X))
+      continue;
+    if ((Sec->Offset + Sec->Size) % Target->PageSize == 0)
+      continue;
+    fillTrap(Buf + alignDown(Sec->Offset + Sec->Size, 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;

lld/test/ELF/fill-trap.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 -x -N16 -j0x1ff0 %t2 | FileCheck %s -check-prefix=FILL
+# RUN: od -Ax -t x1 -N16 -j0x1000 %t2 | FileCheck %s -check-prefix=FILL
 
-# CHECK: ProgramHeader {
-# CHECK:   Type: PT_LOAD
-# CHECK:   Offset: 0x1000
+# CHECK:      ProgramHeader {
+# CHECK:        Type: PT_LOAD
+# CHECK:        Offset: 0x1000
 # CHECK-NEXT:   VirtualAddress:
 # CHECK-NEXT:   PhysicalAddress:
-# CHECK-NEXT:   FileSize: 4096
+# CHECK-NEXT:   FileSize: 8192
 # CHECK-NEXT:   MemSize:
 # CHECK-NEXT:   Flags [
 # CHECK-NEXT:     PF_R
 # CHECK-NEXT:     PF_X
 # CHECK-NEXT:   ]
 
-## Check that executable page is filled with traps at its end.
-# FILL: 001ff0 cccc cccc cccc cccc cccc cccc cccc cccc
+# FILL: 001000 90 cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc
 
-.globl _start
-_start:
-  nop
+
+.section .foo,"ax"
+.align 16
+nop
+.zero 0x1000