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

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

[Konstantin Khorenko]

load_elf_phdrs() returns NULL on any failure, and load_elf_binary()
turns that into -ENOEXEC for the executable and -ELIBBAD for its
interpreter. So a failed allocation or read of the program headers
makes glibc's execvp() run the ELF file with /bin/sh, and reports a
healthy ld.so as a corrupted shared library.

Return an ERR_PTR instead, as binfmt_elf_fdpic does:

  -ENOEXEC     bad e_phentsize, e_phnum or e_phoff, or the file ends
               before the header table
  -ENOMEM      the allocation fails
  read error   kernel_read() fails

Malformed files keep getting -ENOEXEC and -ELIBBAD. kernel_read()
returns a short count when a later page fails, so read on until the
error or the end of the file shows. An e_phoff past LLONG_MAX - size
is rejected up front, as rw_verify_area() would turn it into -EINVAL.

Fixes: 6a8d38945cf4 ("binfmt_elf: Hoist ELF program header loading to a function")
Fixes: a9d9ef133f44 ("binfmt_elf: load interpreter program headers earlier")
Assisted-by: LLM
Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
---
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.
- 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.
- The read fails with an I/O error, or with -ENOMEM when the page cache
  cannot take the pages.

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.

Hence the two Fixes tags.

Testing
=======

With failslab on v7.3-rc7 (CONFIG_FAILSLAB,
CONFIG_FAULT_INJECTION_DEBUG_FS, failslab/ignore-gfp-wait = N): write n
to /proc/thread-self/fail-nth, then execvp("/bin/true"). At the n that
hits the program header allocation of /bin/true, /bin/sh (bash here)
gets the file:

    /bin/true: /bin/true: cannot execute binary file    (exit 126)

while the allocations around it give "execvp: Cannot allocate memory".

The reproducer below does the same with execve() for n = 1, 2, ...
until 50 execve() calls in a row return no error, on /bin/true
(dynamically linked):

  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

- ENOEXEC is the executable's header array, ELIBBAD the interpreter's.
- The two E2BIG come from copy_string_kernel() of the file name, whose
  first write to the new stack fails in get_arg_page(); that is a
  separate issue, not touched here.
- failslab does not reach header tables above 8K, which go to the page
  allocator, nor the read errors, so these rows show only the
  allocation half.
- For the read: a copy of /bin/true with its header table moved across
  offset 4096, served from a FUSE file with direct_io whose reads stop
  at 4096 and fail with EIO past it. execve() returns EIO with the
  patch; the old code turns the first, short read into ENOEXEC.
- A truncated header table, and an e_phoff of 2^63 that kernel_read()
  would refuse with EINVAL, still give ENOEXEC for the executable and
  ELIBBAD for the interpreter.

Reproducer (gcc -O2 -o exec_fail_nth exec_fail_nth.c; run as root):

    // SPDX-License-Identifier: GPL-2.0
    /*
     * Fail the n-th fault injection point (here: slab allocations) of an
     * execve() for n = 1, 2, ... until 50 execve() calls in a row return no
     * error, and count the errors execve() returns. Needs CONFIG_FAILSLAB and
     * CONFIG_FAULT_INJECTION_DEBUG_FS, and failslab set up to fail sleeping
     * allocations:
     *	echo N > /sys/kernel/debug/failslab/ignore-gfp-wait
     *
     * usage: exec_fail_nth [program]	(default /bin/true)
     */
    #define _GNU_SOURCE
    #include <errno.h>
    #include <fcntl.h>
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    #include <unistd.h>
    #include <sys/wait.h>

    int main(int argc, char **argv)
    {
    	char *prog = argc > 1 ? argv[1] : "/bin/true";
    	char *args[] = { prog, NULL };
    	int counts[256] = { 0 }, clean = 0, n, e;

    	for (n = 1; clean < 50 && n < 100000; n++) {
    		int p[2];
    		pid_t pid;

    		if (pipe2(p, O_CLOEXEC))
    			return 2;
    		pid = fork();
    		if (pid < 0)
    			return 2;
    		if (!pid) {
    			char buf[16];
    			int fd = open("/proc/thread-self/fail-nth", O_WRONLY);

    			if (fd < 0 || write(fd, buf, sprintf(buf, "%d", n)) < 0)
    				_exit(3);
    			execve(prog, args, NULL);
    			e = errno;
    			write(p[1], &e, sizeof(e));
    			_exit(4);
    		}
    		close(p[1]);
    		if (read(p[0], &e, sizeof(e)) == sizeof(e)) {
    			counts[e & 255]++;
    			if (e == ENOEXEC || e == ELIBBAD)
    				printf("n=%d: execve() failed with %s\n", n, strerrorname_np(e));
    			clean = 0;
    		} else {
    			clean++;
    		}
    		close(p[0]);
    		if (waitpid(pid, &e, 0) == pid && WIFEXITED(e) && WEXITSTATUS(e) == 3) {
    			fprintf(stderr, "cannot write /proc/thread-self/fail-nth\n");
    			return 2;
    		}
    	}
    	printf("n = 1..%d, execve() errors:", n - 1);
    	for (e = 0; e < 256; e++)
    		if (counts[e])
    			printf(" %s=%d", strerrorname_np(e), counts[e]);
    	printf("\n");
    	if (n - 1 == clean) {
    		fprintf(stderr, "no allocation failed: is failslab/ignore-gfp-wait N?\n");
    		return 2;
    	}
    	return counts[ENOEXEC] || counts[ELIBBAD];
    }
---
 fs/binfmt_elf.c | 50 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 06d0df1053823..05e03f1c9f3f6 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -512,39 +512,47 @@ static unsigned long maximum_alignment(struct elf_phdr *cmds, int nr)
  *
  * Loads ELF program headers from the binary file elf_file, which has the ELF
  * header pointed to by elf_ex, into a newly allocated array. The caller is
- * responsible for freeing the allocated data. Returns NULL upon failure.
+ * responsible for freeing the allocated data. Returns an ERR_PTR upon failure:
+ * -ENOEXEC for a malformed header or when the read reaches the end of the
+ * file, the allocation or read error otherwise.
  */
 static struct elf_phdr *load_elf_phdrs(const struct elfhdr *elf_ex,
 				       struct file *elf_file)
 {
-	struct elf_phdr *elf_phdata = NULL;
-	int retval = -1;
-	unsigned int size;
+	struct elf_phdr *elf_phdata;
+	unsigned int size, done;
+	loff_t pos;
+	ssize_t rv;
 
 	/*
 	 * If the size of this structure has changed, then punt, since
 	 * we will be doing the wrong thing.
 	 */
 	if (elf_ex->e_phentsize != sizeof(struct elf_phdr))
-		goto out;
+		return ERR_PTR(-ENOEXEC);
 
 	/* Sanity check the number of program headers... */
-	/* ...and their total size. */
+	/* ...their total size and their offset. */
 	size = sizeof(struct elf_phdr) * elf_ex->e_phnum;
-	if (size == 0 || size > 65536)
-		goto out;
+	if (size == 0 || size > 65536 || elf_ex->e_phoff > LLONG_MAX - size)
+		return ERR_PTR(-ENOEXEC);
 
 	elf_phdata = kmalloc(size, GFP_KERNEL);
 	if (!elf_phdata)
-		goto out;
-
-	/* Read in the program headers */
-	retval = elf_read(elf_file, elf_phdata, size, elf_ex->e_phoff);
+		return ERR_PTR(-ENOMEM);
 
-out:
-	if (retval) {
-		kfree(elf_phdata);
-		elf_phdata = NULL;
+	/*
+	 * 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);
+		}
 	}
 	return elf_phdata;
 }
@@ -864,8 +872,10 @@ static int load_elf_binary(struct linux_binprm *bprm)
 		goto out;
 
 	elf_phdata = load_elf_phdrs(elf_ex, bprm->file);
-	if (!elf_phdata)
+	if (IS_ERR(elf_phdata)) {
+		retval = PTR_ERR(elf_phdata);
 		goto out;
+	}
 
 	elf_ppnt = elf_phdata;
 	for (i = 0; i < elf_ex->e_phnum; i++, elf_ppnt++) {
@@ -968,8 +978,12 @@ static int load_elf_binary(struct linux_binprm *bprm)
 		/* Load the interpreter program headers */
 		interp_elf_phdata = load_elf_phdrs(interp_elf_ex,
 						   interpreter);
-		if (!interp_elf_phdata)
+		if (IS_ERR(interp_elf_phdata)) {
+			if (PTR_ERR(interp_elf_phdata) != -ENOEXEC)
+				retval = PTR_ERR(interp_elf_phdata);
+			interp_elf_phdata = NULL;
 			goto out_free_dentry;
+		}
 
 		/* Pass PT_LOPROC..PT_HIPROC headers to arch code */
 		elf_property_phdata = NULL;

---
base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
change-id: 20261014-b4-prep-vstor-149847-binfmt_elf-enomem-load-elf-phdrs-395ffbfc33e2

Best regards,
--  
Konstantin Khorenko <khorenko@virtuozzo.com>

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

[Kees Cook]

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

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

[Konstantin Khorenko]

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.