Page MenuHomePhabricator

[ELF] Merge .openbsd.randomdata.* sections into a single .openbsd.randomdata section when linking
Needs RevisionPublic

Authored by brad on Sep 6 2020, 8:45 PM.

Details

Summary

Merge .openbsd.randomdata.* sections into a single .openbsd.randomdata section when linking.

Diff Detail

Event Timeline

brad created this revision.Sep 6 2020, 8:45 PM
brad requested review of this revision.Sep 6 2020, 8:45 PM
MaskRay added a comment.EditedSep 7 2020, 8:35 AM

Can you describe the purpose of the section and why it needs to be suffixed? If .openbsd.randomdata. is only used in very few places, writing a linker script is a more appropriate solution.

This patch needs a test case in section-name.s if it is justified.

jrtc27 added a comment.EditedSep 7 2020, 9:14 AM

Isn't it useless without emitting a PT_OPENBSD_RANDOMIZE to cover it? (reading the somewhat terse https://github.com/openbsd/src/blob/master/libexec/ld.so/SPECS.randomdata)

MaskRay requested changes to this revision.Sep 19 2020, 8:58 PM
This revision now requires changes to proceed.Sep 19 2020, 8:58 PM
brad added a comment.Sep 22 2020, 5:17 PM

I spoke to the author of the diff and asked for some feedback. He said...

Picking a random userland .o file from the build: readelf -a shows that it
contain multiple randomdata sections:

.openbsd.randomdata.retguard.130
.openbsd.randomdata.retguard.236
.openbsd.randomdata.retguard.2936
.openbsd.randomdata.retguard.443
.openbsd.randomdata.retguard.2600

The suffix here is a hash generated by the compiler. This is something
done by default by the compiler on openbsd, so if we were to instead use a
linker script for this then (almost) *EVERY* program built on openbsd
would require a linker script.

As for "isn't this useless without putting them in PT_OPENBSD_RANDOMIZE
segment", well, sure, so add the four chunks under gnu/llvm which define &
mention that symbol...but I think some may argue that doing that should
only be done if the binary's 'OS' field in the header is the openbsd
value, so that may create a different kind of pushback.

grimar added a subscriber: grimar.Sep 23 2020, 3:12 AM

Well, if the default compiler on openbsd does it, I think the change is reasonable. @MaskRay?
This patch, as was mentioned, needs a test case.

Also, I believe LLD already creates the PT_OPENBSD_RANDOMIZE segment and adds the .openbsd.randomdata
section to it already in Writer.cpp:

// PT_OPENBSD_RANDOMIZE is an OpenBSD-specific feature. That makes
// the dynamic linker fill the segment with random data.
if (OutputSection *cmd = findSection(".openbsd.randomdata", partNo))
  addHdr(PT_OPENBSD_RANDOMIZE, cmd->getPhdrFlags())->add(cmd);
brad added a comment.Oct 12 2020, 1:07 PM

I'm not really good with coming up with tests, but especially with lld. Any suggestions? Any further comments?

I'm not really good with coming up with tests, but especially with lld. Any suggestions? Any further comments?

I think you need to update lld\test\ELF\section-name.s.