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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Concurrent unprotected bitfield modifications in `struct smc_connection` can lead to lost updates for `freed`, `killed`, or `out_of_sync` flags, causing reference count underflows and Use-After-Free.
--

commit 2f1e95dfacd6a1bad8a756fc60b2dadb865fcaac
Author: Xiang Mei <xmei5@asu.edu>

net/smc: fix UAF in smc_cdc_rx_handler() by pinning the socket

This commit fixes a use-after-free in smc_cdc_rx_handler() by taking a socket
reference while holding conns_lock. This ensures the connection and socket
remain valid while the receive handler runs.

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index 619b3bab38248..32d6d03df3214 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c

[ ... ]

> @@ -483,21 +483,26 @@ 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);
> -	read_unlock_bh(&lgr->conns_lock);
> -	if (!conn || conn->out_of_sync)
> +	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);
>  
>  	if (cdc->prod_flags.failover_validation) {
>  		smc_cdc_msg_validate(smc, cdc, link);
> -		return;
> +		goto out;

[Severity: Critical]
This is a pre-existing issue, but can concurrent bitfield modifications in
struct smc_connection lead to lost updates and a use-after-free?

In net/smc/smc.h, we have:

struct smc_connection {
	...
	u8			killed : 1;	/* abnormal termination */
	u8			freed : 1;	/* normal termination */
	u8			out_of_sync : 1; /* out of sync with peer */
};

When smc_cdc_msg_validate() is called from softirq (tasklet) context here,
it sets conn->out_of_sync = 1 locklessly.

If a softirq interrupts a process context that is simultaneously setting
conn->freed = 1 in smc_conn_free(), or if they execute concurrently on
different CPUs, the compiler's read-modify-write on the shared byte could
cause one update to overwrite the other.

If the update to conn->freed = 1 is lost, could smc_conn_free() be executed
multiple times on the same connection, bypassing an early if (conn->freed)
check? This seems like it could underflow the link and link-group reference
counts, leading to a use-after-free of those structures.

>  	}
>  	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);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630183227.2044998-1-xmei5@asu.edu?part=1