Re: [PATCH net] net/smc: clamp peer supplied CDC cursors to the buffer size

[Sidraya Jayagond <sidraya@linux.ibm.com>]

Hi Nguyen,
I think Bryam already sent a similar patch to the mailing-list.

https://lore.kernel.org/netdev/20260723-b4-disp-0d07164f-v5-1-6a9e235dbc4e@proton.me/

On 27/07/26 9:38 am, Nguyen Le Thanh Tung wrote:
> smc_cdc_cursor_to_host() converts a peer supplied CDC cursor into the
> connection's host cursor.  The count field is taken verbatim off the
> wire and is only compared against the previously known cursor to
> enforce monotonicity - it is never validated against the size of the
> buffer it indexes:
> 
> 	temp.count = ntohl(net.count);
> 	temp.wrap  = ntohs(net.wrap);
> 	if ((old.wrap > temp.wrap) && temp.wrap)
> 		return;
> 	if ((old.wrap == temp.wrap) && (old.count > temp.count))
> 		return;
> 	smc_curs_copy(local, &temp, conn);
> 
> On the first CDC message of a connection old is {wrap = 0, count = 0},
> so both guards are false for any peer chosen pair and a count of up to
> 0xffffffff is accepted unconditionally.  A conforming peer never emits
> count >= size: smc_curs_add() wraps at the buffer size, which is what
> every consumer of the cursor assumes.
> 
> The most direct consequence is in smc_cdc_handle_urg_data_arrival(),
> which uses the producer cursor as a raw index with no further check:
> 
> 	base = (char *)conn->rmb_desc->cpu_addr + conn->rx_off;
> 	if (conn->urg_curs.count)
> 		conn->urg_rx_byte = *(base + conn->urg_curs.count - 1);
> 
> That path is reached from the receive path whenever the producer cursor
> advances and prod_flags.urg_data_present is set - a single bit copied
> verbatim from the wire - so a peer can read at an attacker chosen offset
> of up to 4GB from a known heap pointer, far outside the RMB.  The same
> unvalidated cursor also feeds the atomic_add() into conn->bytes_to_rcv,
> breaking the 0 <= bytes_to_rcv <= rmb_desc->len invariant that
> smc_rx_recvmsg() relies on when it computes its second copy chunk.
> 
> Clamp count to the size of the buffer the cursor refers to, so that
> everything derived from it stays inside that buffer.  The producer
> cursor indexes the local RMB and is bounded by conn->rmb_desc->len, the
> consumer cursor refers to the peer's RMBE and is bounded by
> conn->peer_rmbe_size - the same sizes smc_cdc_msg_recv_action() already
> passes to smc_curs_diff() for these two cursors, and which it already
> dereferences unconditionally a few lines after the conversion, so no new
> pointer is assumed valid here.  The SMC-D path reads the cursors from
> the DMB rather than off the wire, but they are equally peer controlled
> and reach the same consumers, so clamp there as well.  For SMC-D the DMB
> is registered with sizeof(struct smcd_cdc_msg) extra bytes and
> buf_desc->len holds the remainder (smcd_new_buf_create()), so bounding
> count by rmb_desc->len also keeps the rx_off based dereference inside
> the allocation.
> 
> Found by code inspection.  The unbounded acceptance was confirmed by
> calling the unmodified smc_cdc_cursor_to_host() from a module with
> old = {0, 0} and a peer cursor of count = 0x40000000: the value is
> copied through unchanged.  Replaying the resulting dereference against a
> real 64 byte kmalloc() allocation produces a clean KASAN
> slab-out-of-bounds read.
> 
> Fixes: 5f08318f617b ("smc: connection data control (CDC)")
> Signed-off-by: Nguyen Le Thanh Tung <nguyenlethanhtung555@gmail.com>
> ---
> Compile tested on 6.8.12 with CONFIG_SMC=m: `make W=1 net/smc/` produces
> no new warnings.  The two functions this patch touches are byte identical
> in 6.8 and current mainline.
> 
>  net/smc/smc_cdc.h | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/net/smc/smc_cdc.h b/net/smc/smc_cdc.h
> index 696cc11..a71e46d 100644
> --- a/net/smc/smc_cdc.h
> +++ b/net/smc/smc_cdc.h
> @@ -219,16 +219,34 @@ static inline void smc_host_msg_to_cdc(struct smc_cdc_msg *peer,
>  	peer->conn_state_flags = local->conn_state_flags;
>  }
>  
> +/* Bound a peer supplied cursor count to the size of the buffer it indexes.
> + * A conforming peer never sends count >= size (see smc_curs_add()), and every
> + * user of the cursor - the byte accounting in smc_cdc_msg_recv_action() and
> + * the direct RMB dereference in smc_cdc_handle_urg_data_arrival() - relies on
> + * that. Nothing validates it, so do it here, once, where the value enters the
> + * connection state.
> + */
> +static inline u32 smc_cdc_clamp_count(u32 count, unsigned int size)
> +{
> +	if (WARN_ON_ONCE(size && count >= size))
> +		count = size - 1;
> +	return count;
> +}
> +
> +/* @size: size of the buffer the cursor indexes, i.e. conn->rmb_desc->len for
> + *	  a producer cursor and conn->peer_rmbe_size for a consumer cursor
> + */
>  static inline void smc_cdc_cursor_to_host(union smc_host_cursor *local,
>  					  union smc_cdc_cursor *peer,
> -					  struct smc_connection *conn)
> +					  struct smc_connection *conn,
> +					  unsigned int size)
>  {
>  	union smc_host_cursor temp, old;
>  	union smc_cdc_cursor net;
>  
>  	smc_curs_copy(&old, local, conn);
>  	smc_curs_copy_net(&net, peer, conn);
> -	temp.count = ntohl(net.count);
> +	temp.count = smc_cdc_clamp_count(ntohl(net.count), size);
>  	temp.wrap = ntohs(net.wrap);
>  	if ((old.wrap > temp.wrap) && temp.wrap)
>  		return;
> @@ -246,8 +264,10 @@ 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);
> +	smc_cdc_cursor_to_host(&local->prod, &peer->prod, conn,
> +			       conn->rmb_desc->len);
> +	smc_cdc_cursor_to_host(&local->cons, &peer->cons, conn,
> +			       conn->peer_rmbe_size);
>  	local->prod_flags = peer->prod_flags;
>  	local->conn_state_flags = peer->conn_state_flags;
>  }
> @@ -259,11 +279,13 @@ static inline void smcd_cdc_msg_to_host(struct smc_host_cdc_msg *local,
>  	union smc_host_cursor temp;
>  
>  	temp.wrap = peer->prod.wrap;
> -	temp.count = peer->prod.count;
> +	temp.count = smc_cdc_clamp_count(peer->prod.count,
> +					 conn->rmb_desc->len);
>  	smc_curs_copy(&local->prod, &temp, conn);
>  
>  	temp.wrap = peer->cons.wrap;
> -	temp.count = peer->cons.count;
> +	temp.count = smc_cdc_clamp_count(peer->cons.count,
> +					 conn->peer_rmbe_size);
>  	smc_curs_copy(&local->cons, &temp, conn);
>  	local->prod_flags = peer->cons.prod_flags;
>  	local->conn_state_flags = peer->cons.conn_state_flags;