Re: [PATCH] bpf, sockmap: Fix ring capacity invariant break in bpf_msg_push_data()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Low] bpf: non-standard multi-line comment style in
  bpf_msg_push_data()
- [Low] bpf, sockmap: inaccurate -ENOSPC claim and dead check in
  bpf_msg_push_data()
- [Low] bpf, sockmap: inaccurate Fixes trailer in bpf_msg_push_data()
  fix
- [Low] bpf: unreachable sg.end check in bpf_msg_push_data() copy path
--

commit a346141a5e25b55660858c935ff1dea8fc5392a6
Author: Bo Cao <treebacker@foxmail.com>

> bpf, sockmap: Fix ring capacity invariant break in bpf_msg_push_data()
>
> A push at msg->size ends the element walk on the ring's free slot, so
> the "no space, absorb by copy" fallback reads the free slot's length
> instead of the last live element's. Nothing is absorbed and the split
> path runs anyway: with the ring already holding MAX_MSG_FRAGS live
> elements it advances sg.end onto sg.start, all NR_MSG_FRAG_IDS slots
> become live, and the occupancy (the distance from sg.start to
> sg.end) reads 0 -- "empty". Every capacity check in push/pop is then
> bypassed and later transforms corrupt the element list.
>
> Without the fix, a single send() with an SK_MSG verdict program on a
> full ring deterministically crashes the SK_DROP teardown (arm64,
> 7.4.0-rc2 + the bpf master sockmap series):
>
>   Unable to handle kernel NULL pointer dereference at virtual address 0000000000000008
>   ESR = 0x0000000096000004  FSC = 0x04: level 0 translation fault
>   Internal error: Oops: 0000000096000004 [#1]  SMP
>   CPU: 2 UID: 0 PID: 546 Comm: skmsg_panic Tainted: G        W           7.4.0-rc2 #1 PREEMPT(full)
>   pc : sk_msg_free_elem+0x90/0x148
>   lr : __sk_msg_free+0x8c/0x10c
>   x14: 4141414141414141
>   Call trace:
>    sk_msg_free_elem+0x90/0x148
>    __sk_msg_free+0x8c/0x10c
>    sk_msg_free+0x18/0x24
>    tcp_bpf_sendmsg+0x298/0xa54
>    inet_sendmsg+0x44/0x70
>    __sys_sendto+0x120/0x198
>
> put_page() on a zeroed scatterlist entry. With the fix the helper
> returns -ENOSPC, the message frees normally, and the crash is gone.
>
> Never read the free slot in the fallback: absorb into the last live
> element when the walk ended on it and the ring is full, or fail with
> -ENOSPC if there is nothing to absorb, so the guard slot is never
> consumed.
>
> The bug was found with LLM-assisted source auditing and verified end
> to end: it is reachable with CAP_BPF/CAP_NET_ADMIN and confirmed
> exploitable for local privilege escalation (details reported to the
> maintainers separately). Regression tested with
> tools/testing/selftests/bpf/test_sockmap.
>
> Tested on top of v7.4-rc2 plus the sockmap fix series currently in
> bpf/bpf.git master (b889ea1f6dbe..0048086bd361); neither that series
> nor the June sk_msg fixes (f3f34ca45b96 and friends) touch the ring
> capacity invariant, and the patch applies to mainline and bpf master
> alike.
>
> Fixes: 6fff607e2f14 ("bpf: sk_msg program helper bpf_msg_push_data")
> Signed-off-by: Bo Cao <treebacker@foxmail.com>

[Severity: Low]
Prior to commit 15ab0548e310 ("bpf, sockmap: Several fixes to
bpf_msg_push_data"), bpf_msg_push_data() checked start >= offset + l after
the scatterlist walk and returned -EINVAL whenever the loop ended at
i == msg->sg.end on a non-empty ring.  Commit 15ab0548e310 ("bpf, sockmap:
Several fixes to bpf_msg_push_data") relaxed that check to allow pushing at
the end of a message and added the sk_msg_iter_var_prev(i) adjustment inside
if (copy), overlooking that copy = msg->sg.data[i].length was read earlier.
Should the Fixes: tag point to that commit instead?

  Fixes: 15ab0548e310 ("bpf, sockmap: Several fixes to bpf_msg_push_data")

> diff --git a/net/core/filter.c b/net/core/filter.c
> index d68b521504050..9ecb94a56d548 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2892,8 +2892,29 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start,
>  	 * offset case because its the common case for inserting a
>  	 * header.
>  	 */
> -	if (!space || (space == 1 && start != offset))
> -		copy = msg->sg.data[i].length;
> +	if (!space || (space == 1 && start != offset)) {
> +		/* A push at msg->size leaves the walk on the ring's free
> +		 * slot, which is not a live element: its length reads 0
> +		 * (or stale data left by an earlier ring rotation), so
> +		 * the copy fallback would absorb nothing and the split
> +		 * path below would run. With the ring already full
> +		 * (space == 0) that path advances sg.end onto sg.start,
> +		 * leaving all NR_MSG_FRAG_IDS slots live -- the ring can
> +		 * no longer tell "full" from "empty" and later
> +		 * transforms corrupt state. Absorb into the last live
> +		 * element instead, or fail if there is nothing to absorb.
> +		 */

[Severity: Low]
This isn't a bug, but BPF coding style places the opening /* of a multi-line
comment on its own line with the comment text starting on the next line.

> +		if (i == msg->sg.end) {
> +			if (!space) {
> +				sk_msg_iter_var_prev(i);
> +				copy = msg->sg.data[i].length;
> +				if (!copy)
> +					return -ENOSPC;
> +			}
> +		} else {
> +			copy = msg->sg.data[i].length;
> +		}
> +	}
>  
>  	if (unlikely(copy + len < copy))
>  		return -EINVAL;

[Severity: Low]
Can copy ever be zero here when !space holds, or is the commit message
inaccurate in stating that the helper returns -ENOSPC on a full ring?

Every live scatterlist element in [msg->sg.start, msg->sg.end) has a
non-zero length.  When !space holds, the ring has MAX_MSG_FRAGS live
elements, so stepping i back from msg->sg.end selects the last live element
with msg->sg.data[i].length > 0; bpf_msg_push_data() then absorbs the push
into a newly allocated page and returns 0 (or -ENOMEM) rather than -ENOSPC.

[Severity: Low]
Does stepping i back from msg->sg.end here make the second msg->sg.end check
inside if (copy) unreachable dead code?

net/core/filter.c:bpf_msg_push_data() {
    ...
	if (copy) {
		int front, back;

		raw = page_address(page);

		if (i == msg->sg.end)
			sk_msg_iter_var_prev(i);
    ...
}

copy is initialized to 0 and is only set to a non-zero value when
i != msg->sg.end or after the !space branch above has already decremented i
away from msg->sg.end, so i can never equal msg->sg.end when entering
if (copy).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_84A432FD1AE7054D384BEB36958789AAFA05@qq.com?part=1