Re: [PATCH] bpftool: Fix memory leak in do_build_table_cb

[Daniel Borkmann]

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@xxxxxxxxx>
> ---
>   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: