This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Support execute-only LOAD segments.
ClosedPublic

Authored by ivanlozano on Jul 17 2018, 4:25 PM.

Details

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)

Diff Detail

Event Timeline

ivanlozano created this revision.Jul 17 2018, 4:25 PM

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.

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 added a comment.Jul 18 2018, 10:35 AM

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

error messages shouldn't end with '.'

308

Ditto

ELF/Options.td
250–253

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 added inline comments.Jul 18 2018, 10:39 AM
ELF/Writer.cpp
1771

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

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!

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.

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.

ivanlozano marked 4 inline comments as done.Jul 26 2018, 1:41 PM
ruiu added inline comments.Jul 26 2018, 2:23 PM
ELF/Options.td
135–136

Remove period from the end of the messages.

Please add "(default)" like other messages.

ELF/ScriptParser.cpp
491

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

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

ivanlozano added inline comments.Jul 26 2018, 3:07 PM
ELF/ScriptParser.cpp
491

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 added inline comments.Jul 26 2018, 3:13 PM
ELF/ScriptParser.cpp
491

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 added inline comments.Jul 26 2018, 4:19 PM
ELF/ScriptParser.cpp
491

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 marked 6 inline comments as done.

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 accepted this revision.Jul 27 2018, 11:29 AM

LGTM

Beautiful!

This revision is now accepted and ready to land.Jul 27 2018, 11:29 AM
ruiu added a comment.Jul 27 2018, 11:35 AM

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

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 accepted this revision.Jul 27 2018, 3:03 PM

LGTM

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

xbolva00 edited the summary of this revision. (Show Details)Jul 30 2018, 10:00 AM
This revision was automatically updated to reflect the committed changes.

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

Thanks for patch!