Skip to contentSkip to conversation

binfmt_elf_fdpic: reject PT_LOAD segments with p_filesz > p_memsz

PATCH New Daniel Palmer 1 participant 1 message Jul 10

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

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.

Assisted-by Claude:claude-fable-5 # Discovery of bug, review/rework of patch, reproducer
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")

---
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
index 7e3108489c83..69aa0e04543b 100644
@@ -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