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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The SMC connection creation path publishes the connection to the link group tree before allocating its receive buffer (`rmb_desc`), creating a race condition where an incoming CDC message can cause a NULL pointer dereference (kernel panic) when accessing `conn->rmb_desc->len`.
- [Critical] The SMC-R asynchronous CDC receive handler looks up a connection from the link group tree but drops the lock without taking a reference, leading to a Use-After-Free if the socket is closed concurrently.
--

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

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

This commit bounds the producer cursor count to the receive buffer length
at the wire-to-host conversion for SMC-R and SMC-D. This prevents
out-of-bounds reads during urgent data handling.

> diff --git a/net/smc/smc_cdc.h b/net/smc/smc_cdc.h
> index 696cc11f2303b..ca76ef6303566 100644
> --- a/net/smc/smc_cdc.h
> +++ b/net/smc/smc_cdc.h

[ ... ]

> @@ -246,8 +256,13 @@ 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);
> +	/* bound the wire-controlled producer cursor to our RMB (used as a raw
> +	 * index by the urgent path); leave the consumer cursor unbounded -- it
> +	 * indexes the peer's RMB and is bounded by peer_rmbe_size.
> +	 */
> +	smc_cdc_cursor_to_host(&local->prod, &peer->prod, conn,
> +			       conn->rmb_desc->len);

[Severity: Critical]
This isn't a bug introduced by this patch, but does this create a risk of a
NULL pointer dereference if the connection receives a CDC message before the
buffer is allocated?

Looking at the connection creation path, the connection is published to the
link group tree before allocating its receive buffer:

net/smc/smc_core.c:smc_conn_create() {
    ...
    rc = smc_lgr_register_conn(conn, true);
    ...
}

If a remote peer sends a CDC message to this connection's token before
smc_buf_create() completes, could the asynchronous handler process it and
dereference conn->rmb_desc->len while conn->rmb_desc is still NULL?

The pre-existing code also unconditionally dereferences this in
smc_cdc_msg_recv_action() via:

    diff_prod = smc_curs_diff(conn->rmb_desc->len, &prod_old, ...);

Could we safely ensure rmb_desc is allocated before it gets accessed in
these paths?

> +	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;
>  }

[Severity: Critical]
This is a pre-existing issue, but looking at the surrounding CDC receive
path, is there a potential use-after-free when handling asynchronous
messages?

In the receive handler, the connection is retrieved using smc_lgr_find_conn()
under a read lock, but the lock is dropped without taking a reference on
the socket:

net/smc/smc_cdc.c:smc_cdc_rx_handler() {
    ...
    read_unlock_bh(&lgr->conns_lock);
    if (!conn || conn->out_of_sync)
        return;
    smc = container_of(conn, struct smc_sock, conn);
    ...
    smc_cdc_msg_recv(smc, cdc);
}

If a local process concurrently closes the socket via smc_conn_free(), which
removes the connection and drops the final reference, could the socket be
freed before smc_cdc_msg_recv() executes and calls sock_hold()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705-b4-disp-28a1bbca-v4-0-be089b98acc6@proton.me?part=1