Re: [PATCH net v5 2/2] net/smc: skip the DMB nocopy accounting until the send buffer is set

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net/smc: state corruption from reused DMB peer CDC messages
--

commit 9ad2bd35a07a32a00da8818fbbfd176e9bdd1557
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/smc: skip the DMB nocopy accounting until the send buffer is set

This patch mitigates a null pointer dereference in smc-d dmb nocopy when an
old peer reusing a dmb sends a cdc message before the new connection sets
sndbuf_desc. It skips nocopy accounting while the send buffer is unset.

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index 7bf902ebd7ab6..d6825f48f5688 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c
> @@ -333,6 +333,7 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>  	union smc_host_cursor cons_old, prod_old;
>  	struct smc_connection *conn = &smc->conn;
>  	int diff_cons, diff_prod, diff_tx;
> +	struct smc_buf_desc *sndbuf_desc;
>  
>  	smc_curs_copy(&prod_old, &conn->local_rx_ctrl.prod, conn);
>  	smc_curs_copy(&cons_old, &conn->local_rx_ctrl.cons, conn);

[Severity: High]
Does this code unconditionally overwrite the new connection's receive control
block with the old peer's cursors?

Because the old peer of a reused dmb can still send cdc messages, and these
messages lack sequence numbers or token fields, old updates are blindly
processed.

Looking right after this block in smc_cdc_msg_recv_action(),
smc_cdc_msg_to_host() is called to update conn->local_rx_ctrl. Then
diff_cons is calculated from the old cursors and added to
conn->peer_rmbe_space.

Could this regression corrupt the peer's receive window space by adding a
bogus difference?

[ ... ]

> @@ -353,12 +354,19 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>  		 * peer RMB, then update tx_curs_fin and sndbuf_space
>  		 * here since peer has already consumed the data.
>  		 */
> +		sndbuf_desc = NULL;
>  		if (conn->lgr->is_smcd &&
>  		    smc_ism_support_dmb_nocopy(conn->lgr->smcd)) {
> +			/* NULL until smcd_buf_attach(), but the old peer of
> +			 * a reused DMB can already signal us.
> +			 */
> +			sndbuf_desc = smp_load_acquire(&conn->sndbuf_desc);
> +		}
> +		if (sndbuf_desc) {
>  			/* Calculate consumed data and
>  			 * increment free send buffer space.
>  			 */
> -			diff_tx = smc_curs_diff(conn->sndbuf_desc->len,
> +			diff_tx = smc_curs_diff(sndbuf_desc->len,
>  						&conn->tx_curs_fin,
>  						&conn->local_rx_ctrl.cons);

[Severity: High]
If the old peer sends an update after the new connection has already set
sndbuf_desc, is this check bypassed?

If so, could diff_tx be calculated from the old peer's cursors and added
to conn->sndbuf_space, corrupting the send buffer space?

Furthermore, looking at the end of smc_cdc_msg_recv_action(), diff_prod is
calculated and added to conn->bytes_to_rcv, and the socket is woken up via
sk_data_ready(). By avoiding the null pointer dereference, does this
regression instead corrupt the available data size and trick the new
connection into reading garbage data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-b4-disp-2a8d85ef-v5-0-6b795e8263c9@proton.me?part=2