Re: [PATCH net v2] net/smc: fix NULL dereference and UAF in smc_tcp_syn_recv_sock()
From: "D. Wythe" <alibuda@linux.alibaba.com >
Date: 2026-03-09 06:06:19
Also in:
linux-rdma, linux-s390, lkml
On Mon, Mar 09, 2026 at 10:38:45AM +0800, Jiayuan Chen wrote:
quoted hunk ↗ jump to hunk
Syzkaller reported a panic in smc_tcp_syn_recv_sock() [1]. smc_tcp_syn_recv_sock() is called in the TCP receive path (softirq) via icsk_af_ops->syn_recv_sock on the clcsock (TCP listening socket). It reads sk_user_data to get the smc_sock pointer. However, when the SMC listen socket is being closed concurrently, smc_close_active() sets clcsock->sk_user_data to NULL under sk_callback_lock, and then the smc_sock itself can be freed via sock_put() in smc_release(). This leads to two issues: 1) NULL pointer dereference: sk_user_data is NULL when accessed. 2) Use-after-free: sk_user_data is read as non-NULL, but the smc_sock is freed before its fields (e.g., queued_smc_hs, ori_af_ops) are accessed. The race window looks like this: CPU A (softirq) CPU B (process ctx) tcp_v4_rcv() TCP_NEW_SYN_RECV: sk = req->rsk_listener sock_hold(sk) /* No lock on listener */ smc_close_active(): write_lock_bh(cb_lock) sk_user_data = NULL write_unlock_bh(cb_lock) ... smc_clcsock_release() sock_put(smc->sk) x2 -> smc_sock freed! tcp_check_req() smc_tcp_syn_recv_sock(): smc = user_data(sk) -> NULL or dangling smc->queued_smc_hs -> crash! Note that the clcsock and smc_sock are two independent objects with separate refcounts. TCP stack holds a reference on the clcsock, which keeps it alive, but this does NOT prevent the smc_sock from being freed. Fix this by using RCU and refcount_inc_not_zero() to safely access smc_sock. Since smc_tcp_syn_recv_sock() is called in the TCP three-way handshake path, taking read_lock_bh on sk_callback_lock is too heavy and would not survive a SYN flood attack. Using rcu_read_lock() is much more lightweight. - Set SOCK_RCU_FREE on the SMC listen socket so that smc_sock freeing is deferred until after the RCU grace period. This guarantees the memory is still valid when accessed inside rcu_read_lock(). - Use rcu_read_lock() to protect reading sk_user_data. - Use refcount_inc_not_zero(&smc->sk.sk_refcnt) to pin the smc_sock. If the refcount has already reached zero (close path completed), it returns false and we bail out safely. Note: smc_hs_congested() has a similar lockless read of sk_user_data without rcu_read_lock(), but it only checks for NULL and accesses the global smc_hs_wq, never dereferencing any smc_sock field, so it is not affected. Reproducer was verified with mdelay injection and smc_run, the issue no longer occurs with this patch applied. [1] https://syzkaller.appspot.com/bug?extid=827ae2bfb3a3529333e9 Fixes: 8270d9c21041 ("net/smc: Limit backlog connections") Reported-by: syzbot+827ae2bfb3a3529333e9@syzkaller.appspotmail.com Closes: https://lore.kernel.org/all/67eaf9b8.050a0220.3c3d88.004a.GAE@google.com/T/ (local) Suggested-by: Eric Dumazet <edumazet@google.com> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev> --- v2: - Use rcu_read_lock() + refcount_inc_not_zero() instead of read_lock_bh(sk_callback_lock) + sock_hold(), since this is the TCP handshake hot path and read_lock_bh is too expensive under SYN flood. - Set SOCK_RCU_FREE on SMC listen socket to ensure RCU-deferred freeing. v1: https://lore.kernel.org/netdev/20260307032158.372165-1-jiayuan.chen@linux.dev/ (local) --- net/smc/af_smc.c | 9 +++++++++ 1 file changed, 9 insertions(+)diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c index d0119afcc6a1..72ac1d8c62d4 100644 --- a/net/smc/af_smc.c +++ b/net/smc/af_smc.c@@ -131,7 +131,13 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk, struct smc_sock *smc; struct sock *child; + rcu_read_lock(); smc = smc_clcsock_user_data(sk); + if (!smc || !refcount_inc_not_zero(&smc->sk.sk_refcnt)) { + rcu_read_unlock(); + return NULL; + } + rcu_read_unlock(); if (READ_ONCE(sk->sk_ack_backlog) + atomic_read(&smc->queued_smc_hs) > sk->sk_max_ack_backlog)@@ -153,11 +159,13 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk, if (inet_csk(child)->icsk_af_ops == inet_csk(sk)->icsk_af_ops) inet_csk(child)->icsk_af_ops = smc->ori_af_ops; } + sock_put(&smc->sk); return child; drop: dst_release(dst); tcp_listendrop(sk); + sock_put(&smc->sk); return NULL; }@@ -2691,6 +2699,7 @@ int smc_listen(struct socket *sock, int backlog) write_unlock_bh(&smc->clcsock->sk->sk_callback_lock); goto out; } + sock_set_flag(sk, SOCK_RCU_FREE);
This RCU approach looks good to me. Since SOCK_RCU_FREE is now enabled, other callers of smc_clcsock_user_data() should also follow this RCU-based pattern. It will eventually allow us to completely remove the annoying sk_callback_lock. D. Wythe
sk->sk_max_ack_backlog = backlog; sk->sk_ack_backlog = 0; sk->sk_state = SMC_LISTEN; -- 2.43.0