[PATCH] net/smc: bound the peer producer cursor on SMC-D and SMC-R CDC receive

[PATCH] net/smc: bound the peer producer cursor on SMC-D and SMC-R CDC receive

[Bryam Vargas]

The SMC CDC receive path copies the peer's producer cursor -- a
wire-controlled value -- into the local connection cursor with no upper
bound against the receive buffer (RMB). A malicious peer can advertise a
producer cursor past rmb_desc->len, which is then used out of bounds:

 - the urgent path uses the cursor count as a raw index:
   smc_cdc_handle_urg_data_arrival() dereferences
   *(rmb_desc->cpu_addr + rx_off + urg_curs.count - 1);

 - the receive path turns the cursor into a length:
   smc_cdc_msg_recv_action() feeds it to smc_curs_diff() and
   atomic_add()s the result into conn->bytes_to_rcv. The differing-wrap
   branch returns (len - old.count) + new.count, which exceeds len for a
   forged cursor and accumulates across CDCs, so bytes_to_rcv grows past
   rmb_desc->len even when the cursor count itself is bounded;
   smc_rx_recvmsg() then copies the wrap-around second chunk past the RMB.

The RMB is a kernel allocation, so the reads disclose adjacent kernel
memory, and a cursor pointing at an unmapped offset faults in the receive
tasklet (softirq). Both transports are affected: SMC-D converts the
cursor in smcd_cdc_msg_to_host() and SMC-R in smc_cdc_cursor_to_host(),
and neither bounds the count.

Bound the producer cursor count to the RMB at both conversion boundaries
(this closes the urgent index on SMC-D and SMC-R) and enforce the
documented "0 <= bytes_to_rcv <= rmb_desc->len" invariant in
smc_cdc_msg_recv_action() (this closes the receive length, which the
cursor-count clamp alone cannot because of the differing-wrap diff and
the cumulative atomic_add).

Fixes: de8474eb9d50 ("net/smc: urgent data support")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Reproduced under KASAN on 6.12.y. A forged producer cursor with
prod.count = rmb_desc->len + 1 and prod_flags.urg_data_present set produces
a 1-byte out-of-bounds read in smc_cdc_handle_urg_data_arrival()
(slab-out-of-bounds Read of size 1); the cursor-count clamp makes the same
input in-bounds. A wrap-flipped cursor drives bytes_to_rcv past
rmb_desc->len across several CDCs so smc_rx_recvmsg() over-reads; the
bytes_to_rcv invariant keeps it bounded. SMC-D was exercised over the
in-kernel loopback-ism and the SMC-R converter (smc_cdc_cursor_to_host)
over an emulated RDMA loopback. Clamping prod.count alone does not bound
the recv length, hence the separate bytes_to_rcv hunk.

 net/smc/smc_cdc.c |  2 ++
 net/smc/smc_cdc.h | 12 ++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index 619b3bab3824..738c45fd5cd0 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -382,6 +382,8 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
 		smp_mb__before_atomic();
 		atomic_add(diff_prod, &conn->bytes_to_rcv);
 		/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */
+		if (atomic_read(&conn->bytes_to_rcv) > conn->rmb_desc->len)
+			atomic_set(&conn->bytes_to_rcv, conn->rmb_desc->len);
 		smp_mb__after_atomic();
 		smc->sk.sk_data_ready(&smc->sk);
 	} else {
diff --git a/net/smc/smc_cdc.h b/net/smc/smc_cdc.h
index 696cc11f2303..7fa6e0d3817f 100644
--- a/net/smc/smc_cdc.h
+++ b/net/smc/smc_cdc.h
@@ -230,6 +230,12 @@ static inline void smc_cdc_cursor_to_host(union smc_host_cursor *local,
 	smc_curs_copy_net(&net, peer, conn);
 	temp.count = ntohl(net.count);
 	temp.wrap = ntohs(net.wrap);
+	/* the peer producer cursor is wire-controlled; bound the SMC-R count to
+	 * our RMB before it is used as a raw index by the urgent path, mirroring
+	 * the SMC-D conversion in smcd_cdc_msg_to_host().
+	 */
+	if (temp.count > conn->rmb_desc->len)
+		temp.count = conn->rmb_desc->len;
 	if ((old.wrap > temp.wrap) && temp.wrap)
 		return;
 	if ((old.wrap == temp.wrap) &&
@@ -260,6 +266,12 @@ static inline void smcd_cdc_msg_to_host(struct smc_host_cdc_msg *local,
 
 	temp.wrap = peer->prod.wrap;
 	temp.count = peer->prod.count;
+	/* the peer producer cursor is wire-controlled; a count past our RMB is
+	 * used as a raw index by the urgent path (smc_cdc_handle_urg_data_arrival)
+	 * and as a length by the recv path.  Bound it to the RMB.
+	 */
+	if (temp.count > conn->rmb_desc->len)
+		temp.count = conn->rmb_desc->len;
 	smc_curs_copy(&local->prod, &temp, conn);
 
 	temp.wrap = peer->cons.wrap;
-- 
2.43.0

Re: [PATCH] net/smc: bound the peer producer cursor on SMC-D and SMC-R CDC receive

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/smc: bound the peer producer cursor on SMC-D and SMC-R CDC receive

This patch attempts to close an out-of-bounds read in the SMC CDC
receive path by clamping the peer producer cursor and the
bytes_to_rcv accumulator against rmb_desc->len.  A couple of
questions on the approach below.

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index 619b3bab3824..738c45fd5cd0 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c
> @@ -382,6 +382,8 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>  		smp_mb__before_atomic();
>  		atomic_add(diff_prod, &conn->bytes_to_rcv);
>  		/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */
> +		if (atomic_read(&conn->bytes_to_rcv) > conn->rmb_desc->len)
> +			atomic_set(&conn->bytes_to_rcv, conn->rmb_desc->len);

[Critical]
Is this clamp safe against a concurrent smc_rx_recvmsg() on another
CPU?

smc_cdc_msg_recv() runs the BH path under bh_lock_sock() (slock with
BH disabled), while smc_rx_recvmsg() runs under lock_sock(), which
sets sk_lock.owned but releases the slock for the body of the
syscall.  The body contains:

    /* update cursors */
    if (!(flags & MSG_PEEK)) {
        smp_mb__before_atomic();
        atomic_sub(copylen, &conn->bytes_to_rcv);
        smp_mb__after_atomic();
        ...
    }

so the BH on CPU0 and the recvmsg body on CPU1 can run concurrently
on the same connection.

The new clamp here is a non-atomic read-then-set:

    atomic_add(diff_prod, &conn->bytes_to_rcv);
    if (atomic_read(&conn->bytes_to_rcv) > conn->rmb_desc->len)
        atomic_set(&conn->bytes_to_rcv, conn->rmb_desc->len);

With L = rmb_desc->len, the commit message notes that the
differing-wrap branch of smc_curs_diff() can return up to
(L - old.count) + new.count, i.e. close to 2L, even after the count
itself is clamped.  Can the following interleaving happen?

  CPU0 (BH)                                CPU1 (recvmsg)
  atomic_add(diff_prod) -> X (~2L)
                                           readable =
                                             atomic_read(&bytes_to_rcv) = X
                                           copylen = min(remaining, X)
                                           chunk_len = min(copylen,
                                                           L - cons.count)
                                           second chunk = copylen - chunk_len
                                           memcpy_to_msg(rcvbuf_base + 0,
                                                         second_chunk_len)
  atomic_read() -> X
  atomic_set(&bytes_to_rcv, L)

With cons.count near L/2 and copylen near 2L, the second chunk reads
roughly 3L/2 bytes starting at rcvbuf_base + 0, walking past the end
of the RMB allocation.  Is that not the same OOB read this patch is
meant to close?

A second variant of the same race seems to silently corrupt the
counter:

  CPU0 (BH)                                CPU1 (recvmsg)
  atomic_add(diff_prod)        -> X > L
                                           atomic_sub(Y) -> X - Y
  atomic_read()                -> X - Y
  (X - Y still > L?)
  atomic_set(L)

If X - Y is still above L, CPU0 overwrites the consumer's decrement
and bytes_to_rcv ends up at L even though Y bytes were already
consumed, leaving a persistent over-report that future recvmsg calls
use as readable.

smp_mb__after_atomic() does not turn this read-then-set into an RMW;
atomic_set() is a relaxed store.  Would clamping diff_prod against
(L - atomic_read(&bytes_to_rcv)) before the atomic_add (or using an
atomic_cmpxchg loop) close both windows?

>  		smp_mb__after_atomic();
>  		smc->sk.sk_data_ready(&smc->sk);
>  	} else {
> diff --git a/net/smc/smc_cdc.h b/net/smc/smc_cdc.h
> index 696cc11f2303..7fa6e0d3817f 100644
> --- a/net/smc/smc_cdc.h
> +++ b/net/smc/smc_cdc.h
> @@ -230,6 +230,12 @@ static inline void smc_cdc_cursor_to_host(union smc_host_cursor *local,
>  	smc_curs_copy_net(&net, peer, conn);
>  	temp.count = ntohl(net.count);
>  	temp.wrap = ntohs(net.wrap);
> +	/* the peer producer cursor is wire-controlled; bound the SMC-R count to
> +	 * our RMB before it is used as a raw index by the urgent path, mirroring
> +	 * the SMC-D conversion in smcd_cdc_msg_to_host().
> +	 */
> +	if (temp.count > conn->rmb_desc->len)
> +		temp.count = conn->rmb_desc->len;

[High]
Should this clamp live in the shared helper, or only on the prod
call site?

smc_cdc_cursor_to_host() is invoked twice from smcr_cdc_msg_to_host():

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

The producer cursor is bounded by our local RMB (conn->rmb_desc->len),
but the consumer cursor lives in the peer's RMB space and is bounded
by conn->peer_rmbe_size, which is set independently in
smcr_conn_save_peer_info() from the peer's CLC bufsize.

smc_cdc_msg_recv_action() confirms the asymmetry:

    diff_cons = smc_curs_diff(conn->peer_rmbe_size, &cons_old,
                              &conn->local_rx_ctrl.cons);
    ...
    atomic_add(diff_cons, &conn->peer_rmbe_space);

so peer_rmbe_size (not rmb_desc->len) is the right bound for the cons
cursor.

Whenever peer_rmbe_size > rmb_desc->len, legitimate peer cons.count
values in (rmb_desc->len, peer_rmbe_size] are silently truncated to
rmb_desc->len here, diff_cons is under-credited and
peer_rmbe_space stays artificially low, which can stall TX flow
control on perfectly well-behaved peers.

The SMC-D companion in this same patch is structured the way the
prod/cons asymmetry suggests -- it clamps prod and copies cons
unmodified:

> @@ -260,6 +266,12 @@ static inline void smcd_cdc_msg_to_host(struct smc_host_cdc_msg *local,
>  
>  	temp.wrap = peer->prod.wrap;
>  	temp.count = peer->prod.count;
> +	/* the peer producer cursor is wire-controlled; a count past our RMB is
> +	 * used as a raw index by the urgent path (smc_cdc_handle_urg_data_arrival)
> +	 * and as a length by the recv path.  Bound it to the RMB.
> +	 */
> +	if (temp.count > conn->rmb_desc->len)
> +		temp.count = conn->rmb_desc->len;
>  	smc_curs_copy(&local->prod, &temp, conn);
>  
>  	temp.wrap = peer->cons.wrap;

Should the SMC-R hunk be moved out of the shared smc_cdc_cursor_to_host()
helper and onto the prod call site in smcr_cdc_msg_to_host(), to match
the SMC-D shape?
-- 
pw-bot: cr