Skip to content

[ELF] --no-rosegment: don't mark read-only PT_LOAD segments executable #81223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 9, 2024
Merged

[ELF] --no-rosegment: don't mark read-only PT_LOAD segments executable #81223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 16 additions & 12 deletions lld/ELF/Writer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2353,17 +2353,12 @@ static bool needsPtLoad(OutputSection *sec) {
return true;
}

// 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.
// Adjust phdr flags according to certain options.
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;
}

Expand Down Expand Up @@ -2451,7 +2446,7 @@ SmallVector<PhdrEntry *, 0> Writer<ELFT>::createPhdrs(Partition &part) {
// Segments are contiguous memory regions that has the same attributes
// (e.g. executable or writable). There is one phdr for each segment.
// Therefore, we need to create a new phdr when the next section has
// different flags or is loaded at a discontiguous address or memory region
// compatible flags or is loaded at a discontiguous address or memory region
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean "create a new phdr when the next section has incompatible flags"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catch. Fixed the typo

// using AT or AT> linker script command, respectively.
//
// As an exception, we don't create a separate load segment for the ELF
Expand All @@ -2465,13 +2460,22 @@ SmallVector<PhdrEntry *, 0> Writer<ELFT>::createPhdrs(Partition &part) {
// so when hasSectionsCommand, since we cannot introduce the extra alignment
// needed to create a new LOAD)
uint64_t newFlags = computeFlags(sec->getPhdrFlags());
// When --no-rosegment is specified, RO and RX sections are compatible.
uint32_t diff = flags ^ newFlags;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be worth calling diff incompatible not quite as precise as diff, but it matches the earlier comment about incompatible flags. Works quite well with

if (incompatible)
  load = nullptr;

Not got a strong opinion, could go with either.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed

if (config->singleRoRx && !(newFlags & PF_W))
diff &= ~PF_X;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an interesting conflict here between singleRoRx and execute-only, could be worth making these options incompatible in Driver.cpp as singleRoRx breaks execute-only.

This isn't a regression caused by this patch though so could be done in a different patch.

Copy link
Member Author

@MaskRay MaskRay Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial --execute-only patch (https://reviews.llvm.org/D49456) checks for the incompatibility: it has an error --execute-only does not support intermingling data and code.

It also has this error "--execute-only and -no-rosegment cannot be used together", which is untested. (The !Script->HasSectionsCommand condition is redundant as hasSectionsCommand is not set in Driver.cpp yet)

if (diff)
load = nullptr;

bool sameLMARegion =
load && !sec->lmaExpr && sec->lmaRegion == load->firstSec->lmaRegion;
if (!(load && newFlags == flags && sec != relroEnd &&
sec->memRegion == load->firstSec->memRegion &&
(sameLMARegion || load->lastSec == Out::programHeaders) &&
(script->hasSectionsCommand || sec->type == SHT_NOBITS ||
load->lastSec->type != SHT_NOBITS))) {
if (load && sec != relroEnd &&
sec->memRegion == load->firstSec->memRegion &&
(sameLMARegion || load->lastSec == Out::programHeaders) &&
(script->hasSectionsCommand || sec->type == SHT_NOBITS ||
load->lastSec->type != SHT_NOBITS)) {
load->p_flags |= newFlags;
} else {
load = addHdr(PT_LOAD, newFlags);
flags = newFlags;
}
Expand Down
2 changes: 1 addition & 1 deletion lld/test/ELF/segments.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
# NOROSEGMENT1-NEXT: LOAD 0x001006 0x0000000000000006 0x0000000000000006 0x000001 0x000001 RW 0x1000
# NOROSEGMENT1-NEXT: LOAD 0x001007 0x0000000000000007 0x0000000000000007 0x000002 0x000002 R E 0x1000
# NOROSEGMENT1-NEXT: LOAD 0x001009 0x0000000000000009 0x0000000000000009 0x000001 0x000001 RW 0x1000
# NOROSEGMENT1-NEXT: LOAD 0x00100a 0x000000000000000a 0x000000000000000a 0x000001 0x000001 R E 0x1000
# NOROSEGMENT1-NEXT: LOAD 0x00100a 0x000000000000000a 0x000000000000000a 0x000001 0x000001 R 0x1000
# NOROSEGMENT1-NEXT: GNU_STACK 0x000000 0x0000000000000000 0x0000000000000000 0x000000 0x000000 RW 0

# RUN: ld.lld -N a.o -o omagic
Expand Down