[PATCH] bpftool: Fix memory leak in do_build_table_cb

[PATCH] bpftool: Fix memory leak in do_build_table_cb

[Miaoqian Lin]

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@gmail.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);
 out_close:
 	close(fd);
 out_ret:
-- 
2.25.1

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@gmail.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:
> 

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

[Miaoqian Lin]

Hi, Daniel

On 2022/12/6 4:05, Daniel Borkmann wrote:
> 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@gmail.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?
>
Thanks for your review. You're right. Sorry for the mistake, I meant to free it in the error path.

I'll send v2 to fix this. I spotted it with static detection tool.

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