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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 13 Nov 2014 13:20:20 +0100
From:	Thierry Reding <thierry.reding@...il.com>
To:	Paul Burton <paul.burton@...tec.com>,
	Ralf Baechle <ralf@...ux-mips.org>
Cc:	linux-mips@...ux-mips.org,
	Alexander Viro <viro@...iv.linux.org.uk>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/10] binfmt_elf: load interpreter program headers
 earlier

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

View attachment "patch" of type "text/plain" (297 bytes)

Content of type "application/pgp-signature" skipped