Re: [PATCH] binfmt_elf: return the real error when program headers cannot be loaded

[Kees Cook <kees@kernel.org>]

On Wed, Oct 14, 2026 at 02:30:33PM +0200, Konstantin Khorenko wrote:
> Problem
> =======
> 
> load_elf_phdrs() returns NULL on any failure, and load_elf_binary()
> turns that into a fixed error: -ENOEXEC for the executable, -ELIBBAD
> for its interpreter. The real cause is lost:
> 
>   cause                                  really      execve() returns
>   -------------------------------------  ----------  ----------------
>   bad e_phentsize, e_phnum 0 or too big  bad file    ENOEXEC (right)
>   file ends before the header table      bad file    ENOEXEC (right)
>   kmalloc() of the table fails           ENOMEM      ENOEXEC (wrong)
>   kernel_read() fails                    EIO/ENOMEM  ENOEXEC (wrong)
> 
> For the interpreter every row gives ELIBBAD.
> 
> Consequences
> ============
> 
> ENOEXEC says the file is not an executable, which is a property of the
> file, not a transient failure:
> 
> - glibc's execvp() and execlp() take ENOEXEC as "not a binary" and run
>   the ELF file with /bin/sh, which reports a binary it cannot execute
>   instead of the ENOMEM or EIO.
> - search_binary_handler() tries the next binary format instead of
>   returning the error.

Isn't that what we want? If one handler can't load it, move on to find
one that does.

> - For the interpreter the caller gets ELIBBAD, "Accessing a corrupted
>   shared library", about an ld.so that is fine.
> - A caller that would retry on ENOMEM or report EIO gives up on the
>   file instead.
> 
> When it happens
> ===============
> 
> - The allocation fails under fault injection. Since commit
>   8030790477e8 ("binfmt_elf: remove the 4k limitation of program header
>   size") the table may be up to 64K; above 32K that is an order-4
>   allocation on 4K-page kernels, which can fail on a fragmented system.

I would not want to risk changing established userspace error message
interactions to make fault injection happy.

> - The read fails with an I/O error, or with -ENOMEM when the page cache
>   cannot take the pages.

This is kind of a bad failure case -- I don't think the "right" error
code is going to help much there. ;)

> 
> Where it came from
> ==================
> 
> - Until commit 6a8d38945cf4 ("binfmt_elf: Hoist ELF program header
>   loading to a function") load_elf_binary() returned -ENOMEM and the
>   read error. That commit documented an ERR_PTR return but returned
>   NULL; commit cfc46ca4fdca later fixed the comment, not the code.
> - Commit a9d9ef133f44 ("binfmt_elf: load interpreter program headers
>   earlier") gave the interpreter's headers the same NULL check, and with
>   it -ELIBBAD.

So it's been like this for 12 years? :)

> [...]
>   kernel                errors returned by execve()
>   7.3-rc7               ENOMEM 17, ENOEXEC 1, ELIBBAD 1, E2BIG 2
>   7.3-rc7 + this patch  ENOMEM 19, E2BIG 2

... so there are _fewer_ distinct failure codes with the patch?

> +	/*
> +	 * Read in the program headers. A read that fails part way returns
> +	 * what it got, so read on to see the error or the end of the file.
> +	 */
> +	pos = elf_ex->e_phoff;
> +	for (done = 0; done < size; done += rv) {
> +		rv = kernel_read(elf_file, (char *)elf_phdata + done,
> +				 size - done, &pos);
> +		if (rv <= 0) {
> +			kfree(elf_phdata);
> +			return ERR_PTR(rv ? rv : -ENOEXEC);
> +		}
>  	}

This is a behavioral change mixed into the errno handling change to
support partial reads??

I could be convinced that errnos need to be fixed, but the rationale
here doesn't really make me think it's worth potential disruption to
userspace. I'd need a pretty comprehensive survey of what would change
there and then a clean patch that made zero other behavior changes...

-Kees

-- 
Kees Cook