[PATCH] D40029: [ELF] Only add contiguous isRelro sections to PT_GNU_RELRO PHDR

[Rui Ueyama via llvm-commits]

On Thu, Nov 16, 2017 at 8:40 PM, Peter Smith via Phabricator <
reviews at reviews.llvm.org> wrote:

> peter.smith added a comment.
>
> In https://reviews.llvm.org/D40029#927209, @ruiu wrote:
>
> > > Hmmm, I think that approach will be difficult for a couple of reasons.
> The first is that I think it will be difficult to get all the existing
> dynamic loaders to change unless we could think of an important use-case
> that can't be solved by just a single segment. I think that if I were a
> dynamic loader author I'd say "Why do you want multiple non-contiguous
> RELRO segments in an ELF file designed for an OS with a standard image
> layout? Why do I need to add complexity for an edge case?". The second is
> that there is a large lag time between getting a patch accepted in glibc
> and it getting deployed, for LTS releases we could be looking at several
> years before loaders that could support multiple RELRO segments.
> >
> > I don't know if it is actually a problem. In this patch, you are
> attempting to add a RELRO segment only for the first contiguous regions
> that should be RELRO. My understanding is that the existing loader works OK
> with multiple RELRO segments, but only the first RELRO segment takes
> effect. That means, if you create multiple RELRO segments, that effectively
> works the same as creating a RELRO for the first contiguous region, no? If
> that's the case, creating multiple RELRO should fix the crash bug that you
> are trying to fix.
> >
> > By doing that, we can buy time to fix dynamic loaders. As a result, the
> situation will be (1) if you are using an old loader, your binary still
> work, but the second and further RELRO segments are ignored, and (2) if you
> are using a newer loader, all RELRO segments are processed.
> >
> > The above was my plan. What am I missing?
>
>
> The only thing that I think that you are missing is that with at least the
> current state of glibc, there is at least one case where the PT_GNU_RELRO
> PHDR is searched for from the end of the table, the comment alongside the
> code says, the PT_GNU_RELRO PHDR is usually last. So if we output more than
> one we can't guarantee that the first PHDR will be selected, this could be
> dangerous.
>

I see. Thank you for explaining. But, how much dangerous is it to use only
the last RELRO compared to only the first one?

I wonder if it makes sense to just reverse the RELRO segments.

Anyway, I'm also interested in hearing from pcc.

My suggestion is to:
>
> - Change lld to use .data.rel.ro instead of .bss.rel.ro; this will sort
> out the non-user error case.
> - I would prefer to emit a warning and only output one PT_GNU_RELRO
> header, although I concede that putting out as many PT_GNU_RELRO header as
> we need is valid response to a linker script containing in effect a
> user-error, however I think we should only do this until we've made the .
> bss.rel.ro change.
>
>
> https://reviews.llvm.org/D40029
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171116/c9097a97/attachment.html>