Re: [PATCH net v6 2/3] net/smc: clear the buffer pointers before registering a connection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] net/smc: use-after-free in smc_cdc_msg_recv_action() when
  rmb_desc is cleared in smc_conn_create()
--

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

net/smc: clear the buffer pointers before registering a connection

This commit clears conn->rmb_desc and conn->sndbuf_desc under the socket
spinlock in smc_conn_create() and checks conn->rmb_desc in
smc_cdc_msg_recv_action() to avoid using stale buffer pointers after a
failed handshake attempt.

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index ee5ba6c7a5b4a..b3bcc206efe3e 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c
> @@ -334,6 +334,10 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>  	struct smc_connection *conn = &smc->conn;
>  	int diff_cons, diff_prod, diff_tx;
>  
> +	/* smc_conn_create() can have cleared rmb_desc */
> +	if (!smp_load_acquire(&conn->rmb_desc))
> +		return;
> +
>  	smc_curs_copy(&prod_old, &conn->local_rx_ctrl.prod, conn);
>  	smc_curs_copy(&cons_old, &conn->local_rx_ctrl.cons, conn);
>  	smc_cdc_msg_to_host(&conn->local_rx_ctrl, cdc, conn);
[ ... ]
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 02269cede641e..9ef9019bc4974 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -2024,6 +2024,15 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
>  	spinlock_t *lgr_lock;
>  	int rc = 0;
>  
> +	/* A retried handshake comes back with the buffer pointers of the
> +	 * failed attempt still set: clear them before the connection is
> +	 * registered. smc_cdc_msg_recv_action() tests rmb_desc under this lock.
> +	 */
> +	spin_lock_bh(&smc->sk.sk_lock.slock);
> +	WRITE_ONCE(conn->rmb_desc, NULL);
> +	WRITE_ONCE(conn->sndbuf_desc, NULL);
> +	spin_unlock_bh(&smc->sk.sk_lock.slock);
> +
>  	lgr_list = ini->is_smcd ? &ini->ism_dev[ini->ism_selected]->lgr_list :
>  				  &smc_lgr_list.list;
>  	lgr_lock = ini->is_smcd ? &ini->ism_dev[ini->ism_selected]->lgr_lock :

[Severity: High]
Can smc_cdc_msg_recv_action() still dereference freed conn->rmb_desc,
conn->sndbuf_desc and conn->lgr when the buffer pointers are cleared in
smc_conn_create() instead of smc_conn_free()?

In smc_conn_free(), smc_buf_unuse() releases or frees the buffers before
smc_lgr_unregister_conn() removes the connection from lgr->conns_all, and
smc_lgr_put() and smc_lgr_cleanup_early() then drop the link group:

net/smc/smc_core.c:smc_conn_free() {
	...
	if (!list_empty(&lgr->list))
		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
	smc_lgr_unregister_conn(conn);
	...
	smc_lgr_put(lgr); /* lgr_hold in smc_conn_create() */
}

Because bh_lock_sock(&smc->sk) in smc_cdc_msg_recv() only acquires
sk_lock.slock without waiting for process-context lock_sock(), an
incoming CDC message can run smc_cdc_msg_recv_action() before
smc_conn_create() is called (or when no retry occurs, such as when the
handshake aborts without a fallback device):

kworker                                   <softirq>
smc_listen_rdma_reg()
  ...
                                          smc_cdc_rx_handler()
                                            read_lock_bh(&lgr->conns_lock)
                                            conn = smc_lgr_find_conn(...)
                                            !smp_load_acquire(&conn->rmb_desc)
                                            sock_hold(&smc->sk)
                                            read_unlock_bh(&lgr->conns_lock)
  smc_conn_abort()
    smc_conn_free()
      smc_buf_unuse()
        smc_buf_free() -> kfree(rmb_desc)
      smc_lgr_unregister_conn()
      smc_lgr_put()
    smc_lgr_cleanup_early()
                                            smc_cdc_msg_recv()
                                              bh_lock_sock(&smc->sk)
                                              smc_cdc_msg_recv_action()
                                                !smp_load_acquire(&conn->rmb_desc)
                                                conn->rmb_desc->len
                                                  <-- use-after-free

Even when a retry does reach smc_conn_create(), if the in-flight
smc_cdc_rx_handler() acquires bh_lock_sock(&smc->sk) after the retry has
called __smc_buf_create(), conn->rmb_desc is non-NULL again and
smc_cdc_msg_recv_action() applies the stale CDC from the aborted attempt
to the new connection.

Clearing conn->rmb_desc and conn->sndbuf_desc under sk_lock.slock in
smc_conn_free() after smc_lgr_unregister_conn() and before
smc_buf_unuse() would close both windows.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-b4-disp-1c8d3a53-v6-0-36a38ae35021@proton.me?part=2