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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net/smc: NULL pointer dereference in SMC CDC receive path
--

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

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

smcr_cdc_msg_to_host() and smcd_cdc_msg_to_host() import the peer's
producer cursor from the wire into conn->local_rx_ctrl.prod without
bounding it to the RMB. This patch bounds the producer cursor count
to rmb_desc->len when it comes off the wire for both SMC-R and SMC-D.

> diff --git a/net/smc/smc_cdc.h b/net/smc/smc_cdc.h
> index 696cc11f2303b..4d630a3051184 100644
> --- a/net/smc/smc_cdc.h
> +++ b/net/smc/smc_cdc.h
[ ... ]
> @@ -246,8 +253,9 @@ static inline void smcr_cdc_msg_to_host(struct smc_host_cdc_msg *local,
>  	local->len = peer->len;
>  	local->seqno = ntohs(peer->seqno);
>  	local->token = ntohl(peer->token);
> -	smc_cdc_cursor_to_host(&local->prod, &peer->prod, conn);
> -	smc_cdc_cursor_to_host(&local->cons, &peer->cons, conn);
> +	smc_cdc_cursor_to_host(&local->prod, &peer->prod, conn,
> +			       conn->rmb_desc->len);

[Severity: High]
Is it possible for conn->rmb_desc to be NULL here?

If an existing SMC-R link group is reused for a new connection,
smc_conn_create() inserts the connection into the globally visible
lgr->conns_all RB-tree via smc_lgr_register_conn(conn, true) before
the rmb_desc buffer is allocated by smc_buf_create().

Since the QP is already in RTS state, could a malicious remote peer sharing
the link group predict the new connection's alert_token_local (which is a
predictable atomic counter) and send a CDC message during this race window?

If so, smc_cdc_rx_handler() would find the connection and call
smcr_cdc_msg_to_host() without checking sk_state or if conn->rmb_desc is
allocated, leading to a NULL pointer dereference. Note that bh_lock_sock()
does not block softirqs from pre-empting or running concurrently with the
process-context lock_sock() held during setup.

> +	smc_cdc_cursor_to_host(&local->cons, &peer->cons, conn, 0);
>  	local->prod_flags = peer->prod_flags;
>  	local->conn_state_flags = peer->conn_state_flags;
>  }
> @@ -260,6 +268,9 @@ static inline void smcd_cdc_msg_to_host(struct smc_host_cdc_msg *local,
>  
>  	temp.wrap = peer->prod.wrap;
>  	temp.count = peer->prod.count;
> +	/* as in smc_cdc_cursor_to_host(): bound prod, not cons */
> +	if (temp.count > conn->rmb_desc->len)
> +		temp.count = conn->rmb_desc->len;

[Severity: High]
Would the same race window affect SMC-D connections in smcd_cdc_msg_to_host(),
triggering a NULL pointer dereference if a CDC message is received before
the rmb_desc buffer is allocated?

>  	smc_curs_copy(&local->prod, &temp, conn);
>  
>  	temp.wrap = peer->cons.wrap;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-b4-disp-f345196c-v7-0-ef3dddfbfa68@proton.me?part=1