Re: [PATCH net 1/2] bpf, sockmap: account only unread data in tcp_eat_skb

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] tcp_eat_skb() forcefully advancing copied_seq to end_seq corrupts sequence tracking when an SK_DROP/SK_REDIRECT follows an SK_PASS.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the fix is incomplete: it addresses copied_seq corruption for SK_DROP and SK_REDIRECT, but leaves the exact same vulnerability unpatched for SK_PASS, leading to duplicate data delivery and sequence corruption.
--

commit 53da102476a74ffdf7989fd6066b68acdf141844
Author: Dong Chenchen <dongchenchen2@huawei.com>

bpf, sockmap: account only unread data in tcp_eat_skb

tcp_eat_skb() advances copied_seq by the full skb length when a sockmap
verdict drops or redirects an skb, assuming none of the skb has been consumed.
This assumption does not hold when userspace partially reads an skb before
adding the socket to a sockmap. This patch fixes tcp_eat_skb() to advance
copied_seq to the skb TCP end sequence.

Closes: https://syzkaller.appspot.com/bug?extid=06dbd397158ec0ea4983

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index cc0bd73f36b6d..d640f8e065297 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c

[ ... ]

> @@ -24,10 +24,11 @@ void tcp_eat_skb(struct sock *sk, struct sk_buff *skb)
>  		return;
>  
>  	tcp = tcp_sk(sk);
> -	copied = tcp->copied_seq + skb->len;
> -	WRITE_ONCE(tcp->copied_seq, copied);
> +	end_seq = TCP_SKB_CB(skb)->end_seq;
> +	delta = end_seq - tcp->copied_seq;
> +	WRITE_ONCE(tcp->copied_seq, end_seq);

[Severity: High]
Does forcefully advancing copied_seq to end_seq corrupt sequence tracking when
an SK_DROP or SK_REDIRECT follows an SK_PASS?

If a packet is evaluated as SK_PASS and enqueued to ingress_msg, and a
subsequent packet is evaluated as SK_DROP or SK_REDIRECT, this unconditional
overwrite sets copied_seq to the dropped skb's end_seq. This jumps over the
unread SK_PASS sequence space.

When userspace later reads the SK_PASS data, it will increment copied_seq
beyond rcv_nxt, leading to sequence warnings and breaking native TCP reads.


[Severity: High]
This is a pre-existing issue, but does this fix leave the exact same
vulnerability unpatched for SK_PASS?

While this patch addresses copied_seq corruption for SK_DROP and SK_REDIRECT,
the vulnerability seems to remain for SK_PASS, leading to duplicate data
delivery and sequence corruption.

If userspace performs a partial native read on a socket (advancing
copied_seq), and the socket is then added to a sockmap:

A packet arrival triggers tcp_read_skb(), which dequeues the partially-read
skb without trimming the already-read prefix or accounting for the difference
between copied_seq and the skb's start sequence:

net/ipv4/tcp.c:tcp_read_skb() {
    ...
    while ((skb = skb_peek(&sk->sk_receive_queue)) != NULL) {
        __skb_unlink(skb, &sk->sk_receive_queue);
    ...
}

If the BPF program returns SK_PASS, the full skb->len and off=0 are
unconditionally used to enqueue the packet, blindly pushing already-read data
into the ingress queue:

net/core/skmsg.c:sk_psock_verdict_apply() {
    ...
        if (skb_queue_empty(&psock->ingress_skb)) {
            len = skb->len;
            off = 0;
    ...
}

When userspace reads from the sockmap, it will receive the already-read prefix
again, and copied_seq will be incremented by the full length, overshooting
rcv_nxt.

>  	tcp_rcv_space_adjust(sk);
> -	__tcp_cleanup_rbuf(sk, skb->len);
> +	__tcp_cleanup_rbuf(sk, delta);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702140959.1806754-1-dongchenchen2@huawei.com?part=1