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@xxxxxxxxxx>
> ---
> 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
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:
pgpQG9PPg6eA9.pgp
Description: PGP signature
