Re: [PATCH net v3] net/smc: order the CDC receive path against buffer publication

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unvalidated CDC message cursor counts lead to remote Out-of-Bounds Read and Write in kernel memory.
- [High] Use-After-Free due to connection teardown destroying and freeing buffers before ensuring concurrent receive tasklets or handlers have finished.
- [High] TOCTOU NULL pointer dereference introduced by incomplete caching of `sndbuf_desc`.
--

--- Patch [1]: [PATCH net v3] net/smc: order the CDC receive path against buffer publication ---
commit 53d359a16d34c960781b7b6d70a6d71513349061
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/smc: order the CDC receive path against buffer publication

This commit addresses orderings in the SMC CDC receive path against buffer
publication. It initializes the receive state first and publishes buffers
using smp_store_release(). Handlers consume buffers with smp_load_acquire()
to avoid dereferencing unset pointers.

Closes: https://sashiko.dev/#/patchset/20260714-b4-disp-835288a6-v2-1-581555ef2145@proton.me?part=1

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index 32d6d03df3214..ea61b1e75c72c 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c
[ ... ]
> @@ -351,14 +363,17 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>  
>  		/* if local sndbuf shares the same memory region with
>  		 * peer RMB, then update tx_curs_fin and sndbuf_space
> -		 * here since peer has already consumed the data.
> +		 * here since peer has already consumed the data.  The ghost
> +		 * sndbuf_desc (acquired above) may still be unset in the SMC-D
> +		 * DMB-nocopy setup window, so skip the update while it is NULL.
>  		 */
>  		if (conn->lgr->is_smcd &&
> -		    smc_ism_support_dmb_nocopy(conn->lgr->smcd)) {
> +		    smc_ism_support_dmb_nocopy(conn->lgr->smcd) &&
> +		    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: Critical]
This is a pre-existing issue, but does this cursor difference calculation lack
upper bounds checking?

A peer can send a CDC message with an inflated cursor count value.

Since smc_curs_diff() returns max_t(int, 0, (new->count - old->count)),
the returned difference can be arbitrarily large.

This difference is then applied to sndbuf_space and bytes_to_rcv.

Could an inflated bytes_to_rcv value cause the remainder chunk_len to exceed
the buffer size, resulting in an out-of-bounds read?

net/smc/smc_rx.c:smc_rx_recvmsg() {
    ...
    chunk_len = copylen - chunk_len;
    ...
    rc = memcpy_to_msg(msg, rcvbuf_base + chunk_off, chunk_len);
}

Similarly, could an inflated sndbuf_space lead to an out-of-bounds write
when calculating the second chunk length?

net/smc/smc_tx.c:smc_tx_sendmsg() {
    ...
    rc = memcpy_from_msg(sndbuf_base + chunk_off, msg, chunk_len);
}

>  			/* increase local sndbuf space and fin_curs */
[ ... ]
> @@ -391,10 +406,15 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>  			conn->urg_state = SMC_URG_NOTYET;
>  	}
>  
> -	/* trigger sndbuf consumer: RDMA write into peer RMBE and CDC */
> -	if ((diff_cons && smc_tx_prepared_sends(conn)) ||
> -	    conn->local_rx_ctrl.prod_flags.cons_curs_upd_req ||
> -	    conn->local_rx_ctrl.prod_flags.urg_data_pending) {
> +	/* trigger sndbuf consumer: RDMA write into peer RMBE and CDC.
> +	 * smc_tx_prepared_sends() and smc_tx_pending() dereference sndbuf_desc,
> +	 * so skip the whole trigger while it is unset (the SMC-D DMB-nocopy
> +	 * setup window): there is nothing to send without a send buffer.
> +	 */
> +	if (sndbuf_desc &&
> +	    ((diff_cons && smc_tx_prepared_sends(conn)) ||

[Severity: High]
This isn't a bug introduced by this patch, but could this still lead to a null
pointer dereference?

The condition checks the locally acquired sndbuf_desc, but
smc_tx_prepared_sends() fetches conn->sndbuf_desc again directly:

net/smc/smc_tx.h:smc_tx_prepared_sends() {
    ...
    return smc_curs_diff(conn->sndbuf_desc->len, &sent, &prep);
}

If a concurrent teardown clears conn->sndbuf_desc right after the local
sndbuf_desc check passes, does smc_tx_prepared_sends() dereference null?

> +	     conn->local_rx_ctrl.prod_flags.cons_curs_upd_req ||
> +	     conn->local_rx_ctrl.prod_flags.urg_data_pending)) {
>  		if (!sock_owned_by_user(&smc->sk))
>  			smc_tx_pending(conn);
>  		else
[ ... ]
> @@ -443,13 +463,21 @@ static void smcd_cdc_rx_tsklet(struct tasklet_struct *t)
>  {
>  	struct smc_connection *conn = from_tasklet(conn, t, rx_tsklet);
>  	struct smcd_cdc_msg *data_cdc;
> +	struct smc_buf_desc *rmb_desc;
>  	struct smcd_cdc_msg cdc;
>  	struct smc_sock *smc;
>  
>  	if (!conn || conn->killed)
>  		return;
> +	/* Pair with smp_store_release() in __smc_buf_create(): the connection
> +	 * is published before its RMB is allocated, so bail while rmb_desc is
> +	 * unset to avoid a NULL deref and a load of an uninitialised buffer.
> +	 */
> +	rmb_desc = smp_load_acquire(&conn->rmb_desc);
> +	if (!rmb_desc)
> +		return;

[Severity: High]
This isn't a bug introduced by this patch, but can the receive tasklet race
with connection teardown here?

In smc_conn_kill(), the buffer is detached before the tasklet is stopped:

net/smc/smc_core.c:smc_conn_kill() {
    ...
    if (smc_ism_support_dmb_nocopy(conn->lgr->smcd))
        smcd_buf_detach(conn);
    if (soft)
        tasklet_kill(&conn->rx_tsklet);
}

Additionally, in smc_conn_free(), the buffer is marked for reuse before the
connection is removed from the link group tree:

net/smc/smc_core.c:smc_conn_free() {
    ...
    if (!list_empty(&lgr->list)) {
        smc_buf_unuse(conn, lgr);
        smc_lgr_unregister_conn(conn);
    }
}

Could these sequences allow a concurrent receive tasklet or handler to acquire
the connection and access a freed or reused buffer descriptor?

>  
> -	data_cdc = (struct smcd_cdc_msg *)conn->rmb_desc->cpu_addr;
> +	data_cdc = (struct smcd_cdc_msg *)rmb_desc->cpu_addr;
>  	smcd_curs_copy(&cdc.prod, &data_cdc->prod, conn);
>  	smcd_curs_copy(&cdc.cons, &data_cdc->cons, conn);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-b4-disp-aa52955a-v3-1-03a4411a7549@proton.me?part=1