Closed Bug 635961 Opened 13 years ago Closed 7 years ago

elfhack doesn't save as much when there is a GNU_RELRO segment

Categories

(Firefox Build System :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: espindola, Assigned: glandium)

References

Details

Attachments

(7 files)

Glandium asked me to report this since he was expecting bigger gains than what I found in

http://blog.mozilla.com/respindola/2011/02/22/lto/
Assignee: nobody → mh+mozilla
So, the problem is that there is a GNU_RELRO segment, and that when the elfhack injected code is called, some sections are is already readonly, so elfhack can't apply relocations on them, therefore doesn't pack them in the first place.

Once elfhack injected code will be able to call functions, it will be able to mprotect() itself, which would allow around GNU_RELRO.
Summary: elfhack not as big win as expected → elfhack doesn't save as much when there is a GNU_RELRO segment
Depends on: 638398
Blocks: 1359912
Nathan, please review the patch when you're back.
Flags: needinfo?(nfroyd)
Blocks: 1378986
Comment on attachment 8885019 [details]
Bug 635961 - Allow elfhack to relocate data under the GNU_RELRO segment.

https://reviewboard.mozilla.org/r/155842/#review165734

This is pretty gnarly, but probably not *that* much more gnarly than the rest of elfhack.

::: build/unix/elfhack/elfhack.cpp:685
(Diff revision 2)
> +    // If there is a relro segment, our injected code will run after the linker sets the
> +    // corresponding pages read-only. We need to make our code change that to read-write
> +    // before applying relocations, which means it needs to call mprotect.
> +    // To do that, we need to either find a reference to the mprotect symbol. In case the
> +    // library already has one, we use that, but otherwise, we add the symbol.
> +    // Then the injected code needs to be able to call the corresponding function, which
> +    // means it needs access to a pointer to it. We get such a pointer by making the linker
> +    // apply a relocation for the symbol at an address our code can read.
> +    // The problem here is that there is not much relocated space where we can put such a
> +    // pointer, so we abuse the bss section temporarily (it will be restored to a null
> +    // value before any code can actually use it)

Just...wow.

::: build/unix/elfhack/elfhack.cpp:723
(Diff revision 2)
> +
> +        // Add a relocation for the mprotect symbol.
> +        new_rels.emplace_back();
> +        Rel_Type &rel = new_rels.back();
> +        memset(&rel, 0, sizeof(rel));
> +        rel.r_info = ELF32_R_INFO(std::distance(symtab->syms.begin(), std::vector<Elf_SymValue>::iterator(mprotect)), rel_type2);

Oof.  It's not clear to me that you can just blindly do `iterator(mprotect)` for non-pointer implementations of `vector::iterator`, either.  I don't know what a good solution to the latter is, though.  Maybe you could just do `distance(&symtab->syms[0], mprotect)`; I think that is guaranteed to work.

::: build/unix/elfhack/elfhack.cpp:725
(Diff revision 2)
> +        // Find the beginning of the bss section, and use an aligned location in there
> +        // for the relocation.
> +        for (ElfSegment *segment = elf->getSegmentByType(PT_LOAD); segment;
> +             segment = elf->getSegmentByType(PT_LOAD, segment)) {

The comments here say you're looking for the bss section, but the code says you're looking at segments, which are not the same thing.  What prevents you from returning addresses in the PT_LOAD segment for `.text`/`.rodata`/etc. rather than the PT_LOAD segment where `.bss` lives?

::: build/unix/elfhack/elfxx.h:332
(Diff revision 2)
>      SectionInfo getInfo() { return info; }
>  
>      void shrink(unsigned int newsize) {
> -        if (newsize < shdr.sh_size)
> +        if (newsize < shdr.sh_size) {
>              shdr.sh_size = newsize;
> +            markDirty();

Kind of surprised that not doing this didn't bite us before.

::: build/unix/elfhack/inject.c:73
(Diff revision 2)
> +
> +static inline __attribute__((always_inline))
> +void relro_post()
> +{
> +    mprotect_cb(relro_start, relro_end - relro_start, PROT_READ);
> +    // mprotect_cb is a pointer allocator in .bss, so we need to restore it to

Should this read "...a pointer allocated in .bss..." instead?
(In reply to Mike Hommey [:glandium] from comment #10)
> Nathan, please review the patch when you're back.

Done!
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #13)
> Comment on attachment 8885019 [details]
> Bug 635961 - Allow elfhack to relocate data under the GNU_RELRO segment.
> 
> https://reviewboard.mozilla.org/r/155842/#review165734
> 
> This is pretty gnarly, but probably not *that* much more gnarly than the
> rest of elfhack.
> 
> ::: build/unix/elfhack/elfhack.cpp:685
> (Diff revision 2)
> > +    // If there is a relro segment, our injected code will run after the linker sets the
> > +    // corresponding pages read-only. We need to make our code change that to read-write
> > +    // before applying relocations, which means it needs to call mprotect.
> > +    // To do that, we need to either find a reference to the mprotect symbol. In case the
> > +    // library already has one, we use that, but otherwise, we add the symbol.
> > +    // Then the injected code needs to be able to call the corresponding function, which
> > +    // means it needs access to a pointer to it. We get such a pointer by making the linker
> > +    // apply a relocation for the symbol at an address our code can read.
> > +    // The problem here is that there is not much relocated space where we can put such a
> > +    // pointer, so we abuse the bss section temporarily (it will be restored to a null
> > +    // value before any code can actually use it)
> 
> Just...wow.
> 
> ::: build/unix/elfhack/elfhack.cpp:723
> (Diff revision 2)
> > +
> > +        // Add a relocation for the mprotect symbol.
> > +        new_rels.emplace_back();
> > +        Rel_Type &rel = new_rels.back();
> > +        memset(&rel, 0, sizeof(rel));
> > +        rel.r_info = ELF32_R_INFO(std::distance(symtab->syms.begin(), std::vector<Elf_SymValue>::iterator(mprotect)), rel_type2);
> 
> Oof.  It's not clear to me that you can just blindly do `iterator(mprotect)`
> for non-pointer implementations of `vector::iterator`, either.  I don't know
> what a good solution to the latter is, though.  Maybe you could just do
> `distance(&symtab->syms[0], mprotect)`; I think that is guaranteed to work.

error: no matching function for call to ‘distance(std::vector<Elf_SymValue>::iterator, Elf_SymValue*&)’

> ::: build/unix/elfhack/elfhack.cpp:725
> (Diff revision 2)
> > +        // Find the beginning of the bss section, and use an aligned location in there
> > +        // for the relocation.
> > +        for (ElfSegment *segment = elf->getSegmentByType(PT_LOAD); segment;
> > +             segment = elf->getSegmentByType(PT_LOAD, segment)) {
> 
> The comments here say you're looking for the bss section, but the code says
> you're looking at segments, which are not the same thing.

In a pedantic way, they're not, but in practice, they are: when the file size and the mem size don't match, you have a .bss-like section underneath. Makes me think this code should also check the write flag.

>  What prevents you
> from returning addresses in the PT_LOAD segment for `.text`/`.rodata`/etc.
> rather than the PT_LOAD segment where `.bss` lives?

.text would require tweaking the .dynamic section to add the marker for text relocations. Which in turn would require resizing it, which is hard. Plus, the linker we use on android actively rejects text relocations.

.rodata is usually in a ro PT_LOAD.

There is e.g. .relro or some other sections, but those already have relocations of their own.

Really, there's not much place that is safe to use. Another possibility would be to grow the mem size of the PT_LOAD and stick the pointer there.

> ::: build/unix/elfhack/elfxx.h:332
> (Diff revision 2)
> >      SectionInfo getInfo() { return info; }
> >  
> >      void shrink(unsigned int newsize) {
> > -        if (newsize < shdr.sh_size)
> > +        if (newsize < shdr.sh_size) {
> >              shdr.sh_size = newsize;
> > +            markDirty();
> 
> Kind of surprised that not doing this didn't bite us before.

Turns out we weren't touching things that required it to happen before writing out the hacked ELF.

> ::: build/unix/elfhack/inject.c:73
> (Diff revision 2)
> > +
> > +static inline __attribute__((always_inline))
> > +void relro_post()
> > +{
> > +    mprotect_cb(relro_start, relro_end - relro_start, PROT_READ);
> > +    // mprotect_cb is a pointer allocator in .bss, so we need to restore it to
> 
> Should this read "...a pointer allocated in .bss..." instead?

Indeed.
Flags: needinfo?(nfroyd)
(In reply to Mike Hommey [:glandium] from comment #15)
> (In reply to Nathan Froyd [:froydnj] from comment #13)
> > ::: build/unix/elfhack/elfhack.cpp:723
> > (Diff revision 2)
> > > +
> > > +        // Add a relocation for the mprotect symbol.
> > > +        new_rels.emplace_back();
> > > +        Rel_Type &rel = new_rels.back();
> > > +        memset(&rel, 0, sizeof(rel));
> > > +        rel.r_info = ELF32_R_INFO(std::distance(symtab->syms.begin(), std::vector<Elf_SymValue>::iterator(mprotect)), rel_type2);
> > 
> > Oof.  It's not clear to me that you can just blindly do `iterator(mprotect)`
> > for non-pointer implementations of `vector::iterator`, either.  I don't know
> > what a good solution to the latter is, though.  Maybe you could just do
> > `distance(&symtab->syms[0], mprotect)`; I think that is guaranteed to work.
> 
> error: no matching function for call to
> ‘distance(std::vector<Elf_SymValue>::iterator, Elf_SymValue*&)’

Weeeeeeird.  We could just do mprotect - &symtab->syms[0], then?

I guess we could just go with what's there, though, since we're really dependent on running this on Linux anyway...

> > ::: build/unix/elfhack/elfhack.cpp:725
> > (Diff revision 2)
> > > +        // Find the beginning of the bss section, and use an aligned location in there
> > > +        // for the relocation.
> > > +        for (ElfSegment *segment = elf->getSegmentByType(PT_LOAD); segment;
> > > +             segment = elf->getSegmentByType(PT_LOAD, segment)) {
> > 
> > The comments here say you're looking for the bss section, but the code says
> > you're looking at segments, which are not the same thing.
> 
> In a pedantic way, they're not, but in practice, they are: when the file
> size and the mem size don't match, you have a .bss-like section underneath.
> Makes me think this code should also check the write flag.

Checking the write flag would definitely make this code more obvious, please do that.

> >  What prevents you
> > from returning addresses in the PT_LOAD segment for `.text`/`.rodata`/etc.
> > rather than the PT_LOAD segment where `.bss` lives?

Given your reply, I see that my question was unclear.  It should have been "what in the loop prevents the loop from selecting the PT_LOAD section for .text etc. rather than the one for .bss" or something like that.  Checking the write flag, as mentioned above, would help.

> .text would require tweaking the .dynamic section to add the marker for text
> relocations. Which in turn would require resizing it, which is hard. Plus,
> the linker we use on android actively rejects text relocations.
> 
> .rodata is usually in a ro PT_LOAD.
> 
> There is e.g. .relro or some other sections, but those already have
> relocations of their own.
> 
> Really, there's not much place that is safe to use.

Yeah, I agree.  .bss is probably the best place to do this...
Flags: needinfo?(nfroyd)
Comment on attachment 8885027 [details]
Bug 635961 - Don't disable elfhack when the linker creates GNU_RELRO segments.

https://reviewboard.mozilla.org/r/155856/#review166454
Attachment #8885027 - Flags: review?(nfroyd) → review+
Comment on attachment 8885019 [details]
Bug 635961 - Allow elfhack to relocate data under the GNU_RELRO segment.

https://reviewboard.mozilla.org/r/155842/#review166456

::: build/unix/elfhack/inject.c:73
(Diff revision 3)
> +    // mprotect_cb is a pointer allocator in .bss, so we need to restore it to
> +    // a NULL value.

Still need to fix "...is a pointer allocator...".
Attachment #8885019 - Flags: review?(nfroyd) → review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/ddda63d5366e
Allow elfhack to relocate data under the GNU_RELRO segment. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/c56fa9c1eda0
Don't disable elfhack when the linker creates GNU_RELRO segments. r=froydnj
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6af0ca47efab
Backed out 2 changesets at developer's request a=backout
Comment on attachment 8885019 [details]
Bug 635961 - Allow elfhack to relocate data under the GNU_RELRO segment.

There was a stupid typo, and some missing part of growing sections...
Attachment #8885019 - Flags: review+ → review?(nfroyd)
Comment on attachment 8885019 [details]
Bug 635961 - Allow elfhack to relocate data under the GNU_RELRO segment.

https://reviewboard.mozilla.org/r/155842/#review166842
Attachment #8885019 - Flags: review?(nfroyd) → review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/1dad535d187a
Allow elfhack to relocate data under the GNU_RELRO segment. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/e68cb2040ee3
Don't disable elfhack when the linker creates GNU_RELRO segments. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/1dad535d187a
https://hg.mozilla.org/mozilla-central/rev/e68cb2040ee3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Blocks: 1385117
Minor improvement on Android .apk size from this:

== Change summary for alert #8406 (as of July 28 2017 13:54 UTC) ==

Improvements:

  0%  installer size summary android-4-0-armv7-api15 opt      36,395,751.75 -> 36,323,338.58

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8406
Depends on: 1385703
Depends on: 1389805
Product: Core → Firefox Build System
Depends on: 1470701
Depends on: 1499915
No longer depends on: 1499915
You need to log in before you can comment on or make changes to this bug.