[PATCH net v6 0/3] net/smc: bound wire-controlled CDC cursors against the local buffers

[PATCH net v6 0/3] net/smc: bound wire-controlled CDC cursors against the local buffers

[Bryam Vargas via B4 Relay]

A peer's CDC producer/consumer cursors are copied from the wire and used,
without an upper bound against the local buffers, as (a) a raw index into the
RMB on the urgent path, (b) the receive length in smc_rx_recvmsg(), and (c) the
send length in smc_tx_sendmsg() on the SMC-D DMB-merge path. A malicious or
buggy peer can forge a cursor so each runs past the relevant buffer: on the
rx/urgent side an OOB read of adjacent kernel memory, returned to the
receiving process; on the tx side an OOB write whose length the peer
controls and whose overflowing bytes are the local sender's own outbound
data.

This series bounds each length where it is consumed. Each clamp checks the
local copy of the counter that the copy length is then taken from, so the
tasklet advancing the counter cannot get between the check and the use.

The clamp is not subsumed by validating each cursor at the input boundary. A
peer that only increments prod.wrap with count == 0 hits the differing-wrap
branch of smc_curs_diff(), which returns (len - 0) + 0 == len every CDC, so
bytes_to_rcv (and sndbuf_space on the send side) accumulates past the buffer
while every per-cursor bound sees count == 0 and accepts the message. IOW the
overflow lives in the accumulator, not the cursor. Hidayath Khan's "net/smc:
abort the connection when the peer overruns the RMB" rejects that accumulation
on the receive side at the CDC boundary; these clamps bound each consumer and
do not depend on it.

The nearby readable >= rmb_desc->len / len > sndbuf_desc->len tests only feed
stats counters (SMC_STAT_RMB_RX_FULL / SMC_STAT_RMB_TX_SIZE_SMALL) on an
earlier, separate read; they do not bound the copy.

A/B (in-kernel KASAN replaying the sink arithmetic over a real rmb_desc->len /
sndbuf_desc->len slab; kasan.fault=report kasan_multi_shot, 2026-07-05):
  - urgent index (1/3):  count = len+1        -> slab-out-of-bounds Read;  clamped -> clean
  - recv length  (2/3):  bytes_to_rcv = 5*len via wrap++/count=0 -> OOB Read; clamped -> clean
  - send length  (3/3):  sndbuf_space inflated -> slab-out-of-bounds Write; clamped -> clean
  - signed overflow:     readable = -1 -> v1 ">len" misses -> OOB; "<0 || >len" -> clean
  - concurrent TOCTOU race: a producer-side clamp is racy (OOB in a racing consumer
    kthread on another CPU); the consumer-side clamp: 0 hits in 5,000,000 reads.
  - every in-bounds / honest-peer arm: clean.
End to end over SMC-D loopback, re-run 2026-10-08 on v7.3-rc6 with this
series applied (two AF_SMC sockets, the sender forging wrap++/count=0,
rmb_desc->len 65504; the bound comes from 2/3 itself, not a modeled clamp):
bytes_to_rcv reaches 6*len in both arms. Unpatched, recv() returns 393024
and the second chunk is a 327520-byte read from ring offset 0 whose last
262016 bytes lie past the RMB; KASAN reports it from smc_rx_recvmsg as
use-after-free in _copy_to_iter (the 2026-07-11 run on 7.2-rc1 read the
same bytes and was labelled slab-use-after-free -- the label follows
whatever sits after the RMB). Patched, recv() returns 65504 and KASAN
stays quiet; an honest transfer is clean.

Changes since v5:
  - repost only: rebased on net (v7.3-rc6), no functional change. Carries Sidraya
    Jayagond's Reviewed-by on all three patches, alongside Dust Li's.
  - 2/3 commit message: the over-read is returned to the receiving process,
    not disclosed to the peer as v5 said. Commit message only.
  - end-to-end A/B re-run on v7.3-rc6 with the v6 patches themselves as the
    fixed arm (above).
  - cover: the receive-side accumulator abort is Hidayath Khan's patch, linked
    below; the v5 cover described it as a separate series of mine.
  v5: https://lore.kernel.org/all/20260723-b4-disp-0d07164f-v5-0-6a9e235dbc4e@proton.me/
  abort: https://lore.kernel.org/all/20260804141109.542202-1-hidayath@linux.ibm.com/

Changes since v4:
  - repost only: the netdev patch queue overflowed, so v5 reposted the v4
    series unchanged, carrying Dust Li's Reviewed-by on all three patches.
  v4: https://lore.kernel.org/all/20260705-b4-disp-28a1bbca-v4-0-be089b98acc6@proton.me/

Changes since v3:
  - split into this stable-bound clamp series and a separate validate/abort
    change, per Dust Li's review;
  - tightened the commit messages; noted that the nearby SMC_STAT_* tests are not
    bounds; no functional change to the three clamps.
  v3: https://lore.kernel.org/all/20260614-b4-disp-edd64be9-v3-0-551fa514257e@proton.me/

Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Bryam Vargas (3):
      net/smc: bound the wire-controlled producer cursor to the RMB
      net/smc: bound the receive length to the RMB in smc_rx_recvmsg()
      net/smc: bound the send length to the send buffer in smc_tx_sendmsg()

 net/smc/smc_cdc.h | 27 ++++++++++++++++++++++++---
 net/smc/smc_rx.c  | 12 ++++++++++++
 net/smc/smc_tx.c  | 13 +++++++++++++
 3 files changed, 49 insertions(+), 3 deletions(-)
---
base-commit: 6d25ffca055a77787c21a36b66c253f76239411b
change-id: 20261008-b4-disp-f7cd3d96-ff529d85131e

Best regards,
--  
Bryam Vargas <hexlabsecurity@proton.me>

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

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

smcr_cdc_msg_to_host() and smcd_cdc_msg_to_host() import a peer's
producer cursor from the wire into conn->local_rx_ctrl.prod without
bounding it against the receive buffer. The urgent-data path in
smc_cdc_msg_recv_action() then uses that count as a raw index into the
RMB, so a peer that advertises a producer cursor past rmb_desc->len
reads out of bounds of the RMB allocation in the receive tasklet and
can disclose adjacent kernel memory.

Bound the producer cursor count to rmb_desc->len at the wire-to-host
conversion, for both SMC-R and SMC-D. Bound only the producer cursor:
the consumer cursor indexes the peer's RMB and is bounded by
peer_rmbe_size, so clamping it to our rmb_desc->len would under-credit
peer_rmbe_space and stall transmit to a peer with a larger RMB.
Conforming peers are unaffected.

Fixes: de8474eb9d50 ("net/smc: urgent data support")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
Reviewed-by: Dust Li <dust.li@linux.alibaba.com>
Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
---
 net/smc/smc_cdc.h | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/net/smc/smc_cdc.h b/net/smc/smc_cdc.h
index 696cc11f2303..ca76ef630356 100644
--- a/net/smc/smc_cdc.h
+++ b/net/smc/smc_cdc.h
@@ -221,7 +221,8 @@ static inline void smc_host_msg_to_cdc(struct smc_cdc_msg *peer,
 
 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,
+					  int max_count)
 {
 	union smc_host_cursor temp, old;
 	union smc_cdc_cursor net;
@@ -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;
 	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);
+	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;
 }
@@ -260,6 +275,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 and is used as a raw
+	 * index into our RMB by the urgent path; bound it to the RMB.  The
+	 * consumer cursor below indexes the peer's RMB and is left unbounded.
+	 */
+	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.56.0

[PATCH net v6 2/3] net/smc: bound the receive length to the RMB in smc_rx_recvmsg()

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

conn->bytes_to_rcv is accumulated in the receive tasklet from the peer's
wire-controlled producer cursor via smc_curs_diff(), whose
differing-wrap branch can exceed rmb_desc->len; a forged cursor drives
bytes_to_rcv past the RMB, and over many CDC messages overflows the
signed counter negative. smc_rx_recvmsg() reads it as the readable
length and does a wrap-around copy whose second chunk is not re-bounded
to rmb_desc->len, reading past the RMB into adjacent kernel memory and
returning it to the receiving process. The nearby readable >=
rmb_desc->len test only feeds SMC_STAT_RMB_RX_FULL on a separate earlier
read; it does not bound the copy.

Bound the readable length to rmb_desc->len at the consumer, treating a
negative (sign-overflowed) value as out of range too, so the copy can
never exceed the ring. This enforces the documented
0 <= bytes_to_rcv <= rmb_desc->len invariant where it is race-free
against the producer update in the tasklet; conforming peers are
unaffected.

Fixes: 952310ccf2d8 ("smc: receive data from RMBE")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
Reviewed-by: Dust Li <dust.li@linux.alibaba.com>
Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
---
 net/smc/smc_rx.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
index 197fddc6271f..f781d7229185 100644
--- a/net/smc/smc_rx.c
+++ b/net/smc/smc_rx.c
@@ -442,6 +442,18 @@ int smc_rx_recvmsg(struct smc_sock *smc, struct msghdr *msg,
 		/* initialize variables for 1st iteration of subsequent loop */
 		/* could be just 1 byte, even after waiting on data above */
 		readable = smc_rx_data_available(conn, peeked_bytes);
+		/* bytes_to_rcv is accumulated from the peer's wire-controlled
+		 * producer cursor; a forged cursor can drive it past the RMB,
+		 * or overflow the signed accumulator to a negative value across
+		 * many CDC messages (which a plain "> len" check would miss
+		 * before the size_t cast below turns it huge).  Bound it to the
+		 * RMB in either case so the wrap-around copy cannot run past
+		 * rmb_desc->len.  This enforces the documented
+		 * 0 <= bytes_to_rcv <= rmb_desc->len invariant at the consumer,
+		 * race-free against the producer update in the receive tasklet.
+		 */
+		if (readable < 0 || readable > conn->rmb_desc->len)
+			readable = conn->rmb_desc->len;
 		splbytes = atomic_read(&conn->splice_pending);
 		if (!readable || (msg && splbytes)) {
 			if (splbytes)

-- 
2.56.0

[PATCH net v6 3/3] net/smc: bound the send length to the send buffer in smc_tx_sendmsg()

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

On the SMC-D DMB-merge (nocopy) path, smc_cdc_msg_recv_action()
advances conn->sndbuf_space from the peer's wire-controlled consumer
cursor via smc_curs_diff(), which can return more than sndbuf_desc->len;
a forged cursor drives sndbuf_space past the send buffer, and over many
CDC messages overflows the signed counter negative. smc_tx_sendmsg()
reads it as the write space and does a wrap-around copy whose second
chunk is not re-bounded to sndbuf_desc->len, spilling the local
sender's outbound data past the send buffer at a peer-controlled
length: a heap out-of-bounds write. The nearby len > sndbuf_desc->len
test only feeds SMC_STAT_RMB_TX_SIZE_SMALL on the user length; it does
not bound the copy.

Bound the write space to sndbuf_desc->len at the consumer, treating a
negative (sign-overflowed) value as out of range too, so the copy can
never exceed the ring. This enforces the documented
0 <= sndbuf_space <= sndbuf_desc->len invariant where it is race-free
against the CDC tasklet; conforming peers are unaffected.

Fixes: cc0ab806fc52 ("net/smc: adapt cursor update when sndbuf and peer DMB are merged")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
Reviewed-by: Dust Li <dust.li@linux.alibaba.com>
Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
---
 net/smc/smc_tx.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
index 3144b4b1fe29..5916f02060fb 100644
--- a/net/smc/smc_tx.c
+++ b/net/smc/smc_tx.c
@@ -233,6 +233,19 @@ int smc_tx_sendmsg(struct smc_sock *smc, struct msghdr *msg, size_t len)
 		/* initialize variables for 1st iteration of subsequent loop */
 		/* could be just 1 byte, even after smc_tx_wait above */
 		writespace = atomic_read(&conn->sndbuf_space);
+		/* sndbuf_space is advanced from the peer's wire-controlled
+		 * consumer cursor on the SMC-D DMB-merge path; a forged cursor
+		 * can inflate it past the send buffer, or overflow the signed
+		 * accumulator to a negative value across many CDC messages
+		 * (which a plain "> len" check would miss before the size_t
+		 * cast below turns it huge).  Bound it to the send buffer in
+		 * either case so the wrap-around write cannot run past
+		 * sndbuf_desc->len.  This enforces the documented
+		 * 0 <= sndbuf_space <= sndbuf_desc->len invariant at the
+		 * producer, race-free against the CDC tasklet.
+		 */
+		if (writespace < 0 || writespace > conn->sndbuf_desc->len)
+			writespace = conn->sndbuf_desc->len;
 		/* not more than what user space asked for */
 		copylen = min_t(size_t, send_remaining, writespace);
 		/* determine start of sndbuf */

-- 
2.56.0

Re: [PATCH net v6 0/3] net/smc: bound wire-controlled CDC cursors against the local buffers

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Incomplete fix: the patch caps only the count of the wire-
  controlled producer cursor at rmb_desc->len.

--

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

When CDC messages are converted to host order, this patch clamps the count
of the peer's producer cursor to rmb_desc->len. It does this for both SMC-R
and SMC-D. The consumer cursor is left unbounded because it indexes the
peer's RMB.

> 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;
>  	smc_curs_copy(local, &temp, conn);
>  }

[Severity: High]

Does clamping only the count keep the producer cursor inside the RMB? The
checks above only reject a cursor that moves backwards. A peer can still
move prod.wrap forward by any amount in one message. The only gate before
this is the seqno check in smc_cdc_rx_handler(), and the peer sets seqno.

smcd_cdc_msg_to_host() has the same count-only clamp. It also takes
peer->prod.wrap with no check at all.

The new cursor goes straight into bytes_to_rcv:

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);
		/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */
    ...
}

When the wraps differ, smc_curs_diff() returns
(size - old->count) + new->count. Even with new->count clamped to len, one
message can still add up to 2 * len. For example:

    prod (0, 0) -> (1, len)   adds 2 * len
    then (2, len), (3, len)   each adds another len

Nothing enforces the "guarantee" in that comment.

With only this patch applied, smc_rx_recvmsg() computes copylen as
min(read_remaining, readable), where readable comes from bytes_to_rcv. The
second chunk is then read from offset 0:

	chunk_len = copylen - chunk_len; /* remainder */
	...
	chunk_off = 0; /* modulo offset in recv ring buffer */

Can that remainder be larger than rmb_desc->len? If so, memcpy_to_msg() or
smc_rx_splice() would read past rcvbuf_base + len and copy kernel memory to
userspace.

A later patch in this series, "net/smc: bound the receive length to the
RMB in smc_rx_recvmsg()", caps readable at rmb_desc->len on each pass. That
closes the direct overrun in the second chunk. However, bytes_to_rcv can
still be inflated, and smc_rx_recvmsg() builds its local cursor like this:

	smc_curs_copy(&cons, &conn->local_tx_ctrl.cons, conn);
	if ((flags & MSG_PEEK) && peeked_bytes)
		smc_curs_add(conn->rmb_desc->len, &cons, peeked_bytes);
	/* subsequent splice() calls pick up where previous left */
	if (splbytes)
		smc_curs_add(conn->rmb_desc->len, &cons, splbytes);

smc_curs_add() subtracts size only once. With MSG_PEEK | MSG_WAITALL and a
large read length, peeked_bytes can reach 2 * len after two passes. That
leaves cons.count >= len, and then:

	chunk_len = min_t(size_t, copylen, conn->rmb_desc->len -
			  cons.count);
	chunk_off = cons.count;

The subtraction is done in u32 and wraps, so chunk_len becomes copylen.
memcpy_to_msg() then reads from rcvbuf_base + cons.count, which is
completely past the end of the RMB. Repeated splice() calls that let
splice_pending grow without draining the pipe seem to hit the same path.

Would it work better to reject any producer advance larger than the free
RMB space (for example, bytes_to_rcv + diff_prod > rmb_desc->len) and abort
the connection, instead of clamping the count here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c%40proton.me

Re: [PATCH net v6 2/3] net/smc: bound the receive length to the RMB in smc_rx_recvmsg()

[netdev-bot+sashiko]

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

Critical: 1 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] The fix is incomplete: smc_rx_recvmsg() (net/smc/smc_rx.c)
  can still read past the RMB and copy that kernel memory to userspace.
- [High] smc_rx_recvmsg() (net/smc/smc_rx.c:444-456) can see a negative
  `readable` from a purely local race, with no forged peer.
- [Low] net/smc: invariant violation by subtracting from negative
  bytes_to_rcv

--

net/smc: bound the receive length to the RMB in smc_rx_recvmsg()

This patch limits the readable length in smc_rx_recvmsg() to
rmb_desc->len. It also maps negative values to rmb_desc->len. The goal is
to stop a forged peer producer cursor from pushing the wrap-around copy
past the end of the RMB.

> diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
> index 197fddc6271f0..f781d7229185c 100644
> --- a/net/smc/smc_rx.c
> +++ b/net/smc/smc_rx.c
> @@ -442,6 +442,18 @@ int smc_rx_recvmsg(struct smc_sock *smc, struct msghdr *msg,
>  		/* initialize variables for 1st iteration of subsequent loop */
>  		/* could be just 1 byte, even after waiting on data above */
>  		readable = smc_rx_data_available(conn, peeked_bytes);
> +		/* bytes_to_rcv is accumulated from the peer's wire-controlled
> +		 * producer cursor; a forged cursor can drive it past the RMB,
> +		 * or overflow the signed accumulator to a negative value across
> +		 * many CDC messages (which a plain "> len" check would miss
> +		 * before the size_t cast below turns it huge).  Bound it to the
> +		 * RMB in either case so the wrap-around copy cannot run past
> +		 * rmb_desc->len.  This enforces the documented
> +		 * 0 <= bytes_to_rcv <= rmb_desc->len invariant at the consumer,
> +		 * race-free against the producer update in the receive tasklet.
> +		 */
> +		if (readable < 0 || readable > conn->rmb_desc->len)
> +			readable = conn->rmb_desc->len;

[Severity: Low]
The new comment says this check "enforces the documented
0 <= bytes_to_rcv <= rmb_desc->len invariant at the consumer". Does it?

The clamp only changes the local readable copy. conn->bytes_to_rcv
itself is left as it was. Further down, the non-peek path still does:

	atomic_sub(copylen, &conn->bytes_to_rcv);
	/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */

Suppose a forged producer cursor has already pushed bytes_to_rcv
negative. readable is then mapped to rmb_desc->len, so copylen can be
up to a full ring. That copylen is then subtracted from a counter that
is already below zero. On every pass the counter moves further from
the invariant, not back toward it.

smc_rx_data_available() keeps returning a non-zero negative value.
So every later recvmsg() call goes straight to the copy label and is
clamped to a full ring again. It hands that ring to user space and
advances the consumer cursor through smc_rx_update_consumer().

The connection never recovers from the bad state. The patch just keeps
re-clamping it, and the comment next to the atomic_sub() becomes
false.

Either the commit message and the new comment should stop claiming the
invariant is enforced, or the bad state should be handled. Would it be
better to detect the out-of-range counter and abort the connection, or
mark it out_of_sync, rather than keep reading from it? The detection
could go in smc_cdc_msg_recv_action() when diff_prod is added, or
here.

[Severity: Critical]
The commit message says that with this check "the copy can never exceed
the ring". Is that the case?

The check limits the length copied in this pass. The copy start offset,
though, comes separately from the running peeked_bytes and
splice_pending values:

net/smc/smc_rx.c:smc_rx_recvmsg() {
    ...
	smc_curs_copy(&cons, &conn->local_tx_ctrl.cons, conn);
	if ((flags & MSG_PEEK) && peeked_bytes)
		smc_curs_add(conn->rmb_desc->len, &cons, peeked_bytes);
	/* subsequent splice() calls pick up where previous left */
	if (splbytes)
		smc_curs_add(conn->rmb_desc->len, &cons, splbytes);
    ...
}

smc_curs_add() subtracts size only once:

net/smc/smc_cdc.h:smc_curs_add() {
	curs->count += value;
	if (curs->count >= size) {
		curs->wrap++;
		curs->count -= size;
	}
}

So once peeked_bytes reaches 2 * len - cons.count, cons.count stays at or
above rmb_desc->len.

The chunk calculation then subtracts a u32 from an int:

	chunk_len = min_t(size_t, copylen, conn->rmb_desc->len -
			  cons.count);
	chunk_len_sum = chunk_len;
	chunk_off = cons.count;

The result wraps as unsigned. That gives chunk_len = copylen (up to
rmb_desc->len) and chunk_off >= rmb_desc->len. The call
memcpy_to_msg(msg, rcvbuf_base + chunk_off, chunk_len) would then copy
memory beyond the RMB into the user buffer.

On the splice path, smc_rx_splice() would set partial[0].offset past the
buffer. For is_vm RMBs it would call vmalloc_to_page() on an address
outside the vmalloc area.

For example, take RMB length L, a starting consumer count c with
0 < c < L, bytes_to_rcv >= 3L, and recv(MSG_PEEK | MSG_WAITALL) with a
buffer larger than about 2L:

  pass 1: copies L bytes, peeked_bytes = L
  pass 2: cons = c + L wraps to c, peeked_bytes = 2L
  pass 3: cons = c + 2L, single subtraction leaves c + L >= L,
          chunk_len = min(L, (u32)(L - (c + L))) = L,
          chunk_off = c + L, so [c + L, c + 2L) is read, entirely
          past the RMB

Can a peer push bytes_to_rcv to 3L or more? smc_cdc_msg_recv_action()
still adds diff_prod with no limit:

	atomic_add(diff_prod, &conn->bytes_to_rcv);

The differing-wrap branch of smc_curs_diff() returns
(size - old->count) + new->count. That is close to 2 * len per CDC
message.

The earlier patch in this series, "net/smc: bound the wire-controlled
producer cursor to the RMB", limits count but not wrap. A peer that bumps
wrap on every message can keep growing the counter.

The bad offset also looks reachable without a malicious peer, through the
local MSG_PEEK race described in the next comment.

The later patch in the series, "net/smc: bound the send length to the
send buffer in smc_tx_sendmsg()", only touches net/smc/smc_tx.c, so this
does not seem to be handled anywhere else in the series.

Could bytes_to_rcv be limited at the producer in the tasklet instead?
Another option is to compute the available length as
min(bytes_to_rcv, len) minus peeked_bytes / splbytes, and treat a
negative result as 0.

[Severity: High]
Can readable go negative here from a purely local race, with no forged
peer? smc_rx_data_available() returns:

	return atomic_read(&conn->bytes_to_rcv) - peeked;

Consider two threads on the same AF_SMC socket:

Thread A
recv(MSG_PEEK | MSG_WAITALL, large buffer)
  peeks all N bytes, peeked_bytes = N, read_done < target
  smc_rx_wait()
    sk_wait_event()
      release_sock()

Thread B
recv()
  lock_sock()
  consumes the N bytes
    atomic_sub(copylen, &conn->bytes_to_rcv)  -> 0
    smc_rx_update_consumer()  -> local_tx_ctrl.cons advanced by N

Thread A then wakes on SO_RCVTIMEO, a signal, or new data M < N. The
fcrit() check sees bytes_to_rcv - N < 0, which is non-zero. So
smc_rx_wait() returns and thread A jumps to copy with
readable = M - N < 0.

With this patch, that negative value becomes rmb_desc->len (a full ring)
instead of "no data".

Thread A rebuilds cons from the shared local_tx_ctrl.cons, which thread B
has already advanced, plus the stale peeked_bytes. It then copies a full
ring of bytes that are not valid stream data: either data already
consumed, or space the peer has not written yet. RMB contents are not
cleared when buffers are reused, so these bytes can be left over from
earlier traffic.

Each later pass computes a more negative readable, which is again mapped
to rmb_desc->len, so peeked_bytes grows by a full ring per pass. Within
two or three passes, smc_curs_add() leaves cons.count >= len, and
memcpy_to_msg() reads past the RMB as described in the previous comment.

The new comment says the check is "race-free against the producer update
in the receive tasklet", but this race is between two consumers. TCP
handles the same situation by resetting peek_seq to copied_seq.

Should a negative readable be treated as 0 (no data), or should the peek
state be reset, rather than mapping it to rmb_desc->len?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c%40proton.me

Re: [PATCH net v6 3/3] net/smc: bound the send length to the send buffer in smc_tx_sendmsg()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message's threat model does not match the code it
  changes.
- [Medium] The patch says it 'enforces the documented 0 <= sndbuf_space
  <= sndbuf_desc->len invariant'.

--

net/smc: bound the send length to the send buffer in smc_tx_sendmsg()

This patch clamps the local writespace snapshot in smc_tx_sendmsg() to
sndbuf_desc->len. A negative value is also treated as out of range. The
stated goal is to prevent a heap out-of-bounds write in the wrap-around
copy when the SMC-D DMB-merge path inflates conn->sndbuf_space from a
forged consumer cursor.

> On the SMC-D DMB-merge (nocopy) path, smc_cdc_msg_recv_action()
> advances conn->sndbuf_space from the peer's wire-controlled consumer
> cursor via smc_curs_diff(), which can return more than sndbuf_desc->len;
> a forged cursor drives sndbuf_space past the send buffer, and over many
> CDC messages overflows the signed counter negative. smc_tx_sendmsg()
> reads it as the write space and does a wrap-around copy whose second
> chunk is not re-bounded to sndbuf_desc->len, spilling the local
> sender's outbound data past the send buffer at a peer-controlled
> length: a heap out-of-bounds write.

[Severity: Medium]
Is "the peer's wire-controlled consumer cursor" accurate for this path?

The diff_tx update in smc_cdc_msg_recv_action() only runs behind this
check:

	if (conn->lgr->is_smcd &&
	    smc_ism_support_dmb_nocopy(conn->lgr->smcd)) {

and smc_ism_support_dmb_nocopy() is:

	/* for now only loopback-ism supports
	 * merging sndbuf with peer DMB to avoid
	 * data copies between them.
	 */
	return (smcd->dibs->ops->support_mmapped_rdmb &&
		smcd->dibs->ops->support_mmapped_rdmb(smcd->dibs));

The only in-tree provider of support_mmapped_rdmb is dibs_lo_ops in
drivers/dibs/dibs_loopback.c. dibs_lo_query_rgid() accepts only its own
GID, so the peer on this path is the same kernel's SMC stack.

That kernel builds the consumer cursor from its own connection state in
smcd_cdc_msg_send():

	curs.acurs.counter = atomic64_read(&conn->local_tx_ctrl.cons.acurs);
	cdc.cons.wrap = curs.wrap;
	cdc.cons.count = curs.count;

dibs_lo_move_data() then copies it into the DMB with a bounds check.
Userspace does not write the CDC header area of the DMB.

The buffer lengths also match. smcd_new_buf_create() sets rmb_desc->len
to bufsize - sizeof(struct smcd_cdc_msg). smcd_buf_attach() sets the
attached sndbuf_desc->len to dmb_len - sizeof(struct smcd_cdc_msg). So a
conforming cursor should not produce a diff_tx larger than
sndbuf_desc->len.

Given this, is the patch hardening against a misbehaving in-kernel peer
or a future nocopy-capable device, rather than fixing a heap
out-of-bounds write that a peer can trigger? If so, could the commit
message be reworded? The current wording and the Fixes: and Cc: stable
tags describe a memory corruption that a remote peer controls.

> diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
> index 3144b4b1fe290..5916f02060fb9 100644
> --- a/net/smc/smc_tx.c
> +++ b/net/smc/smc_tx.c
> @@ -233,6 +233,19 @@ int smc_tx_sendmsg(struct smc_sock *smc, struct msghdr *msg, size_t len)
>  		/* initialize variables for 1st iteration of subsequent loop */
>  		/* could be just 1 byte, even after smc_tx_wait above */
>  		writespace = atomic_read(&conn->sndbuf_space);
> +		/* sndbuf_space is advanced from the peer's wire-controlled
> +		 * consumer cursor on the SMC-D DMB-merge path; a forged cursor
> +		 * can inflate it past the send buffer, or overflow the signed
> +		 * accumulator to a negative value across many CDC messages
> +		 * (which a plain "> len" check would miss before the size_t
> +		 * cast below turns it huge).  Bound it to the send buffer in
> +		 * either case so the wrap-around write cannot run past
> +		 * sndbuf_desc->len.  This enforces the documented
> +		 * 0 <= sndbuf_space <= sndbuf_desc->len invariant at the
> +		 * producer, race-free against the CDC tasklet.
> +		 */
> +		if (writespace < 0 || writespace > conn->sndbuf_desc->len)
> +			writespace = conn->sndbuf_desc->len;

[Severity: Medium]
Does this enforce the invariant on conn->sndbuf_space itself? Only the
local writespace snapshot is clamped. The shared atomic is never
corrected.

Later in smc_tx_sendmsg(), the subtraction still runs on the
out-of-range value:

		atomic_sub(copylen, &conn->sndbuf_space);
		/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */

A negative counter goes further negative. A value of
2 * sndbuf_desc->len stays at sndbuf_desc->len after a full-ring copy.

The source of the value is also unchanged.
smc_cdc_msg_recv_action() still adds diff_tx without a bound:

			atomic_add(diff_tx, &conn->sndbuf_space);
			/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */

smcd_cdc_msg_to_host() copies peer->cons without the wrap and count
check that smc_cdc_cursor_to_host() does for SMC-R.

If the premise in the commit message holds, the other readers would
still see the bad value:

  smc_tx_sendmsg():  if (!atomic_read(&conn->sndbuf_space) || ...
  smc_tx_wait(), smc_tx_should_cork(), smc_poll() EPOLLOUT
  smc_ioctl() SIOCOUTQ:
	answ = smc->conn.sndbuf_desc->len -
			atomic_read(&smc->conn.sndbuf_space);

The counter would never reach zero, so sendmsg would not block. It
would keep rewriting the whole ring over data the peer has not consumed
yet. SIOCOUTQ could return a negative value or one larger than the
buffer.

The commit message also says the bound is applied "at the consumer",
but this comment says "at the producer". Which one is meant?

Would it be more accurate to describe this as bounding the copy length?
Alternatively, diff_tx (or the cursor) could also be bounded where it is
received, in smc_cdc_msg_recv_action().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c%40proton.me