Page MenuHomeFreeBSD

imgact_elf: read program headers if not contained in the first page
ClosedPublic

Authored by kib on Thu, May 28, 9:26 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jun 16, 4:02 AM
Unknown Object (File)
Thu, Jun 11, 8:44 AM
Unknown Object (File)
Thu, Jun 11, 8:43 AM
Unknown Object (File)
Tue, Jun 9, 12:20 PM
Unknown Object (File)
Tue, Jun 9, 12:16 PM
Unknown Object (File)
Mon, Jun 8, 3:01 PM
Unknown Object (File)
Mon, Jun 8, 3:00 PM
Unknown Object (File)
Mon, Jun 8, 12:38 PM
Subscribers

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

kib requested review of this revision.Thu, May 28, 9:26 AM
sys/kern/imgact_elf.c
865

Do we also want some validation of e_phoff? I guess not since vn_rdwr() will return an error for short reads?

1033–1034

Should this use imgp->td?

1170

Why exactly do we need the new imgp->phdrs field? It looks like the local variable is sufficient.

kib marked 3 inline comments as done.Thu, May 28, 12:32 PM
kib added inline comments.
sys/kern/imgact_elf.c
865

For the same reason as why we use LK_RETRY. The 'real' VFS calls return errors if anything went wrong.
In fact, even if vn_rdwr() does not return an error but partially read the phdrs, there should be no harm: the rest of the code must be prepared to parse invalid phdrs.

But vn_rdwr() returns EIO if the read was truncated.

kib marked an inline comment as done.

Use local instead of imgp->phdrs. Remove __DECONST().
Remove all unneeded uses of curthread, replaced by imgp->td.

sys/kern/imgact_elf.c
865

the rest of the code must be prepared to parse invalid phdrs

I was more concerned about e_phoff itself: before, it was safe to assume that e_phoff < PAGE_SIZE, now it can be any 64-bit value. But in practice it is bounded by the file size, so probably this is not a real issue.

1228

e.g., is it possible for these calculations to overflow now?

2956

Isn't this assuming that the phdrs are in the first page?

kib marked 3 inline comments as done.Thu, May 28, 1:13 PM
kib added inline comments.
sys/kern/imgact_elf.c
865

I do not understand why would it be important. if e_phoff is arbitrary large, and file is big enough, what is the problem?

1228

It is possible, but it should not matter. First, to wrap around, I believe that the segment mapping must wrap. Second, the effect is only a wrong AT_PHDRS provided to userspace.

Nonetheless, I added the wrapping check at the beginning of the function.

kib marked 2 inline comments as done.

Pass phdrs to the note checker.
Check that AT_PHDRS calculation does not wrap.

sys/kern/imgact_elf.c
865

There is no problem, assuming that we check for overflow in the calculation hdr->e_phoff + hdr->e_phnum * hdr->e_phentsize.

1465

I think m_phdrs can be uninitialized here.

sys/kern/imgact_elf.c
865

Sorry, I see what you mean now, even if overflow happens, the only result is an invalid auxv entry.

kib marked 3 inline comments as done.

Initialize m_phdrs in both places.

This revision is now accepted and ready to land.Thu, May 28, 3:24 PM