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

From: Thierry Reding
Date: Thu Nov 13 2014 - 07:20:47 EST


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: pgp0uydGmgpgM.pgp
Description: PGP signature