Re: [PATCH bpf v3 1/2] bpf: Fix packet pointer invalidation for skb dynptr writes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The `bpf_dynptr_write` helper fails to invalidate packet pointers in global subprograms, leading to a Use-After-Free vulnerability.
--

commit a8b2f9ebe5479e8968087326226bb472f13ee1e0
Author: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn>

bpf: Fix packet pointer invalidation for skb dynptr writes

This patch ensures that skb-backed dynptr writer kfuncs invalidate checked
direct packet pointers, following the same rule as the bpf_dynptr_write()
helper. It updates the CFG analysis to mark these kfuncs as packet-changing
and invalidates packet pointers when the dynptr might be skb-backed.

[ ... ]

> diff --git a/kernel/bpf/cfg.c b/kernel/bpf/cfg.c
> index 26d37066465f3..7ed6bdd986bc8 100644
> --- a/kernel/bpf/cfg.c
> +++ b/kernel/bpf/cfg.c
> @@ -516,7 +516,7 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
>  			 */
>  			if (ret == 0 && bpf_is_kfunc_sleepable(&meta))
>  				mark_subprog_might_sleep(env, t);
> -			if (ret == 0 && bpf_is_kfunc_pkt_changing(&meta))
> +			if (ret == 0 && bpf_kfunc_may_change_pkt_data(&meta))
>  				mark_subprog_changes_pkt_data(env, t);

[Severity: Critical]
This is a pre-existing issue, but since this patch aims to make kfuncs
follow the same rule as the bpf_dynptr_write() helper, could there be a
similar regression with the helper itself?

When bpf_dynptr_write() is called within a global subprogram using an
unspecialized LOCAL dynptr argument that is actually backed by an skb,
does it fail to invalidate packet pointers?

In cfg.c:visit_insn(), CFG analysis appears to miss the side-effect of
bpf_dynptr_write() because the helper cannot be included in
bpf_helper_changes_pkt_data() without triggering a verifier bug in
check_helper_call() (due to its arg1_type being ARG_PTR_TO_DYNPTR
instead of ARG_PTR_TO_CTX).

Furthermore, during the normal verification path in verifier.c:
check_helper_call(), does the check:

	if (dynptr_type == BPF_DYNPTR_TYPE_SKB ||
	    dynptr_type == BPF_DYNPTR_TYPE_SKB_META)
		changes_data = true;

fail to identify BPF_DYNPTR_TYPE_LOCAL arguments from global subprograms
that may be skb-backed? 

If packet pointers are not invalidated, writing to the dynptr might pull
the skb and reallocate the packet buffer, leaving old packet pointers
pointing to freed memory and resulting in a use-after-free.

>  			if (ret == 0 && bpf_is_throw_kfunc(insn))
>  				mark_subprog_might_throw(env, t);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1781531784.git.chenyy23@mails.tsinghua.edu.cn?part=1