[PATCH] D64906: [ELF][PPC] Allow PT_LOAD to have overlapping p_offset ranges

Mon Oct 21 18:20:25 PDT 2019

MaskRay added a comment.

In D64906#1717096 <https://reviews.llvm.org/D64906#1717096>, @troyj wrote:

> > Can you be more specific about how this conflicts with the blog post?
>
> The blog post says "Note that the current dynamic linker code will only work correctly if the PT_GNU_RELRO segment starts on a page boundary. This is because the dynamic linker rounds the p_vaddr field down to the previous page boundary."  The lld code comment says "musl/glibc ld.so rounds the size down" and then proceeds to round the size up in what appears to be a countermeasure.  So the blog post is talking about the starting address of the segment, but the lld code is rounding the size, not the starting address.  For programs that I have linked, the starting address appears to already be on a page boundary, so no rounding is required there, and apply any rounding to the size results in an error because too much of the RW data segment gets marked RO.

It is best not to interpret this as a doctrine. musl does `mprotect(laddr(p, p->relro_start), p->relro_end-p->relro_start, PROT_READ)` where `relro_end` is computed as `(ph->p_vaddr + ph->p_memsz) & -PAGE_SIZE` (glibc is similar). The changed formula matches how musl/glibc mprotect PT_GNU_RELRO. FreeBSD rounds the size up. I have said it can be problematic in one of my previous comments and reiterated in my previous comment to your question.

"PT_GNU_RELRO segment starts on a page boundary" does not preclude the possibility that p_vaddr%maxpagesize!=0.

>> Out of curiosity I want to learn why your software needs a local patch. I am 90% certain that that specific software makes unfounded assumption about the section/segment layout.
> 
> Yes, it makes assumptions that may be unfounded but happen to match ld.bfd and ld.gold behavior.  Specifically, it assumes that the linker creates a single RW segment like ld.bfd and ld.gold, and that the starting address of that RW segment matches the starting address of the RELRO segment.  I'm not aware of any rule that ld.lld violates by creating more than one RW segment, but it is inconvenient that it does not match the behavior of the other linkers.  My local patch stops splitting the RW segment so that there is only one segment and then removes the rounding of the RELRO size.  IF I leave the rounding in place, then the entire RW segment ends up being covered by the RELRO, which is bad because then the program can't write to any of its data.  With the rounding removed, the emitted layout is much closer to the one emitted by ld.bfd and ld.gold.

Your software may need `-z separate-loadable-segments`, as I mentioned in my previous comments to your question. We still have 2 RW, but they don't have overlapping p_offset, and they can actually be merged into one. Put it in another way, after mprotect'ing PT_GNU_RELRO, the memory mapping layout is not different from the case when there is only one RW. If your program does not parse its program header, there should be no runtime perceivable behavior differences. Merging 2 RW into one can be seen as an optimization, and lld does not support it. I think it is fine because a program header just costs sizeof(Elf64_Phdr)=56 bytes on ELF64.

>> The last page may be unprotected.
> 
> The last page being unprotected is better than incorrectly making the first page of writable data be RELRO.  The former may miss identifying some programming errors or possibly open a security hole, but the latter certainly leads to the program crashing.
> 
> A better way might be to nudge the start of the writable data to begin later in the RW segment so that an integral number of initial pages can be RELRO, but ld.lld doesn't seem to do that and I'm not familiar with the code enough to add that myself.  Hence, I'm settling for possibly not protecting all of the RO data until I know of a way to do the above.

The current layout is `R RX RW(RELRO) RW(non-RELRO)`. Android folks proposed an alternative layout in August https://lists.llvm.org/pipermail/llvm-dev/2019-August/134801.html You can find David Chisnall's and my replies. I am not convinced it improves things.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64906/new/

https://reviews.llvm.org/D64906