[PATCH net v1] net/smc: fix NULL dereference and UAF in smc_tcp_syn_recv_sock()

[PATCH net v1] net/smc: fix NULL dereference and UAF in smc_tcp_syn_recv_sock()

[Jiayuan Chen]

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 taking sk_callback_lock to read sk_user_data and
then sock_hold(&smc->sk) under the lock to pin the smc_sock.
The lock is released immediately after sock_hold(), rather
than being held for the entire function, to avoid holding it
across ori_af_ops->syn_recv_sock() which creates child
sockets and could risk deadlocks with nested lock ordering.
sock_put(&smc->sk) is called on all exit paths after the
hold.

[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/
Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 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..21218b9b0f9a 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -131,7 +131,14 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk,
 	struct smc_sock *smc;
 	struct sock *child;
 
+	read_lock_bh(&((struct sock *)sk)->sk_callback_lock);
 	smc = smc_clcsock_user_data(sk);
+	if (!smc) {
+		read_unlock_bh(&((struct sock *)sk)->sk_callback_lock);
+		return NULL;
+	}
+	sock_hold(&smc->sk);
+	read_unlock_bh(&((struct sock *)sk)->sk_callback_lock);
 
 	if (READ_ONCE(sk->sk_ack_backlog) + atomic_read(&smc->queued_smc_hs) >
 				sk->sk_max_ack_backlog)
@@ -153,11 +160,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;
 }
 
-- 
2.43.0

Re: [PATCH net v1] net/smc: fix NULL dereference and UAF in smc_tcp_syn_recv_sock()

[Eric Dumazet]

On Sat, Mar 7, 2026 at 4:22 AM Jiayuan Chen <jiayuan.chen@linux.dev> wrote:
>
> 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 taking sk_callback_lock to read sk_user_data and
> then sock_hold(&smc->sk) under the lock to pin the smc_sock.
> The lock is released immediately after sock_hold(), rather
> than being held for the entire function, to avoid holding it
> across ori_af_ops->syn_recv_sock() which creates child
> sockets and could risk deadlocks with nested lock ordering.
> sock_put(&smc->sk) is called on all exit paths after the
> hold.
>
> [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/
> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
> ---
>  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..21218b9b0f9a 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -131,7 +131,14 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk,
>         struct smc_sock *smc;
>         struct sock *child;
>
> +       read_lock_bh(&((struct sock *)sk)->sk_callback_lock);

This will not survive a SYN flood attack.

Please use RCU instead.

>         smc = smc_clcsock_user_data(sk);
> +       if (!smc) {
> +               read_unlock_bh(&((struct sock *)sk)->sk_callback_lock);
> +               return NULL;
> +       }
> +       sock_hold(&smc->sk);

If you must take a refcount, use

if (!refcount_inc_not_zero(&smc->sk->sk_refcnt)) {
  rcu_read_unlock();
   return NULL;
}


> +       read_unlock_bh(&((struct sock *)sk)->sk_callback_lock);
>
>         if (READ_ONCE(sk->sk_ack_backlog) + atomic_read(&smc->queued_smc_hs) >
>                                 sk->sk_max_ack_backlog)
> @@ -153,11 +160,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;
>  }
>
> --
> 2.43.0
>

Re: [PATCH net v1] net/smc: fix NULL dereference and UAF in smc_tcp_syn_recv_sock()

[Jiayuan Chen]

March 7, 2026 at 11:56, "Eric Dumazet" <edumazet@google.com mailto:edumazet@google.com?to=%22Eric%20Dumazet%22%20%3Cedumazet%40google.com%3E > wrote:


> 
> On Sat, Mar 7, 2026 at 4:22 AM Jiayuan Chen <jiayuan.chen@linux.dev> wrote:
> 
[...]
> >  diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> >  index d0119afcc6a1..21218b9b0f9a 100644
> >  --- a/net/smc/af_smc.c
> >  +++ b/net/smc/af_smc.c
> >  @@ -131,7 +131,14 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk,
> >  struct smc_sock *smc;
> >  struct sock *child;
> > 
> >  + read_lock_bh(&((struct sock *)sk)->sk_callback_lock);
> > 
> This will not survive a SYN flood attack.
> 
> Please use RCU instead.
> 
> > 
> > smc = smc_clcsock_user_data(sk);
> >  + if (!smc) {
> >  + read_unlock_bh(&((struct sock *)sk)->sk_callback_lock);
> >  + return NULL;
> >  + }
> >  + sock_hold(&smc->sk);
> > 
> If you must take a refcount, use
> 
> if (!refcount_inc_not_zero(&smc->sk->sk_refcnt)) {
>  rcu_read_unlock();
>  return NULL;
> }

Thanks for the review.

Will try rcu_read_lock() + refcount_inc_not_zero()
and set SOCK_RCU_FREE on the listen socket.