Re: [PATCH bpf-next v3 5/6] bpf: Check async callback value membership in the setter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] bpf: missing map usercnt check in __bpf_async_set_callback()
--

commit a30152ffd79aa0871058c367266aaec41f12d33b
Author: Yun Lu <luyun@kylinos.cn>

bpf: Check async callback value membership in the setter

This commit checks map element membership in __bpf_async_set_callback() so
that timers and workqueues initialized on deleted map elements cannot have a
callback installed and started.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 716be7a852b58..9fa75bcd5e77f 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1552,11 +1552,25 @@ static int __bpf_async_set_callback(struct bpf_async_kern *async, void *callback
>  				    struct bpf_prog *prog)
>  {
>  	struct bpf_async_cb *cb;
> +	struct bpf_map *map;
>  
>  	cb = READ_ONCE(async->cb);
>  	if (!cb)
>  		return -EINVAL;
>  
> +	/*
> +	 * The prog can still hold a value of an element that was deleted from the map.
> +	 * bpf_async_cancel_and_free() is done with it and nothing stops a new callback
> +	 * when the element is freed. cb is published at this point: either the lookup
> +	 * doesn't find the element or bpf_async_cancel_and_free() finds cb.
> +	 * Nothing runs without callback_fn. An idle cb stays in the element until the
> +	 * element is reused or freed.
> +	 */
> +	smp_mb();
> +	map = READ_ONCE(cb->map);
> +	if (!map || !bpf_map_value_is_linked(map, cb->value))
> +		return -ENOENT;
> +
>  	return bpf_async_update_prog_callback(cb, prog, callback_fn);
>  }

[Severity: High]
Does __bpf_async_set_callback() also need to check atomic64_read(&map->usercnt)
and return -EPERM when it is zero?

Because __bpf_async_init() leaves an idle cb attached to an unlinked element
in a non-preallocated hash or rhash map's bpf_mem_alloc cache while
map->usercnt > 0, a subsequent bpf_map_put_uref() only walks linked elements
in htab_free_malloced_internal_structs() / rhtab_map_free_internal_structs()
and never clears that unlinked element's cb.

If a loaded BPF program then recycles that element back into the map via
bpf_map_update_elem() after map->usercnt has reached 0, copy_map_value()
preserves the timer/wq field and bpf_map_value_is_linked() succeeds here.
Installing cb->prog after map_release_uref() has already run pins the BPF
program and map in a permanent reference cycle, or allows a callback on an
inner map to race with non-synchronous cancellation in htab_map_free() /
rhtab_map_free() and use the freed map and element after they are kfreed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111024333.228633-1-luyun_611@163.com?part=5