Re: [PATCH bpf v1 1/2] mptcp, bpf: reject bpf_sk_release() on msk

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: verifier use-after-free via derived socket pointer fields
--

commit 4a3543a3768c369da5d09b5be8da85c1dbd33f90
Author: Jiayuan Chen <jiayuan.chen@linux.dev>

mptcp, bpf: reject bpf_sk_release() on msk

This commit fixes a reference leak by changing bpf_skc_to_mptcp_sock() to
derive its return value from the subflow using parent_id, preventing
bpf_sk_release() from dropping a refcount that was never taken.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5d7080c260d84..8271d9214da52 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -11263,6 +11273,14 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  		bpf_diag_mod_begin(env, &regs[BPF_REG_0], NULL, BPF_DIAG_MOD_WRITE);
>  		regs[BPF_REG_0].type &= ~PTR_MAYBE_NULL;
>  		regs[BPF_REG_0].id = meta.ref_obj.id;
> +	} else if (is_ptr_derive_function(func_id) &&
> +		   find_reference_state(env->cur_state, meta.ref_obj.id)) {
> +		err = validate_ref_obj(env, &meta.ref_obj);
> +		if (err)
> +			return err;
> +
> +		/* Ensures we don't access the object after a release_reference() */
> +		regs[BPF_REG_0].parent_id = meta.ref_obj.id;

[Severity: High]
Does assigning parent_id without setting a reference ID or PTR_TRUSTED leave
the returned msk pointer untrusted in the verifier?

Because msk is untrusted, if a program accesses its pointer fields
(e.g., sk = msk->first), it appears the access falls through to the deprecated
verifier compatibility block in check_ptr_to_btf_access():

kernel/bpf/verifier.c:check_ptr_to_btf_access() {
    ...
    } else {
        /* Old compat. Deprecated */
        clear_trusted_flags(&flag);
    }
    ...
}

This yields a plain PTR_TO_BTF_ID without the PTR_UNTRUSTED flag. Since
pointer field accesses do not propagate parent_id, sk->parent_id would be 0.

When bpf_sk_release(subflow) is called, the verifier invalidates registers
tied to subflow (including msk via parent_id).

Could sk escape this invalidation cascade since its parent_id is 0?

If so, would the program be able to pass this valid, plain PTR_TO_BTF_ID to
kernel helpers like bpf_skc_to_tcp_sock(sk)?

At runtime, the helper would dereference the pointer without PROBE_MEM
protection after it has been freed, potentially resulting in a use-after-free.

>  	} else if (is_acquire_function(func_id, meta.map.ptr)) {
>  		int id = acquire_reference(env, insn_idx, 0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917081209.361367-1-jiayuan.chen@linux.dev?part=1