Re: [PATCH net] net/smc: fix UAF in smc_cdc_rx_handler() by pinning the socket

[Sidraya Jayagond <sidraya@linux.ibm.com>]

On 27/06/26 7:19 am, Xiang Mei wrote:
> smc_cdc_rx_handler() looks up the connection by token under the link
> group's conns_lock, drops the lock, and then dereferences conn and the
> smc_sock derived from it, ending in sock_hold(&smc->sk) inside
> smc_cdc_msg_recv(). No reference is held across the lock release.
> 
> The only reference pinning the socket while the connection is
> discoverable in the link group is taken in smc_lgr_register_conn()
> (sock_hold) and dropped in __smc_lgr_unregister_conn() (sock_put), both
> under conns_lock. Once the handler drops conns_lock, a concurrent
> close() -> smc_release() -> smc_conn_free() -> smc_lgr_unregister_conn()
> can drop that reference and free the smc_sock, so the handler's later
> sock_hold() runs on freed memory:
> 
>   WARNING: lib/refcount.c:25 at refcount_warn_saturate
>   Workqueue: rxe_wq do_work
>    refcount_warn_saturate (lib/refcount.c:25)
>    smc_cdc_msg_recv (net/smc/smc_cdc.c:430)
>    smc_cdc_rx_handler (net/smc/smc_cdc.c:502)
>    smc_wr_rx_tasklet_fn (net/smc/smc_wr.c:445)
>    tasklet_action_common (kernel/softirq.c:938)
>    handle_softirqs (kernel/softirq.c:622)
>   Kernel panic - not syncing: panic_on_warn set
> 
> Only SMC-R is affected. The SMC-D receive tasklet is stopped by
> tasklet_kill(&conn->rx_tsklet) in smc_conn_free() before the connection
> is unregistered, so it cannot run concurrently with the free.
> 
> Take the socket reference while still holding conns_lock, so the
> registration reference can no longer be the last one, and drop it once
> the handler is done.
> 
> Fixes: d7b0e37c1ac1 ("net/smc: restructure CDC message reception")
> Reported-by: Weiming Shi <bestswngs@gmail.com>
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Xiang Mei <xmei5@asu.edu>
> ---
>  net/smc/smc_cdc.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index 619b3bab3824..b809139d7e87 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c
> @@ -483,21 +483,27 @@ static void smc_cdc_rx_handler(struct ib_wc *wc, void *buf)
>  	lgr = smc_get_lgr(link);
>  	read_lock_bh(&lgr->conns_lock);
>  	conn = smc_lgr_find_conn(ntohl(cdc->token), lgr);
> +	if (conn && !conn->out_of_sync)
> +		sock_hold(&container_of(conn, struct smc_sock, conn)->sk);
> +	else
> +		conn = NULL;
>  	read_unlock_bh(&lgr->conns_lock);
> -	if (!conn || conn->out_of_sync)
> +	if (!conn)
>  		return;
>  	smc = container_of(conn, struct smc_sock, conn);
>  

Fix looks correct.
A few nits on the implementation:
container_of() is called twice for the same conn. The conn = NULL
sentinel and the second post unlock check can also be dropped. Flip the
condition, early return inside the lock, compute smc once:

	if (!conn || conn->out_of_sync) {
		read_unlock_bh(&lgr->conns_lock);
		return;
	}
	smc = container_of(conn, struct smc_sock, conn);
	sock_hold(&smc->sk);
	read_unlock_bh(&lgr->conns_lock);

Also please initialize smc = NULL at declaration, it's not a bug now
since the early return guards it, just to make it refactor safe.

>  	if (cdc->prod_flags.failover_validation) {
>  		smc_cdc_msg_validate(smc, cdc, link);
> -		return;
> +		goto out;
>  	}
>  	if (smc_cdc_before(ntohs(cdc->seqno),
>  			   conn->local_rx_ctrl.seqno))
>  		/* received seqno is old */
> -		return;
> +		goto out;
>  
>  	smc_cdc_msg_recv(smc, cdc);
> +out:
> +	sock_put(&smc->sk);
>  }
>  
>  static struct smc_wr_rx_handler smc_cdc_rx_handlers[] = {