Re: [PATCH net v5 1/2] bpf, skmsg: fix verdict sk_data_ready racing with ktls rx

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Concurrent sockmap deletion causes a deterministic NULL pointer dereference and kernel panic.
- [High] Type confusion leading to out-of-bounds reads and BPF verdict bypass due to blind ULP context cast.
--

commit 99c04859dba221cd63c5238a47d1833983548f83
Author: Xingwang Xiang <v3rdant.xiang@gmail.com>

bpf, skmsg: fix verdict sk_data_ready racing with ktls rx

sk_psock_strp_data_ready() already checks tls_sw_has_ctx_rx() and
defers to psock->saved_data_ready when a TLS RX context is present.
This patch applies the same logic to sk_psock_verdict_data_ready() to
prevent a use-after-free caused by tcp_read_skb draining the queue
without advancing copied_seq.

> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 6187a83bd7411..e1850caf1a71a 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -1268,12 +1268,19 @@ static int sk_psock_verdict_recv(struct sock *sk, struct sk_buff *skb)
>  static void sk_psock_verdict_data_ready(struct sock *sk)
>  {
>  	const struct proto_ops *ops = NULL;
> +	struct sk_psock *psock;
>  	struct socket *sock;
>  	int copied;
>  
>  	trace_sk_data_ready(sk);
>  
>  	rcu_read_lock();
> +	psock = sk_psock(sk);
> +	if (psock && tls_sw_has_ctx_rx(sk)) {

Is it possible for tls_sw_has_ctx_rx() to cause a type confusion regression here?

tls_sw_has_ctx_rx() calls tls_get_ctx(), which casts icsk->icsk_ulp_data
to a struct tls_context pointer. If a socket has a different ULP configured,
like espintcp, and is attached to a sockmap, icsk_ulp_data will point to a
different structure like espintcp_ctx.

Reading from this mismatched structure could yield garbage data. If that
data is non-zero, the code might falsely assume a KTLS RX context is active
and incorrectly skip the BPF verdict program.

While this problem already exists in sk_psock_strp_data_ready(), does this
change spread the same issue to the verdict path?

> +		psock->saved_data_ready(sk);

Can this lead to a NULL pointer dereference regression during concurrent
sockmap deletion?

sk_psock_verdict_data_ready() executes here without holding
sk->sk_callback_lock. If userspace concurrently removes the socket from the
sockmap, sk_psock_stop_verdict() will be called, which explicitly sets
psock->saved_data_ready to NULL.

Since this code doesn't hold read_lock_bh(&sk->sk_callback_lock) or check if
saved_data_ready is NULL before calling it, could it execute the newly nullified
pointer and cause a crash?

I notice this vulnerability already exists in sk_psock_strp_data_ready(), but
does this duplicate it into the verdict handler?

> +		rcu_read_unlock();
> +		return;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260517145630.20521-1-v3rdant.xiang@gmail.com?part=1