[AArch64] Support execute-only LOAD segments.

[ivanlozano]

Summary

This adds an LLD flag to mark executable LOAD segments execute-only for AArch64 targets.

In AArch64 the expectation is that code is execute-only compatible, so this just adds a linker option to enforce this.

Patch by: ivanlozano (Ivan Lozano)

[ivanlozano]

This is an alternative to the patch set D48791, D48793, D48794, D48795 for supporting execute-only segments in AArch64 targets, as per the discussion in D48791.

[peter.smith]

Adding Rui as code owner as he'll probably want to sign off on the introduction of a command line option.

I've got a couple of comments surrounding what happens in the linker script case. As it stands the change above will do the right thing with the default section order as all the executable sections will be in the same OutputSection and the ro-data will be separate. A hand written linker script could easily be written that could mix ro-data and code and wouldn't be caught by the Config->SingleRoRx check. What I suggest is to either not support the option when a linker script is used, or add a check in OutputSection::addSection() that can detect if ro-data and code is mixed in the same OutputSection. Other than that the implementation looks good to me.

[ruiu]

Is this for compatibility with an existing option, or is this a new option? If this is new, I think -aarch64 prefix might be undesirable -- even though "execute only" pages are currently supported by AArch64 among major ISAs, I can imagine that other ISA could support it. So a more platform-neutral naming might be better.

ELF/Driver.cpp

305 ↗

(On Diff #155983)

error messages shouldn't end with '.'

308 ↗

(On Diff #155983)

Ditto

ELF/Options.td

246–249 ↗

(On Diff #155983)

This file is sorted alphabetically, so this should be move all the way top to the file.

test/ELF/aarch64_execute_only.s

1 ↗

(On Diff #155983)

This test is perhaps a bit fragile because it depends on the current order of segments. How about this? You can specify the virtual address of the .text section by -Ttext=<value> and that can work as a marker.

I.e.

ld.lld -Ttext=0xcafe0000 %t.o -o %t.exe -aarch64-execute-only
llvm-readelf -l %t.exe | FileCheck %s

CHECK: LOAD  {{.*}} 0x00000000cafe0000 0x00000000cafe0000 E
CHECK-NOT: LOAD  {{.*}} 0x00000000cafe0000 0x00000000cafe0000 R E

[ruiu]

ELF/Writer.cpp

1771 ↗

(On Diff #155983)

You don't need to check Config->EMachine == EM_AARCH64 because it's already checked.

[ivanlozano]

In D49456#1166818, @ruiu wrote:

Is this for compatibility with an existing option, or is this a new option? If this is new, I think -aarch64 prefix might be undesirable -- even though "execute only" pages are currently supported by AArch64 among major ISAs, I can imagine that other ISA could support it. So a more platform-neutral naming might be better.

Yea this is for a new option; I'll rename it when I update the patch, thanks!

In D49456#1166366, @peter.smith wrote:

I've got a couple of comments surrounding what happens in the linker script case. As it stands the change above will do the right thing with the default section order as all the executable sections will be in the same OutputSection and the ro-data will be separate. A hand written linker script could easily be written that could mix ro-data and code and wouldn't be caught by the Config->SingleRoRx check. What I suggest is to either not support the option when a linker script is used, or add a check in OutputSection::addSection() that can detect if ro-data and code is mixed in the same OutputSection. Other than that the implementation looks good to me.

My first thought was to tackle this via OutputSection::addSection(), however on closer inspection SingleRoRx is always enabled in the ELF/ScriptParser.cpp function ScriptParser::readSections when reading a linker script that includes sections. Given that, it may be your first suggestion (disabling execute-only when using a linker script with sections) is the appropriate solution. (In practice this is already happening because computeFlags returns early on SingleRoRx, but it'll be good to explicitly set the execute-only flag to false in ScriptParser::readSections). I'll give this some more thought, but this might be the approach that I take.

[ivanlozano]

As mentioned in my last comment, I'm disabling the execute-only flag when linker scripts with sections defined are used. This also changes the proposed flag from "aarch64-execute-only" to just "execute-only" and makes the suggested modifications to the test.

[ruiu]

ELF/Options.td

135–136 ↗

(On Diff #157569)

Remove period from the end of the messages.

Please add "(default)" like other messages.

ELF/ScriptParser.cpp

491 ↗

(On Diff #157569)

I don't think silently disabling a security feature is a good idea. I'd just mark executable sections execute-only even if linker script is in use, because that's what user told to the linker to do. Other option would be to report an error when -execute-only and a linker script is used together, but I think that confuses users as to why that combination is not allowed.

test/ELF/execute-only.s

7 ↗

(On Diff #157569)

Remove excess space characters after "LOAD" so that it fits within 80 columns.

[ivanlozano]

ELF/ScriptParser.cpp

491 ↗

(On Diff #157569)

That's a good point, I agree we shouldn't fail silently here.

Leaving ExecuteOnly as-is here while setting SingleRoRx to true doesn't make sense to me; there'd be no RX segments to merge the RO segments into. Adding an error here if ExecuteOnly is true might be better. For the error message, something along the lines of "execute-only cannot be used with a linker script containing SECTIONS because this implies -no-rosegment". That be a bit confusing, but it does cover the reason.

[ruiu]

ELF/ScriptParser.cpp

491 ↗

(On Diff #157569)

You can still use linker scripts along with -execute-only if you carefully write a script so that the script is compatible with execute-only segment, no?

[ivanlozano]

ELF/ScriptParser.cpp

491 ↗

(On Diff #157569)

Chatted offline with ruiu to brainstorm a bit. To avoid complications with the script parser, he suggested a check in finalizeSections which tosses an error if we find non-SHF_EXECINSTR sections intermingled with SHF_EXECINSTR, which seems reasonable to me. I'll test this out and make sure it works as expected.

[ivanlozano]

Added a check in finalizeSections() for intermingled data and executable sections. Also moved the computeFlags() check for ExecuteOnly to occur before the SingleRoRx check.

[ruiu]

LGTM

Beautiful!

[ruiu]

Oh, but one more thing. Please add a test to verify that the error checking is actually working.

[ivanlozano]

Added the test to make sure that linker scripts are handled correctly. Also added a check against Script->HasSectionsCommand in checkOptions differentiate between Config->SingleRoRx being set via the command-line arg and being set via the script parser.

[ruiu]

LGTM

[ivanlozano]

If this looks good, could someone with commit access please land it? Thank you!

[xbolva00]

In D49456#1180339, @ivanlozano wrote:

If this looks good, could someone with commit access please land it? Thank you!

Thanks for patch!

lld/trunk/ELF/Config.h

struct Configuration {
   bool CompressDebugSections;
   bool Cref;
   bool DefineCommon;
   bool Demangle = true;
   bool DisableVerify;
   bool EhFrameHdr;
   bool EmitRelocs;
   bool EnableNewDtags;
+  bool ExecuteOnly;
   bool ExportDynamic;
   bool FixCortexA53Errata843419;
   bool GcSections;
   bool GdbIndex;
   bool GnuHash = false;
   bool GnuUnique;
   bool HasDynamicList = false;
   bool HasDynSymTab;

lld/trunk/ELF/Driver.cpp

if (Config->GcSections)
       error("-r and --gc-sections may not be used together");
     if (Config->GdbIndex)
       error("-r and --gdb-index may not be used together");
     if (Config->ICF != ICFLevel::None)
       error("-r and --icf may not be used together");
     if (Config->Pie)
       error("-r and -pie may not be used together");
   }
+
+  if (Config->ExecuteOnly) {
+    if (Config->EMachine != EM_AARCH64)
+      error("-execute-only is only supported on AArch64 targets");
+
+    if (Config->SingleRoRx && !Script->HasSectionsCommand)
+      error("-execute-only and -no-rosegment cannot be used together");
+  }
 }
 
 static const char *getReproduceOption(opt::InputArgList &Args) {
   if (auto *Arg = Args.getLastArg(OPT_reproduce))
     return Arg->getValue();
   return getenv("LLD_REPRODUCE");
 }
 
void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   Config->DwoDir = Args.getLastArgValue(OPT_plugin_opt_dwo_dir_eq);
   Config->DynamicLinker = getDynamicLinker(Args);
   Config->EhFrameHdr =
       Args.hasFlag(OPT_eh_frame_hdr, OPT_no_eh_frame_hdr, false);
   Config->EmitRelocs = Args.hasArg(OPT_emit_relocs);
   Config->EnableNewDtags =
       Args.hasFlag(OPT_enable_new_dtags, OPT_disable_new_dtags, true);
   Config->Entry = Args.getLastArgValue(OPT_entry);
+  Config->ExecuteOnly =
+      Args.hasFlag(OPT_execute_only, OPT_no_execute_only, false);
   Config->ExportDynamic =
       Args.hasFlag(OPT_export_dynamic, OPT_no_export_dynamic, false);
   Config->FilterList = args::getStrings(Args, OPT_filter);
   Config->Fini = Args.getLastArgValue(OPT_fini, "_fini");
   Config->FixCortexA53Errata843419 = Args.hasArg(OPT_fix_cortex_a53_843419);
   Config->GcSections = Args.hasFlag(OPT_gc_sections, OPT_no_gc_sections, false);
   Config->GnuUnique = Args.hasFlag(OPT_gnu_unique, OPT_no_gnu_unique, true);
   Config->GdbIndex = Args.hasFlag(OPT_gdb_index, OPT_no_gdb_index, false);

lld/trunk/ELF/Options.td

 defm error_limit:
   Eq<"error-limit", "Maximum number of errors to emit before stopping (0 = no limit)">;
 
 def error_unresolved_symbols: F<"error-unresolved-symbols">,
   HelpText<"Report unresolved symbols as errors">;
 
 defm exclude_libs: Eq<"exclude-libs", "Exclude static libraries from automatic export">;
 
+defm execute_only: B<"execute-only",
+    "Do not mark executable sections readable",
+    "Mark executable sections readable (default)">;
+
 defm export_dynamic: B<"export-dynamic",
     "Put symbols in the dynamic symbol table",
     "Do not put symbols in the dynamic symbol table (default)">;
 
 defm export_dynamic_symbol:
   Eq<"export-dynamic-symbol", "Put a symbol in the dynamic symbol table">;
 
 defm fatal_warnings: B<"fatal-warnings",

lld/trunk/ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::finalizeSections() {
   sortSections();
 
   // Now that we have the final list, create a list of all the
   // OutputSections for convenience.
   for (BaseCommand *Base : Script->SectionCommands)
     if (auto *Sec = dyn_cast<OutputSection>(Base))
       OutputSections.push_back(Sec);
 
+  // Ensure data sections are not mixed with executable sections when
+  // -execute-only is used.
+  if (Config->ExecuteOnly)
+    for (OutputSection *OS : OutputSections)
+      if (OS->Flags & SHF_EXECINSTR)
+        for (InputSection *IS : getInputSections(OS))
+          if (!(IS->Flags & SHF_EXECINSTR))
+            error("-execute-only does not support intermingling data and code");
+
   // Prefer command line supplied address over other constraints.
   for (OutputSection *Sec : OutputSections) {
     auto I = Config->SectionStartMap.find(Sec->Name);
     if (I != Config->SectionStartMap.end())
       Sec->AddrExpr = [=] { return I->second; };
   }
 
   // This is a bit of a hack. A value of 0 means undef, so we set it

 
 // Linker scripts are responsible for aligning addresses. Unfortunately, most
 // linker scripts are designed for creating two PT_LOADs only, one RX and one
 // RW. This means that there is no alignment in the RO to RX transition and we
 // cannot create a PT_LOAD there.
 static uint64_t computeFlags(uint64_t Flags) {
   if (Config->Omagic)
     return PF_R | PF_W | PF_X;
+  if (Config->ExecuteOnly && (Flags & PF_X))
+    return Flags & ~PF_R;
   if (Config->SingleRoRx && !(Flags & PF_W))
     return Flags | PF_X;
   return Flags;
 }
 
 // Decide which program headers to create and which sections to include in each
 // one.
 template <class ELFT> std::vector<PhdrEntry *> Writer<ELFT>::createPhdrs() {

lld/trunk/test/ELF/execute-only-mixed-data.s

+// REQUIRES: aarch64
+
+// RUN: llvm-mc -filetype=obj -triple=aarch64-linux-none %s -o %t.o
+
+// RUN: echo "SECTIONS \
+// RUN: { \
+// RUN:  .text : { *(.text) *(.rodata.foo) } \
+// RUN:  .rodata : { *(.rodata.bar) } \
+// RUN: }" > %t.lds
+// RUN: not ld.lld -T%t.lds %t.o -o %t -execute-only 2>&1 | FileCheck %s
+
+// RUN: echo "SECTIONS \
+// RUN: { \
+// RUN:  .text : { *(.text) } \
+// RUN:  .rodata : { *(.rodata.bar) *(.rodata.foo) } \
+// RUN: }" > %t.lds
+// RUN: ld.lld -T%t.lds %t.o -o %t -execute-only 2>&1
+
+// CHECK: -execute-only does not support intermingling data and code
+
+    br lr
+
+.section .rodata.foo
+.word 0x1
+.section .rodata.bar
+.word 0x2

lld/trunk/test/ELF/execute-only.s

+// REQUIRES: aarch64
+
+// RUN: llvm-mc -filetype=obj -triple=aarch64-linux-none %s -o %t.o
+// RUN: ld.lld -Ttext=0xcafe0000 %t.o -o %t.so -shared -execute-only
+// RUN: llvm-readelf -l %t.so | FileCheck %s
+
+// CHECK:      LOAD {{.*}} 0x00000000cafe0000 0x000004 0x000004   E 0x{{.*}}
+// CHECK-NOT:  LOAD {{.*}} 0x00000000cafe0000 0x000004 0x000004 R E 0x{{.*}}
+
+        br lr