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

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

[Bryam Vargas via B4 Relay]

A peer's CDC cursors come off the wire and are used, with no bound
against the buffers they address, as a raw index into the RMB on the
urgent path (1/6), as the rx length (2/6), as the tx write space (3/6),
as the length and the destination of an RDMA write (5/6), and as the
cursor advance at SMC-R link failover (6/6). Any peer that completes the
SMC handshake controls them. With a forged cursor these run past the
buffer: an OOB read of kernel memory returned to the reader, send() data
written past the send buffer, an RDMA write that ends past the peer's
RMBE (its HCA refuses it and the link goes down) or whose source runs
past the send buffer. 4/6 fixes a bug in the existing code, how
smcr_tx_rdma_writes() sizes the source of a write's second chunk; it
sits before 5/6 because 5/6 turns writes that were one chunk into two.

Four cases need no forged cursor: a MSG_PEEK|MSG_WAITALL reader racing
another recv() (2/6), late completions on the old link after a
failover (3/6), a peer that reads across the end of its RMBE while
unconfirmed data waits for the resend after a failover (5/6), and a
completion handled while the failover switches the connection, after
which the next RDMA write can send the peer the memory that follows the
send buffer (4/6; it does on rxe). With a second link in the group the
peer can force the failover: a DELETE LINK request, or failing one of
our RDMA writes.

Bounding each cursor on arrival is not enough. A peer that only bumps
prod.wrap with count 0 makes smc_curs_diff() return the full buffer
length on every CDC, so bytes_to_rcv grows past the RMB while every
cursor looks in range. IOW the overflow is in the accumulator, not the
cursor. 1/6 can clamp on arrival because the urgent path indexes with
the count itself; 2/6, 3/6, 5/6 and 6/6 bound the value where it is
used. The series does not depend on Hidayath Khan's "net/smc: abort the
connection when the peer overruns the RMB" [1] (net-next).

Not fixed here: the failover switch is not serialized against
smc_cdc_tx_handler() (send_lock on one side, the socket spinlock on the
other), so a completion can be handled inside smc_switch_cursor(). It is
then credited to sndbuf_space more than once: 3/6 keeps send() inside
the buffer, but send() can then overwrite data the peer has not
received (by inspection). One handled between the switch's first two
cursor copies makes the next write hand the peer bytes it has already
read. Unpatched does both. v9 ends worse than unpatched in two cases,
both with a completion handled inside the switch (figures from the runs
in the notes under 5/6 and 6/6):
  - one between the first two copies, and the peer has read across the
    end of its RMBE before the resend: unpatched has the write refused
    and loses the connection; with 5/6 the write goes out and the
    connection lives on, the peer gets 600 bytes twice and sndbuf_space
    ends at 3x the buffer (fourth case under 5/6).
  - one that has moved tx_curs_fin ahead: 6/6 sets it back to the
    advanced tx_curs_sent, so an old-link completion handled before the
    validation message's is credited once more, and the first case under
    4/6 ends 1200 bytes over the buffer where unpatched and v8 end 600
    over.

Serializing the switch means taking the socket spinlock around
send_lock in smc_switch_conns(); this series does not change the
locking. smc_curs_comp() is not changed either: it misorders two cursors
across the u16 wrap rollover [2], and 6/6 only caps what the advance
then adds (by inspection).

For stable: 4/6, 5/6 and 6/6 go together. With 6/6 and not 5/6, after
a failover with a consumer count past the peer's RMBE the next write
can end past that RMBE, where unpatched stalls; with 5/6 and not 4/6
the peer can be sent the memory that follows the send buffer, where
unpatched has the write refused. 1/6 and 2/6 are both needed: 1/6
alone bounds the count, not what wrap bumps add to bytes_to_rcv.

Found by inspection of the cursor handling in net/smc; the cases below
were then confirmed on a KASAN rig, except where marked by inspection.

Testing: KASAN, v7.4-rc2 plus d7900fa51642 ("net/smc: fix lgr/lnk
lifetime vs diag reader race"), where net/smc is identical to the base
commit's; unpatched, v8 and v9. The test lists of v7 and v8 are in the
v8 cover (link below): unpatched reads and writes past the buffers, the
series stays inside, conforming traffic is unchanged. Their SMC-D
cases, rerun on loopback, give the same results, v9 as v8 in every one.
Over SMC-R on rxe with two links, v7's two failover cases stay inside
their buffers on v9, no KASAN splat. The SMC-R cases run for v9, with
test hooks making the timing deterministic (inputs and figures in the
notes under 4/6, 5/6 and 6/6):
  - a completion handled inside smc_switch_cursor(): unpatched and v8
    leave local_tx_ctrl_fin at count 16713 for a 16384-byte RMBE and
    tx_curs_fin past tx_curs_prep; v9 leaves tx_curs_fin on tx_curs_prep
    and local_tx_ctrl_fin on the producer cursor (6/6).
  - consumer cursor forged one wrap ahead at count 3 * RMBE, which adds
    four RMBEs to the window: unpatched and v8 post 40000 bytes in one
    chunk for a 16384-byte RMBE, rxe refuses the write and the link goes
    down; v9 posts one RMBE in two chunks and both links stay up (5/6).
  - consumer count forged a million bytes ahead with the wrap unchanged,
    send buffer larger than the RMBE: unpatched and v8 post a second
    chunk longer than the RMBE, with a source SGE that ends 4464 bytes
    past the send buffer; v9 posts one RMBE with every SGE inside (5/6;
    either of 4/6 and 5/6 keeps this source inside).
  - conforming peer, a completion handled between the switch's first two
    cursor copies, which leaves tx_curs_sent 600 bytes behind the
    producer cursor: unpatched and v8 build a source SGE that ends 4536
    bytes past the send buffer and the peer reads bytes from outside it
    (KASAN splat on the unpatched run); v9 builds none (4/6). The peer
    reads 600 bytes twice on all three.
  - same race, the peer reading across the end of its RMBE before the
    resend: unpatched and v8 have the write refused and lose the
    connection. With 5/6 and not 4/6 the source of the second chunk ends
    1464 bytes past the send buffer and the peer reads bytes from
    outside it. v9 keeps source and destination inside and the link up,
    and the peer gets 600 bytes twice.
  - conforming peer, completions lost with the failing link, the resend
    held until the peer has read the data: unpatched and v8 post 11000
    bytes in one chunk that ends past the 16384-byte RMBE, the surviving
    link goes down and the peer's read fails; v9 posts two chunks and
    the peer reads the stream intact (5/6).
  - the two forged-cursor cases of the note under v8's 4/4. Cursor one
    wrap ahead at count 64: v9 as that note reports for v8, nothing is
    written after the switch (v8 and unpatched not rerun). Count at RMBE
    + 4096: unpatched stalls with 3137 bytes still queued, v8 posts
    16384 bytes from offset 1 and the link goes down, v9 posts 16383 + 1
    and the link stays up.
  - conforming traffic on v9: 600001 bytes across a failover in 39
    writes, 18 of them in two chunks, each chunked as the code that 5/6
    replaces would chunk it (compared in the kernel, write by write).

Reverting any one code change of 4/6 or 5/6, or v9's change to 6/6,
makes its case fail. smc_cdc.o built from v9 is byte-identical to v8's
on the same tree: v9 changes only a comment in smc_cdc.c. Not covered:
SMC-R ran on rxe only, with physically contiguous buffers (no real HCA,
no vmalloc'ed send buffer); SMC-D ran on loopback only, where
smcd_tx_rdma_writes() returns before it writes, so the ISM write that
5/6 sizes has not been run. Logs on request.

Changes in v9:
  - 4/6 is new, for the bug in the existing code that the review of
    v7's 4/4 [3] pointed out: the source of the second chunk is sized
    from the send cursor as it was before the first. The review, and my
    reply to it, took it for unreachable by a conforming peer; the race
    above reaches it.
  - 5/6 is new, for the first point of Sashiko's review of v8's 4/4
    [2]: with a consumer count past the peer's RMBE, v8 skipped the
    advance but peer_rmbe_space was still sized from that cursor, and
    the next write ended past the peer's RMBE where unpatched only
    stalls. 5/6 bounds the write where its length and destination are
    computed, which also covers a conforming peer after a failover and
    a forged window with no failover. [3] had already pointed out the
    one-chunk write and the window larger than the RMBE, both in the
    unpatched code, hence the Reported-by and Closes tags on 5/6.
  - 6/6 (was v8 4/4): tx_curs_fin and local_tx_ctrl_fin are set from
    the advanced cursors instead of being added to, for the second
    point of [2]. Without a completion inside the switch the result is
    the same as v8's. The switch is still not serialized (see above).
  - 5/6 and 6/6: the Cc: stable lines point at the descriptions, which
    name the patches each one needs.
  - 2/6: conn->rmb_len is accessed with READ_ONCE()/WRITE_ONCE(); the
    wait predicates read it without the socket lock [4]. The message
    and the comment in smc_rx_pipe_buf_release() now claim the
    splice_pending change only for a reader that ran between the cursor
    update and the drop; the double count that [4] describes, a splice
    whose second chunk fails, is a different bug and is not fixed here.
  - 3/6: code unchanged. Of the three comments in smc_cdc.c that claim
    0 <= sndbuf_space <= len [5], the one in smc_cdc_tx_handler()
    points at the clamp now. The other two stay: the claim holds in
    smcd_cdc_msg_send(), and the DMB-merge block only exists since
    v6.10, so a hunk there would not apply to 6.6.y and older. The
    message now says that the over-credit itself stays. R-b kept.
  - 1/6: unchanged.
  - Rebased on net (8d5ce4751fad).

v8, with the earlier changelogs and test lists:
  https://lore.kernel.org/all/20261029-b4-disp-33001ace-v8-0-f9bb96e17827@proton.me/

[1] https://lore.kernel.org/all/20260804141109.542202-1-hidayath@linux.ibm.com/
[2] https://lore.kernel.org/all/179364387382.3131.13471253186660322977@kernel.org/
[3] https://lore.kernel.org/all/179209532452.434549.6588379141939967966@kernel.org/
[4] https://lore.kernel.org/all/179364387064.3131.5492897013975868107@kernel.org/
[5] https://lore.kernel.org/all/179364387223.3131.12837391370219531719@kernel.org/

---
Bryam Vargas (6):
      net/smc: bound the wire-controlled producer cursor to the RMB
      net/smc: bound the receive length to the RMB
      net/smc: bound the send length to the send buffer in smc_tx_sendmsg()
      net/smc: keep the source of an RDMA write inside the send buffer
      net/smc: keep an RDMA write inside the peer's RMBE
      net/smc: bound the link failover cursor advance to the send buffer

 net/smc/af_smc.c   |  6 +++---
 net/smc/smc.h      |  3 +++
 net/smc/smc_cdc.c  |  4 ++--
 net/smc/smc_cdc.h  | 17 ++++++++++++++---
 net/smc/smc_core.c | 27 +++++++++++++++++----------
 net/smc/smc_rx.c   |  9 +++++++--
 net/smc/smc_rx.h   | 12 +++++++++++-
 net/smc/smc_tx.c   | 46 ++++++++++++++++++++++------------------------
 8 files changed, 79 insertions(+), 45 deletions(-)
---
base-commit: 8d5ce4751fad9499ea408163af7f410f18a85782
change-id: 20261112-b4-disp-0287c16e-bc0127d9de9e

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

[PATCH net v9 1/6] 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 the peer's
producer cursor from the wire into conn->local_rx_ctrl.prod without
bounding it to the RMB. smc_cdc_handle_urg_data_arrival() then uses its
count as a raw index into the RMB, so a producer cursor past
rmb_desc->len makes the receive tasklet read past the RMB, and
recv(MSG_OOB) hands that byte to the application.

Bound the producer cursor count to rmb_desc->len when it comes off the
wire, for SMC-R and SMC-D. Leave the consumer cursor alone: it indexes
the peer's RMB, which can be larger than ours, so clamping it to our
RMB would under-credit peer_rmbe_space and could stall sending to that
peer. 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 | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/net/smc/smc_cdc.h b/net/smc/smc_cdc.h
index 696cc11f2303..4d630a305118 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,12 @@ static inline void smc_cdc_cursor_to_host(union smc_host_cursor *local,
 	if ((old.wrap == temp.wrap) &&
 	    (old.count > temp.count))
 		return;
+	/* The urgent path uses the producer count as a raw index into our
+	 * RMB, so bound it. max_count == 0 leaves the consumer cursor alone:
+	 * it indexes the peer's RMB, which can be larger than ours.
+	 */
+	if (max_count && temp.count > max_count)
+		temp.count = max_count;
 	smc_curs_copy(local, &temp, conn);
 }
 
@@ -246,8 +253,9 @@ 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, 0);
 	local->prod_flags = peer->prod_flags;
 	local->conn_state_flags = peer->conn_state_flags;
 }
@@ -260,6 +268,9 @@ static inline void smcd_cdc_msg_to_host(struct smc_host_cdc_msg *local,
 
 	temp.wrap = peer->prod.wrap;
 	temp.count = peer->prod.count;
+	/* as in smc_cdc_cursor_to_host(): bound prod, not cons */
+	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 v9 2/6] net/smc: bound the receive length to the RMB

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

smc_rx_recvmsg() sizes the copy from bytes_to_rcv and starts it past the
bytes MSG_PEEK or splice() already took. A forged producer cursor drives
bytes_to_rcv past the RMB or negative, and with a conforming peer it
drops below the peeked count when a MSG_PEEK|MSG_WAITALL reader sleeps
while another recv() consumes what it peeked. Either way the copy reads
past the RMB into the reader's buffer. poll(), SIOCINQ and the deferred
accept read the same raw count.

Make smc_rx_data_available() report at most what fits in the RMB behind
the peeked and spliced bytes, never a negative count, and use it in all
of these. Cache the RMB length for the unlocked wait, where rmb_desc may
already be freed, and drop released bytes from splice_pending under the
socket lock, with the consumer cursor update, so a reader cannot run
between the two and count them twice.

Fixes: 952310ccf2d8 ("smc: receive data from RMBE")
Fixes: 9014db202cb7 ("smc: add support for splice()")
Fixes: a4b6539038c1 ("net/smc: fix data error when recvmsg with MSG_PEEK flag")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c@proton.me?part=2
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c%40proton.me?part=2
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c%40proton.me?part=1
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
The wait predicates run unlocked, where a link group termination may
already have freed rmb_desc, hence the cached length; v9 reads and
writes it with READ_ONCE()/WRITE_ONCE(). With the old splice_pending
order, a reader that took the lock between the cursor update and the
drop counted the bytes twice, and the next splice() returned stale bytes
in place of unread data. 1/6 bounds the producer count, but wrap bumps
still grow bytes_to_rcv past the RMB (see the cover).
---
 net/smc/af_smc.c   |  6 +++---
 net/smc/smc.h      |  3 +++
 net/smc/smc_cdc.c  |  2 +-
 net/smc/smc_core.c |  1 +
 net/smc/smc_rx.c   |  9 +++++++--
 net/smc/smc_rx.h   | 12 +++++++++++-
 6 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index ec5980add7a9..90e7d0d2e398 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -2774,7 +2774,7 @@ int smc_accept(struct socket *sock, struct socket *new_sock,
 			if (skb_queue_empty(&clcsk->sk_receive_queue))
 				sk_wait_data(clcsk, &timeo, NULL);
 			release_sock(clcsk);
-		} else if (!atomic_read(&smc_sk(nsk)->conn.bytes_to_rcv)) {
+		} else if (!smc_rx_data_available(&smc_sk(nsk)->conn, 0)) {
 			lock_sock(nsk);
 			smc_rx_wait(smc_sk(nsk), &timeo, 0, smc_rx_data_available);
 			release_sock(nsk);
@@ -2939,7 +2939,7 @@ __poll_t smc_poll(struct file *file, struct socket *sock,
 						mask |= EPOLLOUT | EPOLLWRNORM;
 				}
 			}
-			if (atomic_read(&smc->conn.bytes_to_rcv))
+			if (smc_rx_data_available(&smc->conn, 0))
 				mask |= EPOLLIN | EPOLLRDNORM;
 			if (sk->sk_shutdown & RCV_SHUTDOWN)
 				mask |= EPOLLIN | EPOLLRDNORM | EPOLLRDHUP;
@@ -3237,7 +3237,7 @@ int smc_ioctl(struct socket *sock, unsigned int cmd,
 		    smc->sk.sk_state == SMC_CLOSED)
 			answ = 0;
 		else
-			answ = atomic_read(&smc->conn.bytes_to_rcv);
+			answ = smc_rx_data_available(&smc->conn, 0);
 		break;
 	case SIOCOUTQ:
 		/* output queue size (not send + not acked) */
diff --git a/net/smc/smc.h b/net/smc/smc.h
index 36b243b34a9a..8601304b3c06 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -204,6 +204,9 @@ struct smc_connection {
 						 * rmbe
 						 */
 	int			rtoken_idx;	/* idx to peer RMB rkey/addr */
+	int			rmb_len;	/* rmb_desc->len, still valid
+						 * after rmb_desc is freed
+						 */
 
 	struct smc_buf_desc	*sndbuf_desc;	/* send buffer descriptor */
 	struct smc_buf_desc	*rmb_desc;	/* RMBE descriptor */
diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index 32d6d03df321..e2c765ff2ba3 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -381,7 +381,7 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
 		/* bytes_to_rcv is decreased in smc_recvmsg */
 		smp_mb__before_atomic();
 		atomic_add(diff_prod, &conn->bytes_to_rcv);
-		/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */
+		/* can be out of range, see smc_rx_data_available() */
 		smp_mb__after_atomic();
 		smc->sk.sk_data_ready(&smc->sk);
 	} else {
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 0a9f5418329a..0edbe6106c1a 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -2511,6 +2511,7 @@ static int __smc_buf_create(struct smc_sock *smc, bool is_smcd, bool is_rmb)
 
 	if (is_rmb) {
 		conn->rmb_desc = buf_desc;
+		WRITE_ONCE(conn->rmb_len, buf_desc->len);
 		conn->rmbe_size_comp = bufsize_comp;
 		smc->sk.sk_rcvbuf = bufsize * 2;
 		atomic_set(&conn->bytes_to_rcv, 0);
diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
index 63c178c10bf1..78ab3df1689c 100644
--- a/net/smc/smc_rx.c
+++ b/net/smc/smc_rx.c
@@ -118,15 +118,20 @@ static void smc_rx_pipe_buf_release(struct pipe_inode_info *pipe,
 	struct smc_connection *conn = &priv->smc->conn;
 	struct smc_sock *smc = priv->smc;
 	struct sock *sk = &smc->sk;
+	bool drained;
 
 	lock_sock(sk);
 	if (conn->freed) {
 		release_sock(sk);
 		goto out;
 	}
+	/* lower splice_pending under the socket lock, with the cons update, or
+	 * a reader running between the two counts the released bytes twice
+	 */
+	drained = atomic_sub_and_test(priv->len, &conn->splice_pending);
 	smc_rx_update_cons(smc, priv->len);
 	release_sock(sk);
-	if (atomic_sub_and_test(priv->len, &conn->splice_pending))
+	if (drained)
 		smc_rx_wake_up(sk);
 out:
 	kfree(priv);
@@ -543,7 +548,7 @@ int smc_rx_recvmsg(struct smc_sock *smc, struct msghdr *msg,
 			/* increased in recv tasklet smc_cdc_msg_rcv() */
 			smp_mb__before_atomic();
 			atomic_sub(copylen, &conn->bytes_to_rcv);
-			/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */
+			/* can be out of range, see smc_rx_data_available() */
 			smp_mb__after_atomic();
 			if (msg && smc_rx_update_consumer(smc, cons, copylen))
 				goto out;
diff --git a/net/smc/smc_rx.h b/net/smc/smc_rx.h
index 994f5e42d1ba..1833eca15bfb 100644
--- a/net/smc/smc_rx.h
+++ b/net/smc/smc_rx.h
@@ -25,7 +25,17 @@ int smc_rx_wait(struct smc_sock *smc, long *timeo, size_t peeked,
 		int (*fcrit)(struct smc_connection *conn, size_t baseline));
 static inline int smc_rx_data_available(struct smc_connection *conn, size_t peeked)
 {
-	return atomic_read(&conn->bytes_to_rcv) - peeked;
+	int avail = atomic_read(&conn->bytes_to_rcv);
+
+	/* bytes_to_rcv follows the peer's producer cursor and can be past
+	 * the RMB or negative. Report no more than fits in the RMB behind the
+	 * bytes already spliced or peeked. Compare before subtracting: near
+	 * INT_MIN, avail - peeked wraps. This also runs unlocked, possibly
+	 * after rmb_desc was freed, hence rmb_len.
+	 */
+	avail = min(avail, READ_ONCE(conn->rmb_len) -
+			   atomic_read(&conn->splice_pending));
+	return avail > (int)peeked ? avail - (int)peeked : 0;
 }
 
 #endif /* SMC_RX_H */

-- 
2.56.0

[PATCH net v9 3/6] 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>

conn->sndbuf_space can run past the send buffer. After an SMC-R link
failover, a late completion from the old link can move tx_curs_fin back,
and the next completion credits the same bytes again; across the ring
end, smc_curs_diff() credits the step back itself as more than a whole
ring, so even with a conforming peer sndbuf_space can reach three times
the buffer. The failover catch-up and SMC-D DMB merge (only the loopback
device today) also credit it from the peer's consumer cursor without a
bound. smc_tx_sendmsg() takes it as the write space, and the second
chunk of its copy writes past the send buffer.

Clamp writespace, which sizes the copy, to sndbuf_desc->len, negative
values included, so the copy stays in the send buffer. The over-credit
itself is not undone: with sndbuf_space too large, send() can still
overwrite data in the send buffer that the peer has not received.

Fixes: c6f02ebeea3a ("net/smc: switch connections to alternate link")
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>
---
The overwrite after an over-credit is by inspection; not tested. Fixing
the credit in smc_cdc_tx_handler() needs a cursor compare that survives
the u16 wrap rollover, which smc_curs_comp() does not do, so it is left
out here. A negative value maps to len, not 0, since 0 would make the
send loop spin.
---
 net/smc/smc_cdc.c |  2 +-
 net/smc/smc_tx.c  | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index e2c765ff2ba3..90a79a983b48 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -43,7 +43,7 @@ static void smc_cdc_tx_handler(struct smc_wr_tx_pend_priv *pnd_snd,
 		/* sndbuf_space is decreased in smc_sendmsg */
 		smp_mb__before_atomic();
 		atomic_add(diff, &cdcpend->conn->sndbuf_space);
-		/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */
+		/* can be out of range, see smc_tx_sendmsg() */
 		smp_mb__after_atomic();
 		smc_curs_copy(&conn->tx_curs_fin, &cdcpend->cursor, conn);
 		smc_curs_copy(&conn->local_tx_ctrl_fin, &cdcpend->p_cursor,
diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
index 0f0c6df3bf2c..3913ef103b35 100644
--- a/net/smc/smc_tx.c
+++ b/net/smc/smc_tx.c
@@ -233,6 +233,14 @@ 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 can be out of range: late completions of an
+		 * old SMC-R link credit it again after a failover, and with
+		 * SMC-D DMB merge it follows the peer's consumer cursor.
+		 * Bound writespace, which sizes the copy; a negative value
+		 * would become huge in the size_t min_t() below.
+		 */
+		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 */
@@ -278,7 +286,7 @@ int smc_tx_sendmsg(struct smc_sock *smc, struct msghdr *msg, size_t len)
 		/* increased in send tasklet smc_cdc_tx_handler() */
 		smp_mb__before_atomic();
 		atomic_sub(copylen, &conn->sndbuf_space);
-		/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */
+		/* can be out of range, see writespace above */
 		smp_mb__after_atomic();
 		/* since we just produced more new data into sndbuf,
 		 * trigger sndbuf consumer: RDMA write into peer RMBE and CDC

-- 
2.56.0

[PATCH net v9 4/6] net/smc: keep the source of an RDMA write inside the send buffer

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

smcr_tx_rdma_writes() sizes the first source SGE of the second
destination chunk from the send cursor as it was before the first
chunk, so that SGE runs past the send buffer when the buffer ends
inside the second chunk. A physically contiguous send buffer goes out
under the local DMA lkey, which does not bound the SGE; on rxe the peer
is sent the memory that follows the buffer. A conforming peer gets there
when a completion handled inside smc_switch_cursor() leaves tx_curs_sent
behind local_tx_ctrl.prod. Otherwise it takes a peer_rmbe_space forged
larger than the RMBE and a send buffer larger than the RMBE.

Size the SGE from the running offset, as smcd_tx_rdma_writes() does. A
conforming write sends the same bytes.

Fixes: e6727f39004b ("smc: send data (through RDMA)")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013-b4-disp-f345196c-v7-0-ef3dddfbfa68%40proton.me?part=4
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
New in v9. Sashiko's review of v7's 4/4 [1] pointed this bug out
("could this send adjacent kernel memory to the peer?"); the review,
and my reply to it, took it for unreachable by a conforming peer. The
first case below reaches it.

rxe, two links (v7.4-rc2 plus d7900fa51642, KASAN), send buffer and
peer RMBE 64K, physically contiguous, conforming peer. Every connection
carries one byte first; the counts include it. "With the patch" is the
whole series.
  - 60000 bytes sent and read, then 600 and 400 more, read too;
    failover, with the completion of the 600 handed to
    smc_cdc_tx_handler() between the first two cursor copies of
    smc_switch_cursor() (test hook) and the resend held; then
    send(9400). The switch leaves tx_curs_sent at 60400 and prod at
    61000, so the next write is the 600 bytes between them again plus
    the 9400, 4536 + 5464. Unpatched and v8 size the second chunk's
    first source SGE as 5136 bytes from offset 64936, 4536 past the send
    buffer. The peer reads the first 9400 bytes of that write; from byte
    5136 on (4536 + 600) they come from outside the send buffer. KASAN
    splat in rxe_mr_copy() on the unpatched run, where a freed slab
    object follows the buffer; none on v8's. With the patch no SGE ends
    past the buffer and the peer reads what is in it. It gets 600 bytes
    twice either way (see 6/6).
  - no failover, 60000 bytes sent and read, one send(20000): the write
    is 5536 + 14464 bytes and the send buffer ends with the first chunk.
    Unpatched builds 3 source SGEs, the patch 2; the peer reads the same
    20000 bytes.

With 5/6 and 6/6 applied and this change reverted, the first case and
the fourth case under 5/6 fail. Nothing else does: with the two cursors
in step, one RMBE per call already keeps the source inside (the second
case under 5/6).

A model of smcr_tx_rdma_writes() with the old and the new sizing, over
send buffers and RMBEs of 2 to 16 bytes: the new sizing gives 1344 of
12928 conforming writes a different SGE list, never a longer one, and
every write the same bytes.

Not tested on a real HCA, nor with a vmalloc'ed send buffer, where the
SGE goes out under the buffer's own MR.

[1] https://lore.kernel.org/all/179209532452.434549.6588379141939967966@kernel.org/
---
 net/smc/smc_tx.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
index 3913ef103b35..d37c74754461 100644
--- a/net/smc/smc_tx.c
+++ b/net/smc/smc_tx.c
@@ -379,7 +379,6 @@ static int smcr_tx_rdma_writes(struct smc_connection *conn, size_t len,
 		sg_dma_address(conn->sndbuf_desc->sgt[link->link_idx].sgl);
 	u64 virt_addr = (uintptr_t)conn->sndbuf_desc->cpu_addr;
 	int src_len_sum = src_len, dst_len_sum = dst_len;
-	int sent_count = src_off;
 	int srcchunk, dstchunk;
 	int num_sges;
 	int rc;
@@ -425,8 +424,7 @@ static int smcr_tx_rdma_writes(struct smc_connection *conn, size_t len,
 		dst_off = 0; /* modulo offset in RMBE ring buffer */
 		dst_len = len - dst_len; /* remainder */
 		dst_len_sum += dst_len;
-		src_len = min_t(int, dst_len, conn->sndbuf_desc->len -
-				sent_count);
+		src_len = min_t(int, dst_len, conn->sndbuf_desc->len - src_off);
 		src_len_sum = src_len;
 	}
 	return 0;

-- 
2.56.0

[PATCH net v9 5/6] net/smc: keep an RDMA write inside the peer's RMBE

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

smc_tx_rdma_writes() takes the length of a write from peer_rmbe_space
and decides from the cursor wraps whether the destination is one chunk
or two. Both follow the peer's consumer cursor, which nothing bounds: a
forged one takes a one-chunk write past the peer's RMBE, and so does a
conforming peer that keeps reading across the end of its RMBE before
the resend after an SMC-R link failover. With one RMBE per RMB, as
Linux allocates them, the peer's HCA refuses the write and the link
goes down.

Write at most one RMBE per call and end the first chunk no later than
the RMBE, whatever the wraps say. A conforming peer outside failover
gets the chunks it got before. A write that was one chunk can now be
two, which needs "net/smc: keep the source of an RDMA write inside the
send buffer"; the first chunk needs prod.count inside the RMBE, which
"net/smc: bound the link failover cursor advance to the send buffer"
keeps at failover.

Fixes: e6727f39004b ("smc: send data (through RDMA)")
Fixes: c6f02ebeea3a ("net/smc: switch connections to alternate link")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013-b4-disp-f345196c-v7-0-ef3dddfbfa68%40proton.me?part=4
Cc: stable@vger.kernel.org # see patch description, needs two other patches
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
New in v9, for the first point of Sashiko's review of v8's 4/4 [1]: v8
skipped the failover advance for a consumer count past the peer's RMBE,
but the window for this write was still sized from the same cursor. A
check in smc_write_space() is not enough: smc_cdc_msg_recv_action()
credits peer_rmbe_space from the next cursor, and the differing-wrap
branch here then writes the whole length from prod.count as before. The
review of v7's 4/4 [2] had already pointed out the one-chunk write and
the window larger than the RMBE, both in the unpatched code, hence the
Reported-by and Closes tags.

rxe, two links (v7.4-rc2 plus d7900fa51642, KASAN), send buffer 64K,
peer RMBE 16K, test hooks forging the peer's cursor and making the
timing deterministic. Every connection carries one byte first; the
counts include it. "With the patch" is the whole series.
  - consumer cursor forged one wrap ahead at 3 * RMBE, then send(40000):
    unpatched and v8 post 40000 bytes in one chunk from offset 1, rxe
    refuses the write and the link goes down. With the patch the first
    write is 16383 + 1 bytes, neither chunk ends past the RMBE and both
    links stay up.
  - 40000 bytes sent and read, the consumer count then forged a million
    bytes ahead with the wrap unchanged, then send(30000): unpatched and
    v8 post 9152 + 20848 bytes. The second chunk is longer than the RMBE
    and its source SGE is 20848 bytes from offset 49152 of the
    65536-byte send buffer, 4464 past its end; rxe refuses the write.
    With the patch: 9152 + 7232, every source SGE inside. Same result
    with 4/6 reverted; with this patch's length bound reverted the
    second chunk is still longer than the RMBE and its source stays
    inside (4/6).
  - conforming peer: 12001 bytes sent and confirmed, then 9000 whose
    completions are lost with the failing link, the failover, the resend
    held until the peer has read them, then 2000 more. Unpatched and v8
    post 11000 bytes in one chunk from offset 12001 of the 16384-byte
    RMBE; the surviving link goes down and the peer's read fails. With
    the patch the write is 4383 + 6617 bytes and the peer reads the
    stream intact.
  - conforming peer, send buffer and RMBE 64K: 60000 bytes sent and
    read, then 600 and 6000 that the peer reads only after the switch;
    the completion of the 600 handled between the first two cursor
    copies of the switch, as in the first case under 4/6, which leaves
    tx_curs_sent 600 bytes behind prod; the resend held until the peer
    has read; then send(400). Unpatched and v8 post 7000 bytes in one
    chunk that ends 2064 past the RMBE, rxe refuses the write and the
    connection is lost. With this patch and not 4/6 the write is
    4936 + 2064, inside the RMBE, and the second chunk's source ends
    1464 bytes past the send buffer: the peer reads bytes from outside
    it. With 4/6 the source is inside too, the link stays up and the
    peer gets 600 bytes twice (see 6/6; the run reads 400 of them).
  - the second case of the note under v8's 4/4 (consumer cursor forged
    two wraps ahead at count RMBE + 4096; 40001 bytes sent and read, the
    completions of all but the first held): unpatched moves prod onto
    the forged count and writes nothing more, 3137 bytes stay queued
    (SIOCOUTQ); v8 posts 16384 bytes from offset 1, rxe refuses the
    write and the link goes down; with the patch the write is 16383 + 1
    and the link stays up.
  - 600001 bytes of a conforming stream across a failover, with the
    patch: 39 writes, 18 in two chunks, each chunked as the two removed
    branches would chunk it (compared in the kernel, write by write).

Without the length bound the first two cases fail; with the chunk
decided from the wraps again, the first, third, fourth and fifth do.

The comment on the credit in smc_cdc_msg_recv_action() still claims
0 <= peer_rmbe_space <= peer_rmbe_size: the lines after it exist only
since v6.10, and a hunk there would not apply to 6.6.y and older.

The first chunk needs prod.count inside the RMBE.
smc_tx_advance_cursors() keeps it there and, with 6/6, so does
smc_switch_cursor(); without 6/6 a forged cursor can still leave it
outside at failover, and the write then starts where it started before
this patch (by inspection).

On SMC-D the same lines size the ISM write. On loopback
smcd_tx_rdma_writes() returns before it writes; not tested on ISM
hardware.

[1] https://lore.kernel.org/all/179364387382.3131.13471253186660322977@kernel.org/
[2] https://lore.kernel.org/all/179209532452.434549.6588379141939967966@kernel.org/
---
 net/smc/smc_tx.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
index d37c74754461..c90bdc77f82d 100644
--- a/net/smc/smc_tx.c
+++ b/net/smc/smc_tx.c
@@ -362,7 +362,7 @@ static inline void smc_tx_advance_cursors(struct smc_connection *conn,
 	smp_mb__before_atomic();
 	/* data in flight reduces usable snd_wnd */
 	atomic_sub(len, &conn->peer_rmbe_space);
-	/* guarantee 0 <= peer_rmbe_space <= peer_rmbe_size */
+	/* can be out of range, see smc_tx_rdma_writes() */
 	smp_mb__after_atomic();
 	smc_curs_add(conn->sndbuf_desc->len, sent, len);
 }
@@ -480,7 +480,7 @@ static int smc_tx_rdma_writes(struct smc_connection *conn,
 			      struct smc_rdma_wr *wr_rdma_buf)
 {
 	size_t len, src_len, dst_off, dst_len; /* current chunk values */
-	union smc_host_cursor sent, prep, prod, cons;
+	union smc_host_cursor sent, prep, prod;
 	struct smc_cdc_producer_flags *pflags;
 	int to_send, rmbespace;
 	int rc;
@@ -503,31 +503,23 @@ static int smc_tx_rdma_writes(struct smc_connection *conn,
 		return 0;
 	}
 	smc_curs_copy(&prod, &conn->local_tx_ctrl.prod, conn);
-	smc_curs_copy(&cons, &conn->local_rx_ctrl.cons, conn);
 
 	/* if usable snd_wnd closes ask peer to advertise once it opens again */
 	pflags = &conn->local_tx_ctrl.prod_flags;
 	pflags->write_blocked = (to_send >= rmbespace);
-	/* cf. usable snd_wnd */
-	len = min(to_send, rmbespace);
+	/* cf. usable snd_wnd. The window follows the peer's consumer cursor,
+	 * which comes off the wire: write at most one RMBE per call.
+	 */
+	len = min3(to_send, rmbespace, conn->peer_rmbe_size);
 
 	/* initialize variables for first iteration of subsequent nested loop */
 	dst_off = prod.count;
-	if (prod.wrap == cons.wrap) {
-		/* the filled destination area is unwrapped,
-		 * hence the available free destination space is wrapped
-		 * and we need 2 destination chunks of sum len; start with 1st
-		 * which is limited by what's available in sndbuf
-		 */
-		dst_len = min_t(size_t,
-				conn->peer_rmbe_size - prod.count, len);
-	} else {
-		/* the filled destination area is wrapped,
-		 * hence the available free destination space is unwrapped
-		 * and we need a single destination chunk of entire len
-		 */
-		dst_len = len;
-	}
+	/* at most 2 destination chunks of sum len; the 1st ends no later than
+	 * the peer's RMBE. The cursor wraps do not decide that: the peer's
+	 * cursor comes off the wire, and after a link failover prod can be
+	 * behind it.
+	 */
+	dst_len = min_t(size_t, conn->peer_rmbe_size - prod.count, len);
 	/* dst_len determines the maximum src_len */
 	if (sent.count + dst_len <= conn->sndbuf_desc->len) {
 		/* unwrapped src case: single chunk of entire dst_len */

-- 
2.56.0

[PATCH net v9 6/6] net/smc: bound the link failover cursor advance to the send buffer

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

On SMC-R link failover, smc_switch_cursor() rewinds the send and
producer cursors to the last completed CDC send, then advances them by
the distance the peer's consumer cursor is ahead. That cursor comes
from the wire: a forged one adds an arbitrary amount to sndbuf_space
and to the cursors, and the next RDMA write takes its source past the
send buffer.

Advance only for a count inside the peer's RMBE, and no further than
tx_curs_prep; a conforming peer's cursor is inside both. For any other
count the write that follows can end past the peer's RMBE and needs
"net/smc: keep an RDMA write inside the peer's RMBE" to bound it. Set
tx_curs_fin and local_tx_ctrl_fin from the advanced cursors instead of
adding to them: smc_cdc_tx_handler() can rewrite both while the switch
runs. The switch is still not serialized against the handler: a
completion handled inside it is still credited more than once and can
make the next write hand the peer bytes it has already read.

Fixes: c6f02ebeea3a ("net/smc: switch connections to alternate link")
Cc: stable@vger.kernel.org # see patch description, needs another patch
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
v9: tx_curs_fin and local_tx_ctrl_fin are copied from the advanced
tx_curs_sent and local_tx_ctrl.prod, which change only under send_lock,
instead of being added to (second point of [1]). The write past the
peer's RMBE that v8 introduced for a count past the RMBE is bounded in
5/6. The cap is tx_curs_prep and not the pre-failover tx_curs_sent: an
earlier failover may have rewound that one below the peer's cursor.

rxe, two links (v7.4-rc2 plus d7900fa51642, KASAN), send buffer 64K,
peer RMBE 16K: one byte and then 4096 confirmed; 12000 and 5000 sent and
not confirmed, the 5000 in two writes, 287 up to the end of the RMBE and
4713; all read by a conforming peer. A hook holds the pending
completions and hands the first one to smc_cdc_tx_handler() inside
smc_switch_cursor(), after it has read tx_curs_fin and
local_tx_ctrl_fin: unpatched and v8 leave local_tx_ctrl_fin at count
16713 (the RMBE is 16384 bytes) and tx_curs_fin 12000 bytes past
tx_curs_prep; v9 leaves tx_curs_fin on tx_curs_prep and
local_tx_ctrl_fin on prod. Without the hook, v8 ends the same run with
the cursors v9 leaves.

The first case of the note under v8's 4/4 (cursor forged one wrap
ahead at count 64, with 4096 bytes unconfirmed in one run and corked
in another) gives on v9 what that note reports for v8: the advance
stops at tx_curs_prep and nothing is written after the switch. v8 and
unpatched were not rerun for it.

Not changed: the switch is not serialized against the handler. The
handler takes send_lock inside the socket spinlock (smc_tx_pending())
and the switch holds send_lock only, so keeping it out means taking the
socket spinlock around send_lock in smc_switch_conns(). What remains,
on unpatched, v8 and v9 alike unless noted:
  - a completion handled inside the switch is credited to sndbuf_space
    by the handler and again by the advance: in the run above
    sndbuf_space is 77536 after the switch, for a 65536-byte send
    buffer. The old-link completions that follow set tx_curs_fin back
    and credit 4713 bytes once more: 82249. 3/6 keeps send() inside the
    buffer, but send() can then overwrite data the peer has not received
    (by inspection).
  - a completion between the first two cursor copies of the switch
    leaves tx_curs_sent behind local_tx_ctrl.prod. The next write starts
    at tx_curs_sent and lands at prod, that far past its place in the
    RMBE, so the peer is handed that many bytes a second time. In the
    first case under 4/6 the peer reads 600 bytes twice on all three. In
    the fourth case under 5/6, where the peer has read across the end of
    its RMBE before the resend, unpatched and v8 have the write refused
    and the connection lost; v9's goes out and the peer gets 600 bytes
    twice there too.
  - v9 only: the first case under 4/6 ends with sndbuf_space 1200 bytes
    over the buffer, against 600 on unpatched and v8. v9 sets
    tx_curs_fin back to tx_curs_sent, and the next old-link completion,
    when it is handled before the validation message's, credits those
    600 once more. Handled after it, all three end 600 over (by
    inspection).
  - the completion of the switch's validation message carries
    tx_curs_sent and steps tx_curs_fin back to it whenever a completion
    handled before it left tx_curs_fin ahead; across the end of the ring
    that step is credited as more than a whole ring (the mechanism is in
    3/6's message). In the fourth case under 5/6 it is credited 124472
    bytes for a 65536-byte send buffer on every variant, and on v9,
    where the last write goes out, sndbuf_space ends at 3x the buffer.
    v9's smc_curs_copy() removes one route to that state, the switch
    itself leaving tx_curs_fin ahead (by inspection); late old-link
    completions are still one.

[1] https://lore.kernel.org/all/179364387382.3131.13471253186660322977@kernel.org/
---
 net/smc/smc_core.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 0edbe6106c1a..b5625f1a8792 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1034,7 +1034,7 @@ static int smc_switch_cursor(struct smc_sock *smc, struct smc_cdc_tx_pend *pend,
 			     struct smc_wr_buf *wr_buf)
 {
 	struct smc_connection *conn = &smc->conn;
-	union smc_host_cursor cons, fin;
+	union smc_host_cursor cons, fin, prep;
 	int rc = 0;
 	int diff;
 
@@ -1043,16 +1043,22 @@ static int smc_switch_cursor(struct smc_sock *smc, struct smc_cdc_tx_pend *pend,
 	/* set prod cursor to old state, enforce tx_rdma_writes() */
 	smc_curs_copy(&conn->local_tx_ctrl.prod, &fin, conn);
 	smc_curs_copy(&cons, &conn->local_rx_ctrl.cons, conn);
-
-	if (smc_curs_comp(conn->peer_rmbe_size, &cons, &fin) < 0) {
-		/* cons cursor advanced more than fin, and prod was set
-		 * fin above, so now prod is smaller than cons. Fix that.
+	smc_curs_copy(&prep, &conn->tx_curs_prep, conn);
+
+	if (cons.count < conn->peer_rmbe_size &&
+	    smc_curs_comp(conn->peer_rmbe_size, &cons, &fin) < 0) {
+		/* cons cursor advanced more than fin, and prod was set fin
+		 * above: catch up. cons comes off the wire, so only for a
+		 * count inside the peer's RMBE and no further than
+		 * tx_curs_prep. smc_cdc_tx_handler() is not excluded and can
+		 * rewrite the fin cursors meanwhile: set them, do not add.
 		 */
-		diff = smc_curs_diff(conn->peer_rmbe_size, &fin, &cons);
+		diff = min(smc_curs_diff(conn->peer_rmbe_size, &fin, &cons),
+			   smc_curs_diff(conn->sndbuf_desc->len,
+					 &conn->tx_curs_sent, &prep));
 		smc_curs_add(conn->sndbuf_desc->len,
 			     &conn->tx_curs_sent, diff);
-		smc_curs_add(conn->sndbuf_desc->len,
-			     &conn->tx_curs_fin, diff);
+		smc_curs_copy(&conn->tx_curs_fin, &conn->tx_curs_sent, conn);
 
 		smp_mb__before_atomic();
 		atomic_add(diff, &conn->sndbuf_space);
@@ -1060,8 +1066,8 @@ static int smc_switch_cursor(struct smc_sock *smc, struct smc_cdc_tx_pend *pend,
 
 		smc_curs_add(conn->peer_rmbe_size,
 			     &conn->local_tx_ctrl.prod, diff);
-		smc_curs_add(conn->peer_rmbe_size,
-			     &conn->local_tx_ctrl_fin, diff);
+		smc_curs_copy(&conn->local_tx_ctrl_fin,
+			      &conn->local_tx_ctrl.prod, conn);
 	}
 	/* recalculate, value is used by tx_rdma_writes() */
 	atomic_set(&smc->conn.peer_rmbe_space, smc_write_space(conn));

-- 
2.56.0

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

[sashiko-bot]

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> smcr_cdc_msg_to_host() and smcd_cdc_msg_to_host() import the peer's
> producer cursor from the wire into conn->local_rx_ctrl.prod without
> bounding it to the RMB. smc_cdc_handle_urg_data_arrival() then uses its
> count as a raw index into the RMB, so a producer cursor past
> rmb_desc->len makes the receive tasklet read past the RMB, and
> recv(MSG_OOB) hands that byte to the application.
> 
> Bound the producer cursor count to rmb_desc->len when it comes off the
> wire, for SMC-R and SMC-D. Leave the consumer cursor alone: it indexes
> the peer's RMB, which can be larger than ours, so clamping it to our
> RMB would under-credit peer_rmbe_space and could stall sending to that
> peer. Conforming peers are unaffected.
> 
> Fixes: de8474eb9d50 ("net/smc: urgent data support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-b4-disp-0287c16e-v9-0-e5e8fa688177@proton.me?part=1

Re: [PATCH net v9 2/6] net/smc: bound the receive length to the RMB

[sashiko-bot]

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> smc_rx_recvmsg() sizes the copy from bytes_to_rcv and starts it past the
> bytes MSG_PEEK or splice() already took. A forged producer cursor drives
> bytes_to_rcv past the RMB or negative, and with a conforming peer it
> drops below the peeked count when a MSG_PEEK|MSG_WAITALL reader sleeps
> while another recv() consumes what it peeked. Either way the copy reads
> past the RMB into the reader's buffer. poll(), SIOCINQ and the deferred
> accept read the same raw count.
> 
> Make smc_rx_data_available() report at most what fits in the RMB behind
> the peeked and spliced bytes, never a negative count, and use it in all
> of these. Cache the RMB length for the unlocked wait, where rmb_desc may
> already be freed, and drop released bytes from splice_pending under the
> socket lock, with the consumer cursor update, so a reader cannot run
> between the two and count them twice.
> 
> Fixes: 952310ccf2d8 ("smc: receive data from RMBE")
> Fixes: 9014db202cb7 ("smc: add support for splice()")
> Fixes: a4b6539038c1 ("net/smc: fix data error when recvmsg with MSG_PEEK flag")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c@proton.me?part=2
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c%40proton.me?part=2
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c%40proton.me?part=1
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-b4-disp-0287c16e-v9-0-e5e8fa688177@proton.me?part=2

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

[sashiko-bot]

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> conn->sndbuf_space can run past the send buffer. After an SMC-R link
> failover, a late completion from the old link can move tx_curs_fin back,
> and the next completion credits the same bytes again; across the ring
> end, smc_curs_diff() credits the step back itself as more than a whole
> ring, so even with a conforming peer sndbuf_space can reach three times
> the buffer. The failover catch-up and SMC-D DMB merge (only the loopback
> device today) also credit it from the peer's consumer cursor without a
> bound. smc_tx_sendmsg() takes it as the write space, and the second
> chunk of its copy writes past the send buffer.
> 
> Clamp writespace, which sizes the copy, to sndbuf_desc->len, negative
> values included, so the copy stays in the send buffer. The over-credit
> itself is not undone: with sndbuf_space too large, send() can still
> overwrite data in the send buffer that the peer has not received.
> 
> Fixes: c6f02ebeea3a ("net/smc: switch connections to alternate link")
> 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>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-b4-disp-0287c16e-v9-0-e5e8fa688177@proton.me?part=3

Re: [PATCH net v9 4/6] net/smc: keep the source of an RDMA write inside the send buffer

[sashiko-bot]

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> smcr_tx_rdma_writes() sizes the first source SGE of the second
> destination chunk from the send cursor as it was before the first
> chunk, so that SGE runs past the send buffer when the buffer ends
> inside the second chunk. A physically contiguous send buffer goes out
> under the local DMA lkey, which does not bound the SGE; on rxe the peer
> is sent the memory that follows the buffer. A conforming peer gets there
> when a completion handled inside smc_switch_cursor() leaves tx_curs_sent
> behind local_tx_ctrl.prod. Otherwise it takes a peer_rmbe_space forged
> larger than the RMBE and a send buffer larger than the RMBE.
> 
> Size the SGE from the running offset, as smcd_tx_rdma_writes() does. A
> conforming write sends the same bytes.
> 
> Fixes: e6727f39004b ("smc: send data (through RDMA)")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013-b4-disp-f345196c-v7-0-ef3dddfbfa68%40proton.me?part=4
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-b4-disp-0287c16e-v9-0-e5e8fa688177@proton.me?part=4

Re: [PATCH net v9 5/6] net/smc: keep an RDMA write inside the peer's RMBE

[sashiko-bot]

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> smc_tx_rdma_writes() takes the length of a write from peer_rmbe_space
> and decides from the cursor wraps whether the destination is one chunk
> or two. Both follow the peer's consumer cursor, which nothing bounds: a
> forged one takes a one-chunk write past the peer's RMBE, and so does a
> conforming peer that keeps reading across the end of its RMBE before
> the resend after an SMC-R link failover. With one RMBE per RMB, as
> Linux allocates them, the peer's HCA refuses the write and the link
> goes down.
> 
> Write at most one RMBE per call and end the first chunk no later than
> the RMBE, whatever the wraps say. A conforming peer outside failover
> gets the chunks it got before. A write that was one chunk can now be
> two, which needs "net/smc: keep the source of an RDMA write inside the
> send buffer"; the first chunk needs prod.count inside the RMBE, which
> "net/smc: bound the link failover cursor advance to the send buffer"
> keeps at failover.
> 
> Fixes: e6727f39004b ("smc: send data (through RDMA)")
> Fixes: c6f02ebeea3a ("net/smc: switch connections to alternate link")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013-b4-disp-f345196c-v7-0-ef3dddfbfa68%40proton.me?part=4
> Cc: stable@vger.kernel.org # see patch description, needs two other patches
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-b4-disp-0287c16e-v9-0-e5e8fa688177@proton.me?part=5

Re: [PATCH net v9 6/6] net/smc: bound the link failover cursor advance to the send buffer

[sashiko-bot]

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> On SMC-R link failover, smc_switch_cursor() rewinds the send and
> producer cursors to the last completed CDC send, then advances them by
> the distance the peer's consumer cursor is ahead. That cursor comes
> from the wire: a forged one adds an arbitrary amount to sndbuf_space
> and to the cursors, and the next RDMA write takes its source past the
> send buffer.
> 
> Advance only for a count inside the peer's RMBE, and no further than
> tx_curs_prep; a conforming peer's cursor is inside both. For any other
> count the write that follows can end past the peer's RMBE and needs
> "net/smc: keep an RDMA write inside the peer's RMBE" to bound it. Set
> tx_curs_fin and local_tx_ctrl_fin from the advanced cursors instead of
> adding to them: smc_cdc_tx_handler() can rewrite both while the switch
> runs. The switch is still not serialized against the handler: a
> completion handled inside it is still credited more than once and can
> make the next write hand the peer bytes it has already read.
> 
> Fixes: c6f02ebeea3a ("net/smc: switch connections to alternate link")
> Cc: stable@vger.kernel.org # see patch description, needs another patch
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-b4-disp-0287c16e-v9-0-e5e8fa688177@proton.me?part=6