This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Place RW sections that go after relro to another memory page.
ClosedPublic

Authored by grimar on Dec 10 2015, 10:28 AM.

Details

Summary

Ian Lance Taylor writes:
"When the dynamic linker sees a PT_GNU_RELRO segment, it uses mprotect to mark the pages as read-only after the dynamic relocations have been applied. Of course this only works if the segment does in fact cover an entire page. The linker will try to force this to happen." (http://www.airs.com/blog/archives/189).

Before this patch sections that go after relro sequence were placed at the same memory page with relro ones. It caused segmentation fault on freebsd.
This should fix the https://llvm.org/bugs/show_bug.cgi?id=25790 (I have no freebsd to check)

Diff Detail

Event Timeline

grimar updated this revision to Diff 42444.Dec 10 2015, 10:28 AM
grimar retitled this revision from to [ELF] - Place RW sections that go after relro to another memory page..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: emaste, grimar, llvm-commits.

This should fix the https://llvm.org/bugs/show_bug.cgi?id=25790 (I have no freebsd to check)

Confirmed, this fixes PR25790.

(I'm curious what happened on Linux (glibc) though -- does it leave the page RW, effectively disabling (some of) relro?)

ruiu added inline comments.Dec 10 2015, 11:47 AM
ELF/Writer.cpp
845

What does this condition mean? (Particularly, why do you have to check for PF_W?)

847

What's the meaning of this flag?

910

Can you add new code here?

if (/* Some expression to check if the last section is in RELRO but the current one is not */) {
  VA = RoundUpToAlignment(VA, Target->getPageSize());
  FileOff = RoundUpToAlignment(FileOff, Target->getPageSize());
}

This should fix the https://llvm.org/bugs/show_bug.cgi?id=25790 (I have no freebsd to check)

Confirmed, this fixes PR25790.

(I'm curious what happened on Linux (glibc) though -- does it leave the page RW, effectively disabling (some of) relro?)

I am also very curious about that. There are no many choices, so i also think it disables it partially or fully, but I didn`t check.

grimar updated this revision to Diff 42507.Dec 11 2015, 12:45 AM
  • Review comments addressed.
  • Renamed GnuRelroPhdr to RelroPhdr and GnuRelroAligned to RelroAligned to pacify clang-format.
grimar updated this revision to Diff 42508.Dec 11 2015, 12:49 AM

Renamed GnuRelroPhdr to RelroPhdr in methods signatures (forgot to do that in prev diff).

grimar added inline comments.Dec 11 2015, 12:51 AM
ELF/Writer.cpp
845

That this condition checks if we already ended proccessing the relro sections or not.
If !Config->ZRelro then we are not anyways. If we are in a writable segment and isRelroSection then we are still processing.
updateRelro() also checks if we are still in a writable segment.
Thats just for additional safety I think. I guess using linkerscript we can place some of them to not writable segment for example (not sure for what), so its just checks that we are still doing what was supposed: preparing sections to be converted from W to R.

847

Flag has same use meaning as it used for in updateRelro(). If p_type is not set then PT_GNU_RELRO header was not "updated" yet. What means we didn`t start to proccess the relro sections sequence. If it is set and we are here then it we are somewhere after relro.

910

Done.

grimar updated this revision to Diff 43110.Dec 17 2015, 1:48 AM
  • Rebased.
ruiu added inline comments.Dec 18 2015, 10:29 PM
ELF/Writer.cpp
843–851

Can you rename this isFirstNonRelroSection? I think it conveys the meaning better.

847

Please add that as a comment.

// p_type is zero if we have not created RELRO PHDR yet.
889

Do you need this boolean variable? Because of the p_type check, I think isRelroEnded returns true only once.

grimar updated this revision to Diff 43357.Dec 21 2015, 4:17 AM

Review comments addressed.

grimar added inline comments.Dec 21 2015, 4:18 AM
ELF/Writer.cpp
843–851

Method unfortunately does not return is that is first non relro section or not.
"unfortunately" because it seems not to be possible to know here without additional flags outside.
Please see my comment for "bool RelroAligned = false;" below.
I renamed it to isPostRelroSection.

847

Done.

889

It will return true for each section after relro. Thats why I need that flag.
Lets look again on conditions:

if (!Config->ZRelro || ((Cur->p_flags & PF_W) && isRelroSection(Sec)))
  return false;
return RelroPhdr->p_type;

If current section is not relro or not in writable segment it will return p_type, which is here can be:

  1. zero - if we did not start proccessing the relro yet. So this flag was never set.
  2. non zero - if we had relro and finished with it.

So because of second I need to know if current section is the first section after relro or not, that is for what flag is there,

rafael edited edge metadata.Dec 22 2015, 8:42 PM
rafael added a subscriber: rafael.

I think this can be better expressed my moving the "is in relro" check
out of the method.

What do you think of the attached variation?

Cheers,
Rafael

What do you think of the attached variation?

Works for me. btw, updateRelro() in variation does not need Sec argument then.

template <class ELFT>
void Writer<ELFT>::updateRelro(Elf_Phdr *Cur, Elf_Phdr *GnuRelroPhdr,
                               OutputSectionBase<ELFT> *Sec, uintX_t VA) {
  if (!GnuRelroPhdr->p_type)
    setPhdr(GnuRelroPhdr, PT_GNU_RELRO, PF_R, Cur->p_offset, Cur->p_vaddr,
            VA - Cur->p_vaddr, 1 /*p_align*/);
  GnuRelroPhdr->p_filesz = VA - Cur->p_vaddr;
  GnuRelroPhdr->p_memsz = VA - Cur->p_vaddr;
}

Thanks!

I also refactored the code so that there is only one point where we do
page alignments.

Thanks!

I also refactored the code so that there is only one point where we do
page alignments.

Nice. Will you commit then ?

r256334. Thanks!

grimar accepted this revision.Dec 23 2015, 7:28 AM
grimar added a reviewer: grimar.

Accept since commited r256334.
Thanks to Rafael for improvements.

This revision is now accepted and ready to land.Dec 23 2015, 7:28 AM
grimar closed this revision.Dec 23 2015, 7:28 AM