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

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

[Nguyen Le Thanh Tung]

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;
-- 
2.53.0

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

[Sidraya Jayagond]

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;