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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

In https://reviews.llvm.org/D40029#927020, @ruiu wrote:

> I think that the right thing to do is to create a RELRO segment for each contiguous RELRO sections and then update the loader so that it can handle multiple RELROs. What do you think?


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.

Looking at the specific problem again, and coming across https://reviews.llvm.org/D28272 "Reserve space for copy relocations of read-only symbols in relro" what I think has happened here is:

- LLD uses a section InX::BssRelRo with name .bss.rel.ro to contain copy relocations of read-only symbols, if the relro sections are not matched in the linker script these get placed in a single contiguous chunk by sortSections().
- ld.bfd and gold use a section called .data.rel.ro so .bss.rel.ro doesn't appear on the default ld linker script and unfortunately the .bss.* matches our .bss.rel.ro so we get non-contiguous relro.
- In comment https://reviews.llvm.org/D28272#637284 there is a comment

> 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.

It isn't clear to me that the follow up patch landed, if it did we wouldn't have had this problem as .data.rel.ro would have matched in the linker script. Given how difficult it will be to update all the loaders to support multiple RELRO headers I think that the best course of action is:

- Work out what happened to the follow up patch for https://reviews.llvm.org/D28272, and get it applied. This would make the default ld linker script work out of the box.
- Warn or error if multiple non-contiguous relro is encountered. At least on the glibc loader the PT_GNU_RELRO is searched for in more than one direction so outputting more than one could cause crashes or security leaks.

I'll add the author of https://reviews.llvm.org/D28272 as a reviewer.


https://reviews.llvm.org/D40029