netdev - Re: [PATCH bpf-next v6 2/6] bpf: Support associating BPF program with struct

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: <c891d752-33cc-413d-8311-dcf8afbf339d@linux.dev>
Date: Mon, 17 Nov 2025 13:39:44 -0800
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: ameryhung@...il.com
Cc: netdev@...r.kernel.org, alexei.starovoitov@...il.com, andrii@...nel.org,
 daniel@...earbox.net, tj@...nel.org, martin.lau@...nel.org,
 kernel-team@...a.com, ast@...nel.org, eddyz87@...il.com,
 yonghong.song@...ux.dev, clm@...a.com, ihor.solodrai@...ux.dev,
 bpf@...r.kernel.org, bot+bpf-ci@...nel.org
Subject: Re: [PATCH bpf-next v6 2/6] bpf: Support associating BPF program with
 struct_ops



On 11/14/25 2:51 PM, bot+bpf-ci@...nel.org wrote:
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index a41e6730e..626aa9197 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
> 
> [ ... ]
> 
>> @@ -811,6 +822,12 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
>>   		bpf_link_init(&link->link, BPF_LINK_TYPE_STRUCT_OPS,
>>   			      &bpf_struct_ops_link_lops, prog, prog->expected_attach_type);
>>   		*plink++ = &link->link;
>> +
>> +		err = bpf_prog_assoc_struct_ops(prog, &st_map->map);
>> +		if (err) {
>> +			bpf_prog_put(prog);
>> +			goto reset_unlock;
>> +		}
> 
> Can this double-put the prog reference? In bpf_struct_ops_map_update_elem:
> 
>    prog = bpf_prog_get(prog_fd);              // refcnt = 1
>    bpf_link_init(..., prog, ...);             // link takes ownership
>    *plink++ = &link->link;                    // link stored in st_map
>    err = bpf_prog_assoc_struct_ops(prog, ...);
>    if (err) {
>      bpf_prog_put(prog);                      // refcnt = 0, prog freed
>      goto reset_unlock;
>    }
> 
> Then at reset_unlock:
> 
>    bpf_struct_ops_map_put_progs(st_map);      // calls bpf_link_put(link)
> 
> The bpf_link_put will eventually call bpf_link_dealloc()->bpf_prog_put(link->prog),
> attempting to put the already-freed prog reference. This looks like a use-after-free
> if bpf_prog_assoc_struct_ops fails (which can happen when a non-struct_ops program
> is already associated and returns -EBUSY).

The ai-review should be correct in general on the double bpf_prog_put.

> 
> Should the error path skip the bpf_prog_put and let bpf_struct_ops_map_put_progs
> handle the cleanup via the link?

bpf_prog_assoc_struct_ops will never return error for 
BPF_PROG_TYPE_STRUCT_OPS. If that is the case, maybe completely remove 
the err check.