Re: [PATCH bpf-next 2/3] bpf, sockmap: Restrict iter-context sockmap updates to iter/sockmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf: may_update_sockmap() allows attach_func_name spoofing via dst_prog
- [Low] bpf: multi-line comment style violation in may_update_sockmap()
--

commit ba865d5e27181c6f2b179215e5da8fe24b46fb84
Author: Michal Luczaj <mhal@rbox.co>

bpf, sockmap: Restrict iter-context sockmap updates to iter/sockmap

This commit restricts sockmap and sockhash updates from BPF_TRACE_ITER
programs so that only iter/sockmap programs are permitted to perform them,
checking both env->prog->type and env->prog->aux->attach_func_name in
may_update_sockmap().

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ddfa0c6d2755f..c63abb13ec6dd 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -10331,6 +10331,7 @@ static bool may_update_sockmap(struct bpf_verifier_env *env, int func_id)
>  {
>  	enum bpf_attach_type eatype = env->prog->expected_attach_type;
>  	enum bpf_prog_type type = resolve_prog_type(env->prog);
> +	const char *afname = env->prog->aux->attach_func_name;
>  
>  	if (func_id != BPF_FUNC_map_update_elem &&
>  	    func_id != BPF_FUNC_map_delete_elem)
> @@ -10341,7 +10342,12 @@ static bool may_update_sockmap(struct bpf_verifier_env *env, int func_id)
>  	 */
>  	switch (type) {
>  	case BPF_PROG_TYPE_TRACING:
> -		if (eatype == BPF_TRACE_ITER)
> +		/* iter/sockmap only; type check rejects a freplace of any
> +		 * iterator's bpf_iter_sockmap() subprog.
> +		 */

[Severity: Low]
This isn't a bug, but in the BPF subsystem multi-line comments should have
the opening /* on its own line with the comment text starting on the next
line. Should this comment in may_update_sockmap() follow that style?

> +		if (eatype == BPF_TRACE_ITER &&
> +		    env->prog->type == BPF_PROG_TYPE_TRACING &&
> +		    !strcmp(afname + strlen(BPF_ITER_FUNC_PREFIX), "sockmap"))
>  			return true;

[Severity: High]
Can a non-sockmap BPF_TRACE_ITER program still bypass this check in
may_update_sockmap() by specifying attach_prog_fd (dst_prog) during
BPF_PROG_LOAD?

Checking env->prog->type == BPF_PROG_TYPE_TRACING excludes BPF_PROG_TYPE_EXT,
but bpf_prog_load_check_attach() in kernel/bpf/syscall.c also allows
attach_prog_fd (setting prog->aux->dst_prog) when prog_type is
BPF_PROG_TYPE_TRACING.

In kernel/bpf/verifier.c, bpf_check_attach_target() rejects tgt_prog for
BPF_TRACE_RAW_TP, but does not reject tgt_prog != NULL for BPF_TRACE_ITER:

kernel/bpf/verifier.c:bpf_check_attach_target() {
	...
	if (tgt_prog) {
		...
		btf = tgt_prog->aux->btf;
	} else {
		btf = bpf_get_btf_vmlinux();
		...
	}
	...
	case BPF_TRACE_ITER:
		if (!btf_type_is_func(t)) {
			bpf_log(log, "attach_btf_id %u is not a function\n",
				btf_id);
			return -EINVAL;
		}
		...
}

When check_attach_btf_id() calls bpf_check_attach_target(), tgt_info.tgt_name
is resolved from tgt_prog->aux->btf (which is user-controlled BTF) and stored
in prog->aux->attach_func_name before calling bpf_iter_prog_supported(prog):

kernel/bpf/bpf_iter.c:bpf_iter_prog_supported() {
	const char *attach_fname = prog->aux->attach_func_name;
	u32 prog_btf_id = prog->aux->attach_btf_id;
	...
	list_for_each_entry(iter, &targets, list) {
		if (iter->btf_id && iter->btf_id == prog_btf_id) {
			tinfo = iter;
			break;
		}
		if (!strcmp(attach_fname + prefix_len, iter->reg_info->target)) {
			cache_btf_id(iter, prog);
			tinfo = iter;
			break;
		}
	}
	...
}

If a user loads a target BPF program whose BTF defines a function named
bpf_iter_sockmap at the same BTF ID as another iterator in vmlinux BTF:

1. For an iterator appearing before sockmap in the targets list (such as
   bpf_sk_storage_map, which is registered via late_initcall in
   net/core/bpf_sk_storage.c after net/core/sock_map.c), once its
   iter->btf_id is cached, bpf_iter_prog_supported() matches
   iter->btf_id == prog_btf_id first and sets up ctx_arg_info for
   bpf_sk_storage_map. Then may_update_sockmap() sees afname as
   bpf_iter_sockmap and allows sockmap updates, and bpf_iter_link_attach()
   attaches the program to bpf_sk_storage_map. Because
   __bpf_sk_storage_map_seq_show() only holds rcu_read_lock() on
   sk_storage->owner without locking the socket, updating a sockmap with an
   AF_UNIX stream socket from this iterator can trigger the
   unix_stream_bpf_update_proto() sk_pair race described in commit
   64c2f93fc325 ("bpf, sockmap: Take state lock for af_unix iter").

2. For an iterator appearing after sockmap in targets (such as iter/unix),
   bpf_iter_prog_supported() matches "sockmap" by name at the sockmap entry
   and calls cache_btf_id(iter, prog), overwriting sockmap's cached
   iter->btf_id with the other iterator's BTF ID and breaking subsequent
   legitimate iter/sockmap attachments in bpf_iter_link_attach(). In
   addition, bpf_iter_link_replace() only compares type,
   expected_attach_type, and attach_btf_id, allowing this program to replace
   an existing iter/unix link.

Should bpf_check_attach_target() reject tgt_prog != NULL for BPF_TRACE_ITER
(or should may_update_sockmap() check !env->prog->aux->dst_prog)?

>  		break;
>  	case BPF_PROG_TYPE_SOCK_OPS:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-sockmap-update-limit-iter-v1-1-23fa946db59e@rbox.co?part=2