rtld: Round down relro_size
ClosedPublic
Actions

Authored by kib on Aug 11 2021, 6:58 AM.

Details

Reviewers

markj
emaste
fbsd-phab_maskray.me

Commits

rGc9f833abf1d7: rtld: Round down relro_size

Summary

lld rounds up p_memsz(PT_GNU_RELRO) to satisfy common-page-size. If the page
size is smaller than common-page-size, rounding up relro_size may incorrectly
make some RW pages read-only.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

fbsd-phab_maskray.me created this revision.Aug 11 2021, 6:58 AM

Herald added subscribers: Contributor Reviews (src), bdragon, imp. · View Herald TranscriptAug 11 2021, 6:58 AM

fbsd-phab_maskray.me requested review of this revision.Aug 11 2021, 6:58 AM

Harbormaster completed remote builds in B40978: Diff 93523.Aug 11 2021, 6:58 AM

khng added reviewers: kib, markj.Aug 11 2021, 7:06 AM

(FreeBSD on non-FreeBSD isn't smooth. i cannot do a build test.)

Harbormaster completed remote builds in B40979: Diff 93524.Aug 11 2021, 7:12 AM

I do not understand this patch.

If linker specified that the end of relro is in the middle of some page, this means that there are relocations targeting that page. If we truncate relro_size, then relocation processing would operate on ro mapping and get SIGBUS/SIGSEGV.

Linker should not set relro_size that way. BTW, does bfd ld or gold produce something similar?

kib added a reviewer: emaste.Aug 11 2021, 8:08 AM

In D31498#710053, @kib wrote:

I do not understand this patch.

If linker specified that the end of relro is in the middle of some page, this means that there are relocations targeting that page. If we truncate relro_size, then relocation processing would operate on ro mapping and get SIGBUS/SIGSEGV.

For PT_GNU_RELRO, [p_vaddr,p_vaddr+p_memsz) specifies the address range which can be read-only. ld.so can shrink the range (rounddown), but not extend the range (roundup).

Extending the range can actually potentially cause SIGSEGV.

Linker should not set relro_size that way. BTW, does bfd ld or gold produce something similar?

GNU ld, gold, and ld.lld ensures p_vaddr+p_memsz is a multiple of common-page-size.
While max-page-size >= system the page size, common-page-size can be smaller than the system page size.

ld.lld uses a different (IMO better) layout (https://reviews.llvm.org/D58892) but the difference does not matter for this patch.

Handle main object same as the rest.
Minor style issue.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 13 2021, 9:59 AM

Closed by commit rGc9f833abf1d7: rtld: Round down relro_size (authored by kib). · Explain Why

This revision was automatically updated to reflect the committed changes.

kib added a commit: rGc9f833abf1d7: rtld: Round down relro_size.

In D31498#710007, @fbsd-phab_maskray.me wrote:

(FreeBSD on non-FreeBSD isn't smooth. i cannot do a build test.)

@fbsd-phab_maskray.me Could you post the error message somewhere? In general, cross-builds from linux+macOS should work, and I try to fix them after any breaking change.

In D31498#711395, @arichardson wrote:

In D31498#710007, @fbsd-phab_maskray.me wrote:

(FreeBSD on non-FreeBSD isn't smooth. i cannot do a build test.)

@fbsd-phab_maskray.me Could you post the error message somewhere? In general, cross-builds from linux+macOS should work, and I try to fix them after any breaking change.

The new behavior matches Linux glibc/musl.

I don't have a error message. If you want to test a breaking case for the old behavior, perhaps use -z common-page=1024 and then set the system page size larger than common-page-size.
I believe mprotect will have an incorrect length.

In D31498#711595, @fbsd-phab_maskray.me wrote:

In D31498#711395, @arichardson wrote:

In D31498#710007, @fbsd-phab_maskray.me wrote:

(FreeBSD on non-FreeBSD isn't smooth. i cannot do a build test.)

@fbsd-phab_maskray.me Could you post the error message somewhere? In general, cross-builds from linux+macOS should work, and I try to fix them after any breaking change.

The new behavior matches Linux glibc/musl.

I don't have a error message. If you want to test a breaking case for the old behavior, perhaps use -z common-page=1024 and then set the system page size larger than common-page-size.
I believe mprotect will have an incorrect length.