diff mbox series

bpftool: Fix memory leak in do_build_table_cb

Message ID 20221205081300.561974-1-linmq006@gmail.com
State New
Headers
Series bpftool: Fix memory leak in do_build_table_cb |

Commit Message

Miaoqian Lin Dec. 5, 2022, 8:13 a.m. UTC 
  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(-)

Comments

Daniel Borkmann Dec. 5, 2022, 8:05 p.m. UTC | #1 
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:
>
Miaoqian Lin Dec. 6, 2022, 7:14 a.m. UTC | #2 
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:
>>
>
diff mbox series

Patch

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: