Re: [PATCH 02/10] binfmt_elf: load interpreter program headers earlier

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2976 bytes --]

On Thu, Sep 11, 2014 at 08:30:15AM +0100, Paul Burton wrote:
> Load the program headers of an ELF interpreter early enough in
> load_elf_binary that they can be examined before it's too late to return
> an error from an exec syscall. This patch does not perform any such
> checking, it merely lays the groundwork for a further patch to do so.
> 
> No functional change is intended.
> 
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> ---
>  fs/binfmt_elf.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
[...]

kmemleak started complaining for me recently and the stacktrace (see
below) points to this function:

	unreferenced object 0xec0f77c0 (size 192):
	  comm "kworker/u8:0", pid 169, jiffies 4294939367 (age 86.360s)
	  hex dump (first 32 bytes):
	    01 00 00 70 1c ef 01 00 1c ef 01 00 1c ef 01 00  ...p............
	    a0 00 00 00 a0 00 00 00 04 00 00 00 04 00 00 00  ................
	  backtrace:
	    [<c00ec080>] __kmalloc+0x104/0x190
	    [<c01387d4>] load_elf_phdrs+0x60/0x8c
	    [<c0138cb4>] load_elf_binary+0x280/0x12d8
	    [<c00f8ef0>] search_binary_handler+0x80/0x1f0
	    [<c00fa370>] do_execveat_common+0x570/0x658
	    [<c00fa480>] do_execve+0x28/0x30
	    [<c0038eb4>] ____call_usermodehelper+0x144/0x19c
	    [<c000e638>] ret_from_fork+0x14/0x3c
	    [<ffffffff>] 0xffffffff

> @@ -605,7 +598,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  	int load_addr_set = 0;
>  	char * elf_interpreter = NULL;
>  	unsigned long error;
> -	struct elf_phdr *elf_ppnt, *elf_phdata;
> +	struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL;
>  	unsigned long elf_bss, elf_brk;
>  	int retval, i;
>  	unsigned long elf_entry;
> @@ -729,6 +722,12 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  		/* Verify the interpreter has a valid arch */
>  		if (!elf_check_arch(&loc->interp_elf_ex))
>  			goto out_free_dentry;
> +
> +		/* Load the interpreter program headers */
> +		interp_elf_phdata = load_elf_phdrs(&loc->interp_elf_ex,
> +						   interpreter);
> +		if (!interp_elf_phdata)
> +			goto out_free_dentry;
>  	}
>  
>  	/* Flush all traces of the currently running executable */
> @@ -912,7 +911,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
>  		elf_entry = load_elf_interp(&loc->interp_elf_ex,
>  					    interpreter,
>  					    &interp_map_addr,
> -					    load_bias);
> +					    load_bias, interp_elf_phdata);
>  		if (!IS_ERR((void *)elf_entry)) {
>  			/*
>  			 * load_elf_interp() returns relocation
> @@ -1009,6 +1008,7 @@ out_ret:
>  
>  	/* error cleanup */
>  out_free_dentry:
> +	kfree(interp_elf_phdata);

I think what happens is that the interp_elf_phdata memory is freed only
in the error cleanup path, but not when the function actually succeeds.

The attached patch plugs the leak for me.

Thierry

[-- Attachment #1.2: patch --]
[-- Type: text/plain, Size: 308 bytes --]

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index f95da60e440e..8a9be83e88c2 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1029,6 +1029,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
 		}
 	}
 
+	kfree(interp_elf_phdata);
 	kfree(elf_phdata);
 
 	set_binfmt(&elf_format);

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]