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

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

Pre-existing issues:
- [High] Time-Of-Check to Time-Of-Use (TOCTOU) vulnerability leading to NULL pointer dereference in CDC receive path.
- [High] Improper teardown ordering allows Use-After-Free of SMC-D send buffer descriptor.
- [High] Premature buffer freeing in `smc_conn_free` causes Use-After-Free of `rmb_desc` during CDC receive.
--

--- Patch [1]: [PATCH net v4] net/smc: order the CDC receive path against buffer publication ---
commit 4253ee2293b7503f954eb700f0d01241f120e824
Author: Bryam Vargas <[email protected]>

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

This commit fixes a vulnerability where SMC CDC receive handlers dereference
unpublished buffer descriptors. It ensures the receive state is initialized
first and the buffers are published last using smp_store_release() and
smp_load_acquire().

Closes: https://sashiko.dev/#/patchset/[email protected]?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
[ ... ]
> @@ -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)) ||
> +	     conn->local_rx_ctrl.prod_flags.cons_curs_upd_req ||
> +	     conn->local_rx_ctrl.prod_flags.urg_data_pending)) {

[Severity: High]
This is a pre-existing issue, but does this create a Time-Of-Check to
Time-Of-Use vulnerability?

The code caches conn->sndbuf_desc into the local variable sndbuf_desc and
checks it for NULL, but then execution continues into smc_tx_prepared_sends(conn).

Looking at smc_tx_prepared_sends() in net/smc/smc_tx.h:

net/smc/smc_tx.h:smc_tx_prepared_sends() {
    union smc_host_cursor sent, prep;

    smc_curs_copy(&sent, &conn->tx_curs_sent, conn);
    smc_curs_copy(&prep, &conn->tx_curs_prep, conn);
    return smc_curs_diff(conn->sndbuf_desc->len, &sent, &prep);
}

Doesn't this helper re-read the global conn->sndbuf_desc? If a concurrent
teardown thread calls smcd_buf_detach() and nullifies it after the local
check, couldn't this lead to a NULL pointer dereference or use-after-free?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but could there be a use-after-free
in the SMC-D send buffer descriptor teardown?

Looking at smc_conn_free() in net/smc/smc_core.c:

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

Because smcd_buf_detach() unconditionally frees conn->sndbuf_desc before
the receive tasklet is killed, and the tasklet runs concurrently in softirq
context, could the tasklet access the freed buffer descriptor if a peer
sends a CDC message during teardown?

[Severity: High]
This is a pre-existing issue, but does the teardown order in smc_conn_free()
allow a use-after-free of the receive buffer?

Looking at smc_conn_free() in net/smc/smc_core.c:

net/smc/smc_core.c:smc_conn_free() {
    ...
    smc_buf_unuse(conn, lgr);
    smc_lgr_unregister_conn(conn);
    ...
}

If the buffer had a registration error, smc_buf_unuse() will free the
rmb_desc. However, the connection is still in the link group's connection
tree at this point.

Could a concurrent IB receive handler (smc_cdc_rx_handler) look up the
connection and extract the dangling conn->rmb_desc pointer before it is
unregistered, leading to a use-after-free when smc_cdc_msg_recv_action()
dereferences it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1