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

[Konstantin Khorenko <khorenko@virtuozzo.com>]

On 10/15/26 06:48, Kees Cook wrote:
> 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 a malformed file, yes, and the patch keeps -ENOEXEC for every
malformed case. For a failed allocation or read it does not help: the
file has already matched the ELF magic, so the other handlers decline
it too, and the loop ends with -ENOEXEC anyway. Moving on only loses
the error.


>> - 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.

Fault injection is just how I found it, not why it matters.

The case that matters is execvp().
glibc runs a file with /bin/sh when execve() fails with ENOEXEC.
That exec of the shell succeeds, so the caller's process image is gone,
and the shell then exits with "cannot execute binary file".

For example, QEMU's cpr-exec live update does exactly this:
it re-execs itself with execvp(), and on an error it resumes the VM.
If reading the program headers fails with EIO, or with ENOMEM from
the page cache, the guest is killed by /bin/sh instead of resuming.


>> - 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. ;)

It helps execvp() callers: with EIO or ENOMEM they get the error back
and can retry or report it. With ENOEXEC they never get the error,
because the shell has replaced them.

>>
>> 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? :)

Yes, since v3.19. :)

It needs an allocation or read failure at exactly this point to show,
which is why it went unnoticed.
So yes, the chances are low, but the price might be a killed VM.

>> [...]
>>   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?

Well, this part was not clear enough, sorry for that.

The truth is those runs i did injected only allocation failures, so every
failure is ENOMEM.
The ENOEXEC and ELIBBAD in the first row were allocation failures reported
as a bad file.
(E2BIG comes from copy_strings(); that is a separate issue, left alone.)


On the userspace side: the patch adds no errno that execve() cannot
return today. The same file is read in two other places on the same
exec, and both pass the kernel_read() error through:

- prepare_binprm() reads the first BINPRM_BUF_SIZE bytes of the binary
  and returns the kernel_read() result as it is.
- elf_read() reads the PT_INTERP path and the interpreter's ELF header.
  It returns the kernel_read() error as it is, and -EIO for a short
  read.

load_elf_phdrs() itself reads the table with elf_read() too, then drops
whatever it returned and reports NULL. So a read error on the program
header table is the only read error of an ELF exec that reaches
userspace as ENOEXEC. The patch makes it behave like the reads around
it.

>> +	/*
>> +	 * 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??

Yes, I missed this point, thank you for pointing it out!

Yes, previously in case we get a partial read we consider the exec failed
with no other options and now (with the patch), we try to read again and
the new read may succeed or whatever - yes, it's a behavioral change.

My idea was: in case we get a partial read, we still don't know the
reason - the error code (which will be returned only on the next read),
so read once again, get that error code and return IT to the userspace.

But definitely it's a more complex thing here and it should not go along
with this patch. If we decide - I can create another patch for that,
or we can just drop this hunk at all.


> 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...

Understood, will try my best.
I can create a v2 which will change only the errno, for two cases:
 - the kmalloc() of the table fails: -ENOMEM instead of -ENOEXEC;
 - kernel_read() returns an error: that error instead of -ENOEXEC.

Short reads case keep -ENOEXEC.

Sounds good?

--
Cheers, Konstantin.