binfmt_elf_fdpic: reject PT_LOAD segments with p_filesz > p_memsz
Subsystems (3)
PATCH 1/1 binfmt_elf_fdpic: reject PT_LOAD segments with p_filesz > p_memsz
Both fdpic loaders assume every PT_LOAD segment satisfies p_filesz <= p_memsz, but this is never validated after the program headers are read.
A malformed ELF with p_filesz > p_memsz misbehaves in both paths:
- elf_fdpic_map_file_constdisp_on_uclinux() sizes its single anonymous mapping from p_memsz, then read_code()s p_filesz bytes into each segment, overrunning the reserved space.
- elf_fdpic_map_file_by_direct_mmap() computes
excess = p_memsz - p_filesz as an unsigned value, which underflows and drives an oversized anonymous mapping / clear_user().
Validate the invariant once in elf_fdpic_fetch_phdrs(), which is the common path for both the executable and the interpreter, so every loader is covered. Do it in a dedicated loop rather than the existing stack-size loop, which breaks early on PT_GNU_STACK and could skip later PT_LOAD segments.
---
Backstory:
When I got access to fable again I thought I'd let it recheck all of the nommu related files to see if there were any horrible bugs.
This is 1 of about 3 bugs that seem legit.
fable did most of the hard work here so I'm not going to claim to have found this bug. I wrote an initial patch and fable told me to put the check in a different place to cover another issue and rewrote the commit message. It knows best. :)
I have checked in my m68k nommu environment that this bug is valid and allows a bad ELF to corrupt memory using a reproducer fable helped create. Link is in the commit message. I also checked this fixes blocks a bad ELF from loading.
fs/binfmt_elf_fdpic.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
fs/binfmt_elf_fdpic.c +16 -0
@@ -157,6 +157,22 @@ static int elf_fdpic_fetch_phdrs(struct elf_fdpic_params *params,if (unlikely (retval != size))return retval <0 ? retval : -ENOEXEC ; +/* reject any PT_LOAD segment that claims to hold more data in the file +* than it reserves in memory; the loaders below assume +* p_filesz <= p_memsz and overrun/underflow their size arithmetic +* otherwise +*/ + phdr = params->phdrs; +for (loop =0 ; loop < params->hdr.e_phnum; loop++, phdr++) { +if (phdr->p_type !=PT_LOAD ) +continue ; + +if (phdr->p_filesz > phdr->p_memsz) { +kdebug ("Bad segment %d: p_filesz > p_memsz" , loop); +return -ENOEXEC ; + } + } +/* determine stack size for this binary */ phdr = params->phdrs;for (loop =0 ; loop < params->hdr.e_phnum; loop++, phdr++) {
Signature · 1 line
2.53.0