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

[Jiayuan Chen <jiayuan.chen@linux.dev>]

Thanks for the review.


On 3/12/26 11:05 AM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
>
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> net/smc: fix NULL dereference and UAF in smc_tcp_syn_recv_sock()
>
> This patch fixes a race condition in smc_tcp_syn_recv_sock() where
> sk_user_data can be nullified or the smc_sock can be freed during
> concurrent socket closure, using RCU and refcount_inc_not_zero() for
> safe access.
>
> Closes: https://lore.kernel.org/all/67eaf9b8.050a0220.3c3d88.004a.GAE@google.com/T/
>
>> 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!
> The race diagram describes the tcp_check_req() path (TCP_NEW_SYN_RECV
> handling), but the actual syzbot crash occurred through a different path.
> Looking at the syzbot stacktrace, the crash shows:
>
> smc_tcp_syn_recv_sock+0xa7/0x4b0 called from tcp_get_cookie_sock+0xd5/0x790
>
> This is the SYN cookie path:
>
> tcp_v4_rcv() -> tcp_v4_do_rcv() -> tcp_v4_cookie_check() ->
> cookie_v4_check() -> tcp_get_cookie_sock() -> smc_tcp_syn_recv_sock()
>
> The syzbot log even shows 'Possible SYN flooding on port [::]:20002.
> Sending cookies.' confirming syncookies were active. Both paths are
> vulnerable to the race, but the race diagram doesn't match the actual
> crash path that the fuzzer triggered.
>
> Would it be more accurate to note that both paths (tcp_check_req for
> normal SYN handling and tcp_get_cookie_sock for SYN cookies) can trigger
> the race?


For the first point about the call stack: both the tcp_check_req() path 
(normal SYN handling)
and the tcp_get_cookie_sock() path (SYN cookies) call syn_recv_sock() 
without holding the
listener lock, so the race condition is the same. The diagram uses 
tcp_check_req() as an
example because I have reproduced it under my env, but the fix covers 
both paths.

>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> --- 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;
>>
>> -	smc = smc_clcsock_user_data(sk);
>> +	rcu_read_lock();
>> +	smc = smc_clcsock_user_data_rcu(sk);
>> +	if (!smc || !refcount_inc_not_zero(&smc->sk.sk_refcnt)) {
>> +		rcu_read_unlock();
>> +		return NULL;
> When smc_tcp_syn_recv_sock() is called from the SYN cookie path
> (tcp_get_cookie_sock() in net/ipv4/syncookies.c:205 and
> net/ipv6/syncookies.c:275), the dst parameter is non-NULL.
>
> The new early return here returns NULL without calling dst_release(dst).
> The syn_recv_sock contract requires the callee to either consume dst
> (attach to child socket on success) or release it (call dst_release on
> failure).
>
> Looking at tcp_get_cookie_sock() in net/ipv4/syncookies.c:
>
>      child = af_ops->syn_recv_sock(..., &dst, ...)
>      if (!child) {
>          reqsk_free(req);
>          return NULL;
>      }
>
> When syn_recv_sock returns NULL, tcp_get_cookie_sock immediately returns
> NULL without releasing dst, expecting the syn_recv_sock callee to have
> already handled it.
>
> The existing 'drop:' label at line 165 correctly calls dst_release(dst),
> but the new early return at line 138 bypasses it. This causes a dst_entry
> reference count leak under SYN flood conditions on an SMC listener.
>
> Should this be: goto drop instead of return NULL, or should dst_release(dst)
> be added before the return NULL?



The second point about the dst leak is valid. Will fix in v3.