Re: [PATCH v3 1/3] net/smc: bound the wire-controlled producer cursor to the RMB

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free in connection lookup when receiving CDC messages.
- [High] Data Race on cursor `tx_curs_fin` leading to cursor corruption during link failover.
--

commit 0a889a6cc8b55d508b84bd0b15d47a8711c29162
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/smc: bound the wire-controlled producer cursor to the RMB

This commit bounds the wire-controlled producer cursor count to rmb_desc->len
to prevent out-of-bounds reads in the receive tasklet. It applies this bound
only to the producer cursor to avoid under-crediting peer space.

> diff --git a/net/smc/smc_cdc.h b/net/smc/smc_cdc.h
> index 696cc11f2303b..ca76ef6303566 100644
> --- a/net/smc/smc_cdc.h
> +++ b/net/smc/smc_cdc.h
[ ... ]
> @@ -260,6 +275,12 @@ static inline void smcd_cdc_msg_to_host(struct smc_host_cdc_msg *local,
>  
>  	temp.wrap = peer->prod.wrap;
>  	temp.count = peer->prod.count;
> +	/* the peer producer cursor is wire-controlled and is used as a raw
> +	 * index into our RMB by the urgent path; bound it to the RMB.  The
> +	 * consumer cursor below indexes the peer's RMB and is left unbounded.
> +	 */
> +	if (temp.count > conn->rmb_desc->len)
> +		temp.count = conn->rmb_desc->len;
>  	smc_curs_copy(&local->prod, &temp, conn);
>  
>  	temp.wrap = peer->cons.wrap;

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a
use-after-free to occur in smc_cdc_rx_handler() during connection lookup?

Looking at net/smc/smc_cdc.c:smc_cdc_rx_handler(), the connection pointer is
retrieved under the read lock but accessed after the lock is dropped:

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

It seems a reference to the socket is not acquired before releasing the read
lock.

Could smc_conn_free() concurrently remove the connection and drop the final
socket reference, freeing the memory before the receive tasklet dereferences
conn->out_of_sync and calls smc_cdc_msg_recv() (which then performs
sock_hold() on freed memory)?

[Severity: High]
This is a pre-existing issue, but does the cursor tx_curs_fin suffer from a
data race during link failover?

In net/smc/smc_core.c:smc_switch_cursor(), the cursor is modified under
send_lock without the socket lock:

		smc_curs_add(conn->sndbuf_desc->len,
			     &conn->tx_curs_fin, diff);

Concurrently, net/smc/smc_cdc.c:smc_cdc_tx_handler() overwrites the same
cursor under the socket lock without send_lock:

		smc_curs_copy(&conn->tx_curs_fin, &cdcpend->cursor, conn);

Since smc_curs_add() performs non-atomic field updates on the 64-bit cursor
and they do not share a lock, could this concurrent read-modify-write and
store operation corrupt the cursor's wrap and count fields?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260614-b4-disp-edd64be9-v3-0-551fa514257e@proton.me?part=1