Re: [PATCH bpf-next 33/38] bpf: Drop packet pointers behind kfuncs writing an skb

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: missing seen_direct_write for dynptr_write_kfuncs in check_kfunc_call()
--

commit 851ba1993e9264b3115b008c369c9dc215b291c2
Author: Josef Bacik <josef@toxicpanda.com>

bpf: Drop packet pointers behind kfuncs writing an skb

This commit invalidates packet pointers and skb dynptr slices when kfuncs
such as bpf_dynptr_copy(), bpf_dynptr_memset(),
bpf_probe_read_{user,kernel}[_str]_dynptr(), and
bpf_copy_from_user[_task][_str]_dynptr() write through an skb or skb_meta
dynptr.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 6ee9b5c6fe477..e002728abd041 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -15625,7 +15659,9 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  		}
>  	}
>  
> -	if (bpf_is_kfunc_pkt_changing(&meta))
> +	if (is_kfunc_dynptr_write(&meta) ?
> +	    dynptr_write_changes_pkt_data(meta.dynptr_write_type) :
> +	    bpf_is_kfunc_pkt_changing(&meta))
>  		clear_all_pkt_pointers(env);
>  
>  	proto_slots = kfunc_abi_slots(&desc->func_model);

[Severity: High]
When a kfunc in dynptr_write_kfuncs writes to a BPF_DYNPTR_TYPE_SKB or
BPF_DYNPTR_TYPE_SKB_META dynptr, does check_kfunc_call() also need to set
env->seen_direct_write (for instance via may_access_direct_pkt_data(env,
NULL, BPF_WRITE)) so that convert_ctx_accesses() emits
bpf_unclone_prologue()?

Unlike bpf_dynptr_write(), which always goes through __bpf_dynptr_write() ->
__bpf_skb_store_bytes() -> bpf_try_make_writable(), all of the kfuncs in
dynptr_write_kfuncs first try to obtain a writable slice in C via
bpf_dynptr_slice_rdwr() and write directly into it:

kernel/bpf/helpers.c:bpf_dynptr_memset() {
	...
	slice = bpf_dynptr_slice_rdwr(p, offset, NULL, size);
	if (likely(slice)) {
		memset(slice, val, size);
		return 0;
	}
	...
}

For linear skb data and skb metadata, bpf_dynptr_slice_rdwr() returns a
direct pointer into skb->head without uncloning the skb, relying on the
verifier having emitted bpf_unclone_prologue():

kernel/bpf/helpers.c:bpf_dynptr_slice_rdwr() {
	/*
	 * 1) The requested slice is in the head of the skb. In this case, the
	 * returned pointer is directly to skb data, and if the skb is cloned, the
	 * verifier will have uncloned it (see bpf_unclone_prologue()) already.
	 * The pointer can be directly written into.
	 */
	...
	return bpf_dynptr_slice(p, offset, buffer__nullable, buffer__szk);
}

When a program calls bpf_dynptr_slice_rdwr() directly, check_special_kfunc()
calls may_access_direct_pkt_data(env, NULL, BPF_WRITE), which sets
env->seen_direct_write = true. However, specialize_kfunc() restores
env->seen_direct_write to its previous value when specializing
KF_bpf_dynptr_from_skb:

kernel/bpf/verifier.c:specialize_kfunc() {
	...
	} else if (func_id == special_kfunc_list[KF_bpf_dynptr_from_skb]) {
		seen_direct_write = env->seen_direct_write;
		is_rdonly = !may_access_direct_pkt_data(env, NULL, BPF_WRITE);

		if (is_rdonly)
			addr = (unsigned long)bpf_dynptr_from_skb_rdonly;

		/* restore env->seen_direct_write to its original value, since
		 * may_access_direct_pkt_data mutates it
		 */
		env->seen_direct_write = seen_direct_write;
	}
}

If a program creates an skb or skb_meta dynptr and only modifies it through
one of the kfuncs in dynptr_write_kfuncs without performing other direct
packet writes, env->seen_direct_write remains false. Could this skip
bpf_unclone_prologue() and allow these kfuncs to overwrite shared skb->head
memory in place on cloned skbs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017012058.355787-1-alexei.starovoitov@gmail.com?part=33