This is an archive of the discontinued LLVM Phabricator instance.

[lld][ELF]Add option to make .dynamic read only
ClosedPublic

Authored by jakehehrlich on May 16 2017, 12:15 PM.

Details

Summary

The .dynamic section of an ELF almost doesn't need to be written to with the exception of the DT_DEBUG entry. For several reasons having a read only .dynamic section would be useful. This change adds the -z keyword "rodynamic" which forces .dynamic to be read-only. In this case DT_DEBUG will not be emited.

Diff Detail

Repository
rL LLVM

Event Timeline

jakehehrlich created this revision.May 16 2017, 12:15 PM
ruiu edited edge metadata.May 16 2017, 12:48 PM

I have a couple of questions.

  • Why can't .dynamic be read-only for all output?
  • .dynamic is already covered by PT_GNU_RELRO segments. Does it make sense to make it read-only from beginning?

It seems that you are proposing a new command line option that even GNU linkers don't have, so this patch needs extra attention. Are you adding that option to GNU linkers too?

ruiu added a comment.May 16 2017, 12:53 PM

On second thought, it isn't good to discuss new linker command line option in a code review thread, as most people do not read every review threads. Please start a thread in llvm-dev to propose a new option with an explanation why you want it.

emaste added a subscriber: emaste.May 16 2017, 2:27 PM

Does it make sense to make it read-only from beginning?

For me I think having .dynamic relro is sufficient, and might actually like to have MIPS use the conventional DT_DEBUG scheme.

pcc added a subscriber: pcc.May 16 2017, 2:49 PM

Does it make sense to make it read-only from beginning?

For me I think having .dynamic relro is sufficient, and might actually like to have MIPS use the conventional DT_DEBUG scheme.

Agreed. It seems that this scheme exists only for historical reasons: looking through binutils history, DT_MIPS_RLD_MAP was introduced in 1994 [0], while PT_GNU_RELRO was introduced 10 years later [1].

[0] https://github.com/bminor/binutils-gdb/commit/4c040a649b610ab1a0b5aa30c01148bffa9e2f4b
[1] https://github.com/bminor/binutils-gdb/commit/8c37241be3b1baf394090269b4b67babceb83d61

jakehehrlich edited the summary of this revision. (Show Details)
jakehehrlich added a reviewer: rafael.

After some discussion on llvm-dev it was decided that it would be best to not mess with DT_DEBUG_INDIRECT.

ruiu added a comment.May 23 2017, 3:55 PM

I'm not convinced that we need this, so please don't submit.

If this is for your OS's vDSO which needs to be read by an in-kernel ELF loader which doesn't support the entire ELF spec, this patch is to work around your loader's limitation. If you don't want to implement a complete ELF loader, why do you want to "fix" the problem in the right way? It is odd that you do hack at one place and do the right thing at the other place. Your loader can ignore RW bit and map .dynamic sections to read-only segments.

ruiu added inline comments.May 25 2017, 4:05 PM
ELF/SyntheticSections.cpp
1028–1029

... on MIPS and on Fuchsia OS which passes -z rodynamic.

1032

Probably it is better to not add this comment as it is not informative but could be confusing (e.g. why users may want it on other systems?)

1077–1078

Add a space after // and a full stop at end of the line.

But you want to extend the comment to explain the background. I'd write something like:

DT_DEBUG points to a location containing some debug info so that debuggers can find debug info at runtime. We need it for each process, so we don't write it for DSOs.

DT_DEBUG is the only .dynamic entry to which the loader is expected to write. Some systems, such as Fuchsia, chose to make .dynamic sections completely read-only (and provides alternative mean to find debug info). If the target is such system, we don't write DT_DEBUG.

I made the changes Rui recommended.

ruiu added inline comments.May 25 2017, 4:35 PM
ELF/Config.h
147

These variables are named after their corresponding command line options, so ZRodynamic.

ELF/SyntheticSections.cpp
1080

You want to break the paragraph by adding a blank // line.

1081–1082

Let's avoid writing comments too abstractly. "Some systems (currently only Fuchsia)" and instead of "Such systemS", "Fuchsia".

Fixed a few more things.

ruiu added inline comments.May 25 2017, 5:45 PM
ELF/Config.h
147–148

Sort.

ELF/Driver.cpp
690–691

Sort

ELF/SyntheticSections.cpp
1084

What I wanted is this

// blah blah blah ...
// blah blah blah ...
// 
// blah blah blah ...
// blah blah blah ...

and not

// blah blah blah ...
// blah blah blah ...
// blah blah blah ...
// blah blah blah ...
//

(Please look around to find a local convention and follow it.)

Sorry about these last fixes. I shouldn't have made those mistakes. Should be fixed.

ruiu added inline comments.May 25 2017, 6:20 PM
test/ELF/rodynamic.s
3

This doesn't test the feature you've just added, since -shared is passed. You want to remove that flag.

9–14

This checks the presence of .dynamic section, which is not what you actually want to do. You want to use llvm-readobj like this.

RUN: ld.lld -o %t1 %t.o
RUN: llvm-readobj -dynamic-table %t1 | FileCheck -check-prefix=DEFAULT %s
DEFAULT: DEBUG

RUN: ld.lld -o %t2 %t.o -z rodynamic
RUN: llvm-readobj -dynamic-table %t2 | FileCheck %s
CHECK-NOT: DEBUG

Right! I forgot to check that DEBUG wasn't being emitted in the test. I also forgot to check the default behavior. The other part of the test ensures that the .dynamic section is in fact read-only and that in the default case it is read-write. I had to produce a shared library to make the executable have a .dynamic section as well so I added a simple file to Inputs.

ruiu accepted this revision.May 25 2017, 7:22 PM

LGTM with these changes.

test/ELF/rodynamic.s
4

Break right here

8

and here to make it more comfortable to read. (It is a good practice to separate code blocks with blank lines so that you don't need to read an entire block of code without breathing.)

This revision is now accepted and ready to land.May 25 2017, 7:22 PM

Made the final fix.

This revision was automatically updated to reflect the committed changes.