Re: [PATCH bpf] bpf: Fix NULL pointer dereference in __bpf_sk_storage_map_seq_show

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 4801 bytes --]

> diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
> index 1d295a8769fa..7228520aa2b6 100644
> --- a/net/core/bpf_sk_storage.c
> +++ b/net/core/bpf_sk_storage.c
> @@ -806,6 +806,8 @@ static int __bpf_sk_storage_map_seq_show(struct seq_file *seq,
>  		ctx.map = info->map;
>  		if (selem) {
>  			sk_storage = rcu_dereference(selem->local_storage);
> +			if (!sk_storage)
> +				return 0;
                                   ^
Should this return SEQ_SKIP instead of 0?

Returning 0 tells bpf_seq_read() that the object was shown, but it was
actually skipped. In kernel/bpf/bpf_iter.c bpf_seq_read():

	err = seq->op->show(seq, p);
	if (err > 0) {
		/* object is skipped, decrease seq_num, so next
		 * valid object can reuse the same seq_num.
		 */
		bpf_iter_dec_seq_num(seq);
		seq->count = offs;
	} else if (err < 0 || seq_has_overflowed(seq)) {

With return 0, bpf_iter_dec_seq_num() is not called, so the skipped
element consumes a seq_num that no BPF program invocation ever observes.

If the skipped element is the first one returned by
bpf_sk_storage_map_seq_start(), seq_num 0 is consumed with no program
run, the next valid element is shown with ctx->meta->seq_num == 1, and
iterator programs that emit a header on `ctx->meta->seq_num == 0`
silently lose their header. Examples that would break:

tools/testing/selftests/bpf/progs/bpf_iter_bpf_hash_map.c:
	if (ctx->meta->seq_num == 0)
		BPF_SEQ_PRINTF(seq, "      id   refcnt  ...\n");

tools/testing/selftests/bpf/progs/bpf_iter_tcp4.c:
	if (ctx->meta->seq_num == 0)
		BPF_SEQ_PRINTF(seq, "  sl  local_address ...\n");

For later elements the observable effect is a gap in the seq_num
sequence.

The other socket iterators handle this exact situation (object found
stale after handed to ->show()) by returning SEQ_SKIP:

net/ipv4/tcp_ipv4.c bpf_iter_tcp_seq_show():
	if (unlikely(sk_unhashed(sk))) {
		ret = SEQ_SKIP;
		goto unlock;
	}

net/ipv4/udp.c bpf_iter_udp_seq_show():
	if (unlikely(sk_unhashed(sk))) {
		ret = SEQ_SKIP;
		goto unlock;
	}

net/unix/af_unix.c bpf_iter_unix_seq_show():
	ret = SEQ_SKIP;

Returning SEQ_SKIP is safe for the first object too: seq_num is u64
and bpf_iter_dec_seq_num()/bpf_iter_inc_seq_num() in
kernel/bpf/bpf_iter.c wrap back to 0 before the next ->show().

---

A subsystem pattern flags the following as potentially concerning:

>  			sk_storage = rcu_dereference(selem->local_storage);
> +			if (!sk_storage)
> +				return 0;
>  			ctx.sk = sk_storage->owner;
>  			ctx.value = SDATA(selem)->data;

The fix adds a single check-based abort path for a race that has no
exclusion mechanism, and the instructions that follow the abort point
still consume the contested selem.

Looking at bpf_selem_unlink_nofail() in kernel/bpf/bpf_local_storage.c,
it tears the selem down in this order:

1. b->lock: hlist_del_init_rcu(&selem->map_node);
   bpf_obj_free_fields(smap->map.record, SDATA(selem)->data);
2. RCU_INIT_POINTER(SDATA(selem)->smap, NULL);
3. local_storage->lock: hlist_del_init_rcu(&selem->snode);
4. RCU_INIT_POINTER(selem->local_storage, NULL);    <- new check sees this

A concurrent iterator that re-reads selem->local_storage between steps
1 and 4 still sees a non-NULL pointer and proceeds to:

	ctx.sk = sk_storage->owner;
	/* owner is deliberately NOT cleared in the destroy path:
	 * "Okay to skip clearing owner_storage and storage->owner
	 * in destroy() since the owner is going away" - so ctx.sk
	 * can be a sock already inside __sk_destruct() */

	ctx.value = SDATA(selem)->data;
	/* bpf_obj_free_fields() has already released the value's
	 * special fields (kptr/timer/etc.) at step 1 */

So the NULL check closes the specific general-protection fault in the
report (sk_storage == NULL, offset 0x88 == offsetof(struct
bpf_local_storage, owner), matching the KASAN address
0xdffffc0000000011) but does not make the iteration race-safe; it
narrows the window rather than closing it.

Worth confirming with the local-storage maintainers whether the
iterator needs to observe selem->state / SDATA(selem)->smap (or take
the same exclusion bpf_selem_unlink_nofail uses) rather than only
local_storage?

The residual window is not introduced by this patch - the same two
lines executed unconditionally before it, so relative to the parent
commit the change is a strict improvement. Practical impact of the
residual window is also limited: ctx.sk is registered
PTR_TO_BTF_ID_OR_NULL so programs must NULL-check it, and ctx.value is
PTR_TO_BUF, so a BPF program cannot dereference a released kptr read
out of it without a kfunc.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33042447773