Page MenuHomePhabricator

ELF: Reserve space for copy relocations of read-only symbols in relro.
ClosedPublic

Authored by pcc on Jan 3 2017, 6:14 PM.

Details

Summary

When reserving copy relocation space for a shared symbol, scan the DSO's
program headers to see if the symbol is in a read-only segment. If so,
reserve space for that symbol in a new synthetic section named .bss.rel.ro
which will be covered by the relro program header.

This fixes the security issue disclosed on the binutils mailing list at:
https://sourceware.org/ml/libc-alpha/2016-12/msg00914.html

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 82988.Jan 3 2017, 6:14 PM
pcc retitled this revision from to ELF: Reserve space for copy relocations of read-only symbols in relro..
pcc updated this object.
pcc added reviewers: rafael, ruiu, davide.
pcc added subscribers: llvm-commits, tmsriram, eugenis.
emaste added a subscriber: emaste.Jan 3 2017, 7:04 PM
ruiu edited edge metadata.Jan 4 2017, 9:22 PM

Looks like ld.bfd and ld.gold have something similar already. Are they using the same section name, .bss.rel.ro?

lld/ELF/Relocations.cpp
419–426 ↗(On Diff #82988)

Can you factor the new code as a function taking a SharedSymbol and returning a boolean value?

pcc added a comment.Jan 5 2017, 12:39 PM
In D28272#636529, @ruiu wrote:

Looks like ld.bfd and ld.gold have something similar already. Are they using the same section name, .bss.rel.ro?

It looks like they are both using the name .data.rel.ro.

Relatedly, I discovered a bug in this patch. We can't use SHT_NOBITS for the new section because we need to lay out all NOBITS sections contiguously at the end of the LOAD. RELRO also needs to be contiguous (there can only be one RELRO header), so we can't have another RELRO covering part of the bss.

In principle, we could teach the linker to emit two r/w LOAD headers, one for RELRO and the other for non-RELRO, each with its own bss, but it's not clear that that would be worth it. So we might as well use SHT_PROGBITS and rename the section to .data.rel.ro to match the type. I'll upload a new patch that does that.

lld/ELF/Relocations.cpp
419–426 ↗(On Diff #82988)

Will do

pcc updated this revision to Diff 83332.Jan 5 2017, 5:41 PM
pcc marked an inline comment as done.
pcc edited edge metadata.
  • Change section ordering as follows: non-RELRO non-NOBITS, TLS non-NOBITS, TLS NOBITS, RELRO non-NOBITS, RELRO NOBITS, non-RELRO NOBITS
ruiu added a comment.Jan 5 2017, 7:03 PM

LGTM, but you may want to wait for a while to give Rafael to take another look.

lld/ELF/Symbols.h
297–300 ↗(On Diff #83332)

Do you have to inline this function? I think you added #include "OutputSections.h" for this function, but I don't want to add a new #include to this file to avoid cyclic inclusion.

This revision was automatically updated to reflect the committed changes.
pcc marked an inline comment as done.
pcc added a comment.Jan 9 2017, 5:34 PM

I committed the section ordering changes separately in r291523.