Re: [PATCH bpf v2 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:
- [High] Static CFG analysis incorrectly assumes dynptr writers do not change packet data, leading to Use-After-Free via global subprograms and freplace.
--

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

bpf: Fix packet pointer invalidation for skb dynptr writes

This patch modifies the verifier to treat skb dynptr writer kfuncs as
packet-changing operations, ensuring that direct packet pointers are
invalidated after calls like bpf_dynptr_copy or bpf_dynptr_memset.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2abc79dbf281c..5ea51bd284f84 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -11229,7 +11260,33 @@ static bool is_kfunc_bpf_preempt_enable(struct bpf_kfunc_call_arg_meta *meta)
>  
>  bool bpf_is_kfunc_pkt_changing(struct bpf_kfunc_call_arg_meta *meta)
>  {
> -	return meta->func_id == special_kfunc_list[KF_bpf_xdp_pull_data];
> +	return meta->func_id == special_kfunc_list[KF_bpf_xdp_pull_data] ||
> +	       meta->pkt_dynptr_write;
> +}

[Severity: High]
Does this modification cause issues during the control flow graph analysis
phase?

Looking at kernel/bpf/cfg.c, check_cfg() calls bpf_is_kfunc_pkt_changing()
using a statically fetched meta where register states are not yet available:

kernel/bpf/cfg.c:check_cfg() {
    ...
    if (ret == 0 && bpf_is_kfunc_pkt_changing(&meta))
        mark_subprog_changes_pkt_data(env, t);
    ...
}

Since pkt_dynptr_write is only evaluated later during check_kfunc_args()
based on register states, will it always be false during CFG analysis?

If so, it seems this might leave the changes_pkt_data flag incorrectly set
to false for subprograms mutating packets via dynptr writers.

Could this lead to use-after-free scenarios with global subprograms or
freplace? For example, if a global subprogram's dynptr argument is
defaulted to BPF_DYNPTR_TYPE_LOCAL in btf_prepare_func_args():

kernel/bpf/verifier.c:btf_prepare_func_args() {
    ...
    } else if (arg->arg_type == ARG_PTR_TO_DYNPTR) {
        __mark_dynptr_reg(reg, BPF_DYNPTR_TYPE_LOCAL, true, ++env->id_gen, 0);
    ...
}

Will the dynptr write evade detection entirely, leaving the caller with
stale, uninvalidated packet pointers?

> +
> +static bool dynptr_type_pkt_data(enum bpf_dynptr_type type)
> +{
> +	return type == BPF_DYNPTR_TYPE_SKB ||
> +	       type == BPF_DYNPTR_TYPE_SKB_META;
> +}

[ ... ]

> @@ -12214,6 +12271,9 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
>  						  &meta->ref_obj, &meta->dynptr);
>  			if (ret < 0)
>  				return ret;
> +			if (is_kfunc_pkt_dynptr_writer(meta, i) &&
> +			    dynptr_type_pkt_data(meta->dynptr.type))
> +				meta->pkt_dynptr_write = true;
>  			break;
>  		}

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