rtld: more caution when parsing notes in digest_notes()
ClosedPublic
Actions

Authored by kib on Sun, Sep 13, 10:17 AM.

Details

Reviewers

emaste
jrtc27

Commits

rGfa848d4d0c03: rtld: more caution when parsing in digest_notes()

Summary

Incorrect ELF might have PT_NOTE slightly larger than the needed to
contain all notes, and the PT_NOTE size could be larger than one page.
Then rtld mmaps just the notes bytes to parse.  After the last note,
we iterate past the mapped region trying to read the Elf_Note header.
This was found in wild.

Require full elf note to fit into the [start_note, end_note) region to
continue the parsing.  Check it in stages, first verifying the Elf_Note
header structure fits, to be able to read the name and data length.
After that, check the whole note against limit.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kib created this revision.Sun, Sep 13, 10:17 AM

Herald added subscribers: bdragon, imp. · View Herald TranscriptSun, Sep 13, 10:17 AM

kib requested review of this revision.Sun, Sep 13, 10:17 AM

Require whole note to fit into the mapped region, not only the header.

kib added reviewers: emaste, jrtc27.Sun, Sep 13, 10:55 AM

kib added a subscriber: makc.

I can confirm that this patch fixes the problem for me. Tested on stable/15-n285266-30211e66b989

emaste accepted this revision.Mon, Sep 14, 11:55 AM

emaste added inline comments.

libexec/rtld-elf/rtld.c
1792	I personally find `if` statements that omit one of the components a bit confusing. No worries if you prefer it as is..

This revision is now accepted and ready to land.Mon, Sep 14, 11:55 AM

kib marked an inline comment as done.Mon, Sep 14, 9:11 PM

kib added inline comments.

libexec/rtld-elf/rtld.c
1792	Perhaps you mean 'for'. But style is explicit about `for (;;) {}` e.g. Yes, I think this is fine there.

Less weird check for the boundary. Update the commit message.

This revision now requires review to proceed.Mon, Sep 14, 9:12 PM

This revision was not accepted when it landed; it landed in state Needs Review.Mon, Sep 14, 9:55 PM

Closed by commit rGfa848d4d0c03: rtld: more caution when parsing in digest_notes() (authored by kib). · Explain Why

This revision was automatically updated to reflect the committed changes.

kib added a commit: rGfa848d4d0c03: rtld: more caution when parsing in digest_notes().

Can we have errata once the fix is merged to the stable branches? I'm currently pending update of Plasma ports. In principle I could exclude offending plugin from the package and push the update, but this seems dubious to me.

How did a plugin end up with a broken ELF note, and why are we shipping it?

Perhaps you mean 'for'. But style is explicit about for (;;) {} e.g. Yes, I think this is fine there.

Err, yes I meant for, not sure how I made that mistake. We do specify for (;;) for forever loops, but don't really speak to for loops that omit some but not all components. But either way, doesn't really matter it's fine with me either way.