linux-kernel - Re: [PATCH] bpftool: Fix memory leak in do_build_table

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]

Message-ID: <8d7ac47d-5d76-eaf1-7c1e-a4418d80dac5@iogearbox.net>
Date:   Mon, 5 Dec 2022 21:05:08 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Miaoqian Lin <linmq006@...il.com>,
        Quentin Monnet <quentin@...valent.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
        bpf@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] bpftool: Fix memory leak in do_build_table_cb

On 12/5/22 9:13 AM, Miaoqian Lin wrote:
> strdup() allocates memory for path. We need to release the memory in
> the following error paths. Add free() to avoid memory leak.
> 
> Fixes: 8f184732b60b ("bpftool: Switch to libbpf's hashmap for pinned paths of BPF objects")
> Signed-off-by: Miaoqian Lin <linmq006@...il.com>
> ---
>   tools/bpf/bpftool/common.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index 0cdb4f711510..8a820356525e 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -499,9 +499,11 @@ static int do_build_table_cb(const char *fpath, const struct stat *sb,
>   	if (err) {
>   		p_err("failed to append entry to hashmap for ID %u, path '%s': %s",
>   		      pinned_info.id, path, strerror(errno));
> -		goto out_close;
> +		goto out_free;
>   	}
>   
> +out_free:
> +	free(path);

It would be ok if you were to add the free(path) into the err condition, but here you
also cause the !err to be freed which would trigger as UAF. See the hashmap_insert()
where just set the pointer entry->value = <path>.. how was this tested before submission?

>   out_close:
>   	close(fd);
>   out_ret:
>