Re: [PATCH net v5 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 3 potential issue(s) to consider:

New issues:
- [High] NULL pointer dereference by sending an early CDC message with a guessed connection token before the connection's receive buffer (`rmb_desc`) is allocated.

Pre-existing issues:
- [Critical] Heap Out-of-Bounds Write due to unbounded consumer cursor inflating `sndbuf_space`.
- [High] A malicious peer can continuously inflate `conn->bytes_to_rcv` beyond the receive buffer size by manipulating the `wrap` field (or incrementally advancing the `count` field) of the producer cursor, leading to a massive out-of-bounds read during `recvmsg`.
--

--- Patch [1]: [PATCH net v5 1/3] net/smc: bound the wire-controlled producer cursor to the RMB ---
commit bb6f7b4d6d71f8888e49e10514b07e9e257bfa1f
Author: Bryam Vargas <hexlabsecurity@proton.me>

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

This patch bounds the producer cursor count to the receive buffer length at
the wire-to-host conversion for both SMC-R and SMC-D. It leaves the consumer
cursor unbounded, as it indexes the peer's RMB and clamping it would stall
transmit to peers with larger RMBs.

> 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

[ ... ]

> @@ -235,6 +236,15 @@ static inline void smc_cdc_cursor_to_host(union smc_host_cursor *local,
>  	if ((old.wrap == temp.wrap) &&
>  	    (old.count > temp.count))
>  		return;
> +	/* The peer producer cursor is wire-controlled and is later used as a
> +	 * raw index into our RMB by the urgent path; bound its count to the
> +	 * RMB.  max_count == 0 leaves the consumer cursor unbounded here: it
> +	 * indexes the peer's RMB (bounded by peer_rmbe_size, not our
> +	 * rmb_desc->len), so clamping it to rmb_desc->len would under-credit
> +	 * peer_rmbe_space and stall transmit to peers with a larger RMB.
> +	 */
> +	if (max_count && temp.count > max_count)
> +		temp.count = max_count;

[Severity: High]
This is a pre-existing issue, but does bounding only the count field leave
the wrap field unchecked?

If a peer sends multiple CDC messages where wrap increments, or count
incrementally advances without exceeding rmb_desc->len, could smc_curs_diff()
compute a positive diff_prod?

net/smc/smc_cdc.c:smc_cdc_msg_recv_action() {
    ...
	diff_prod = smc_curs_diff(conn->rmb_desc->len, &prod_old,
				  &conn->local_rx_ctrl.prod);
	if (diff_prod) {
    ...
		atomic_add(diff_prod, &conn->bytes_to_rcv);
    ...
}

Could this unconditionally inflate conn->bytes_to_rcv beyond the receive
buffer size, causing copylen to exceed the buffer length in smc_rx_recvmsg()?

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

Could this result in an out-of-bounds read of adjacent kernel memory?

>  	smc_curs_copy(local, &temp, conn);
>  }
>  
> @@ -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: High]
Can conn->rmb_desc be NULL here?

If a CDC message is received during connection setup before the receive
buffers are allocated:

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

If the connection is found before smc_buf_create() completes, does
dereferencing conn->rmb_desc->len result in a NULL pointer dereference?

> +	smc_cdc_cursor_to_host(&local->cons, &peer->cons, conn, 0);

[Severity: Critical]
This is a pre-existing issue, but does leaving the consumer cursor unbounded
allow inflating conn->sndbuf_space?

If peer->cons.count is set to a very large value, could smc_curs_diff() return
a large diff_tx?

net/smc/smc_cdc.c:smc_cdc_msg_recv_action() {
    ...
			diff_tx = smc_curs_diff(conn->sndbuf_desc->len,
						&conn->tx_curs_fin,
						&conn->local_rx_ctrl.cons);
			/* increase local sndbuf space and fin_curs */
			smp_mb__before_atomic();
			atomic_add(diff_tx, &conn->sndbuf_space);
    ...
}

When sendmsg() is subsequently called, could this inflated sndbuf_space cause
copylen to be miscalculated?

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

Could this lead to an out-of-bounds write from userspace into the
kernel heap?

>  	local->prod_flags = peer->prod_flags;
>  	local->conn_state_flags = peer->conn_state_flags;
>  }

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-b4-disp-0d07164f-v5-0-6a9e235dbc4e@proton.me?part=1