[PATCH net v5 0/2] net/smc: order the CDC receive path against buffer publication

[PATCH net v5 0/2] net/smc: order the CDC receive path against buffer publication

[Bryam Vargas via B4 Relay]

A CDC can reach a connection before its RMB, or its SMC-D nocopy send
buffer, is set, and smc_cdc_msg_recv_action() then derefs a NULL
pointer in softirq. For the SMC-R RMB (1/2) the CDC comes from a peer
in the link group that predicts the next token. For the nocopy sndbuf
(2/2, SMC-D loopback only) it comes from the old peer of a reused DMB.
Each patch sets its pointer last with a release store, and the rx path
drops the CDC (1/2) or skips the nocopy accounting (2/2) while the
pointer is unset.

1/2 goes back to 69cb7dc0218b (v4.18), 2/2 to cc0ab806fc52 (v6.10).
2/2 relies on b395dd319cea ("net/smc: do not dereference an unset send
buffer on the SMC-D teardown path"), already in 6.12.y, 6.18.y and
7.2.y.

The old peer can also close the new connection or, once the sndbuf is
attached, push the new connection's sndbuf_space past the sndbuf size,
with or without this series; 3/4 of my v7 series [1] bounds the send()
copy, not sndbuf_space. FWIW smc_conn_free() releasing the RMB before
the connection leaves the token tree [2] is pre-existing too.

1/2 conflicts in __smc_buf_create() with v7 2/4: keep v7's rmb_len
assignment at the top of the is_rmb block, before the release store,
and the tree is the same in either merge order.

v7 1/4 adds an rmb_desc->len load in smcr_cdc_msg_to_host() that any CDC
in 1/2's window reaches (by inspection), as Sashiko flagged [3]. In C,
net already loads rmb_desc->len later in smc_cdc_msg_recv_action(), as I
told Paolo [4], but in my x86 and s390x builds that load only runs when
the cursor wraps differ. 1/2 drops the CDC before both loads.

Tested with KASAN on x86 (v7.3-rc6, whose net/smc differs from net only
by 64a6b36b0e18) and on s390x under QEMU (net). Unpatched, two cases
with the cursor wraps apart reach their NULL loads: a CDC forged with
the next token (1/2, over rxe), and the old peer's consumer update to
a reused DMB (2/2). In the second, nothing is forged, but the old peer
times its read off a test-only counter and the new connect() is starved
of CPU. KASAN reports a null-ptr-deref on x86; on s390x, with the
default lowcore, each load reads page 0 and nothing faults. With the
series, and test-only delays holding the windows open, test-only probes
see the CDC dropped and the update skip the accounting with the cursor
wraps apart. With either NULL check removed they see the NULL load
again, and with either store moved back they see a CDC handled before
the fields that go with the pointer are set. On top of the v7 series and
two other net fixes [5] [6], the patched runs read the same. The
ordering was also checked against LKMM. Test patch and logs on request.

v5:
 - Split in two, one patch per Fixes: tag, and rebased on net, which
   now has the SMC-D teardown fixes 36cdf5d48ca1 and b395dd319cea.
 - Dropped the sndbuf_desc check on the consumer trigger, unneeded
   after b395dd319cea, and the tasklet's rmb_desc acquire, as
   smcd->lock orders that load (by inspection). The sndbuf_desc
   acquire moved into the nocopy branch.
 - Kept per-buffer checks: one readiness flag would drop a conforming
   client's first CDC on SMC-D, and registering the connection later
   would reorder the connect and listen paths.
 - The Closes: tag now points at Sashiko's report [3], not at a review
   of v2. Dropped the R-b tags from Dust Li and Sidraya Jayagond, since
   the patch changed.
v4: https://lore.kernel.org/all/20260728-b4-disp-52ee4e7d-v4-1-0dda94b0f397@proton.me/
v3: https://lore.kernel.org/all/20260716-b4-disp-aa52955a-v3-1-03a4411a7549@proton.me/
v2: https://lore.kernel.org/all/20260714-b4-disp-835288a6-v2-1-581555ef2145@proton.me/
v1: https://lore.kernel.org/all/20260711-b4-disp-c36a9798-v1-1-340b0c6053fb@proton.me/

[1] https://lore.kernel.org/all/20261013-b4-disp-f345196c-v7-0-ef3dddfbfa68@proton.me/
[2] https://lore.kernel.org/all/20260729165306.BE7C21F000E9@smtp.kernel.org/
[3] https://lore.kernel.org/all/sashiko-outbox-164917@kernel.org/
[4] https://lore.kernel.org/all/20260728172552.69349-1-hexlabsecurity@proton.me/
[5] https://lore.kernel.org/all/20261013-b4-disp-872a3689-v1-1-2a4c63b04761@proton.me/
[6] https://lore.kernel.org/all/20261013-b4-disp-193a2a59-v1-1-ed88ac10a41c@proton.me/

---
Bryam Vargas (2):
      net/smc: order the CDC receive path against RMB publication
      net/smc: skip the DMB nocopy accounting until the send buffer is set

 net/smc/smc_cdc.c  | 16 ++++++++++++++--
 net/smc/smc_core.c | 10 ++++++----
 2 files changed, 20 insertions(+), 6 deletions(-)
---
base-commit: 513444cc62e231a11c9b15ea88de0890da9b2b4f
change-id: 20261015-b4-disp-2a8d85ef-2748bca10e1a

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

[PATCH net v5 1/2] net/smc: order the CDC receive path against RMB publication

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

smc_conn_create() adds an SMC-R connection to the token tree before
smc_buf_create() sets conn->rmb_desc. Tokens come from one global
counter, so a peer in the link group can predict the next one. A CDC
with that token, arriving in the window, makes
smc_cdc_msg_recv_action() dereference a NULL rmb_desc in softirq.

Set rmb_desc with a release store after the receive fields that go
with it, and drop a CDC in smc_cdc_rx_handler() while rmb_desc is
unset. A conforming peer learns the token only from the CLC accept or
confirm, sent after smc_buf_create(), so nothing it sends is dropped.
The SMC-D receive tasklet needs no check: smc_ism_set_conn() publishes
the connection under smcd->lock, which smcd_handle_irq() also takes.

Fixes: 69cb7dc0218b ("net/smc: add common buffer size in send and receive buffer descriptors")
Closes: https://sashiko.dev/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c@proton.me?part=1
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/smc/smc_cdc.c  | 6 +++++-
 net/smc/smc_core.c | 3 ++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index 32d6d03df321..7bf902ebd7ab 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -483,7 +483,11 @@ static void smc_cdc_rx_handler(struct ib_wc *wc, void *buf)
 	lgr = smc_get_lgr(link);
 	read_lock_bh(&lgr->conns_lock);
 	conn = smc_lgr_find_conn(ntohl(cdc->token), lgr);
-	if (!conn || conn->out_of_sync) {
+	/* The connection is in the token tree before __smc_buf_create()
+	 * sets its RMB: drop a CDC that comes earlier. rmb_desc is never
+	 * cleared, so the callees can load it plainly.
+	 */
+	if (!conn || conn->out_of_sync || !smp_load_acquire(&conn->rmb_desc)) {
 		read_unlock_bh(&lgr->conns_lock);
 		return;
 	}
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 9974149659c2..26f19a00c159 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -2502,12 +2502,13 @@ static int __smc_buf_create(struct smc_sock *smc, bool is_smcd, bool is_rmb)
 	}
 
 	if (is_rmb) {
-		conn->rmb_desc = buf_desc;
 		conn->rmbe_size_comp = bufsize_comp;
 		smc->sk.sk_rcvbuf = bufsize * 2;
 		atomic_set(&conn->bytes_to_rcv, 0);
 		conn->rmbe_update_limit =
 			smc_rmb_wnd_update_limit(buf_desc->len);
+		/* Set last; pairs with the acquire in smc_cdc_rx_handler() */
+		smp_store_release(&conn->rmb_desc, buf_desc);
 		if (is_smcd)
 			smc_ism_set_conn(conn); /* map RMB/smcd_dev to conn */
 	} else {

-- 
2.56.0

Re: [PATCH net v5 1/2] net/smc: order the CDC receive path against RMB publication

[sashiko-bot]

> From: Bryam Vargas <hexlabsecurity@proton.me>
> 
> smc_conn_create() adds an SMC-R connection to the token tree before
> smc_buf_create() sets conn->rmb_desc. Tokens come from one global
> counter, so a peer in the link group can predict the next one. A CDC
> with that token, arriving in the window, makes
> smc_cdc_msg_recv_action() dereference a NULL rmb_desc in softirq.
> 
> Set rmb_desc with a release store after the receive fields that go
> with it, and drop a CDC in smc_cdc_rx_handler() while rmb_desc is
> unset. A conforming peer learns the token only from the CLC accept or
> confirm, sent after smc_buf_create(), so nothing it sends is dropped.
> The SMC-D receive tasklet needs no check: smc_ism_set_conn() publishes
> the connection under smcd->lock, which smcd_handle_irq() also takes.
> 
> Fixes: 69cb7dc0218b ("net/smc: add common buffer size in send and receive buffer descriptors")
> Closes: https://sashiko.dev/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c@proton.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. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-b4-disp-2a8d85ef-v5-0-6b795e8263c9@proton.me?part=1

[PATCH net v5 2/2] net/smc: skip the DMB nocopy accounting until the send buffer is set

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

With SMC-D DMB nocopy (the loopback device), smcd_buf_attach() sets
conn->sndbuf_desc after the connection can already receive a CDC. When
a DMB is reused, the peer of its previous connection can still hold
the DMB token, and its next consumer update makes
smc_cdc_msg_recv_action() dereference the NULL sndbuf_desc in the
nocopy accounting.

Set sndbuf_desc with a release store after sndbuf_space, and skip the
nocopy accounting while sndbuf_desc is unset. Before the attach the
current peer has consumed nothing of ours, so no update from it is
lost. The rest of the receive path handles an unset sndbuf_desc since
commit b395dd319cea ("net/smc: do not dereference an unset send buffer
on the SMC-D teardown path").

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>
---
 net/smc/smc_cdc.c  | 10 +++++++++-
 net/smc/smc_core.c |  7 ++++---
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index 7bf902ebd7ab..d6825f48f568 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -333,6 +333,7 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
 	union smc_host_cursor cons_old, prod_old;
 	struct smc_connection *conn = &smc->conn;
 	int diff_cons, diff_prod, diff_tx;
+	struct smc_buf_desc *sndbuf_desc;
 
 	smc_curs_copy(&prod_old, &conn->local_rx_ctrl.prod, conn);
 	smc_curs_copy(&cons_old, &conn->local_rx_ctrl.cons, conn);
@@ -353,12 +354,19 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
 		 * peer RMB, then update tx_curs_fin and sndbuf_space
 		 * here since peer has already consumed the data.
 		 */
+		sndbuf_desc = NULL;
 		if (conn->lgr->is_smcd &&
 		    smc_ism_support_dmb_nocopy(conn->lgr->smcd)) {
+			/* NULL until smcd_buf_attach(), but the old peer of
+			 * a reused DMB can already signal us.
+			 */
+			sndbuf_desc = smp_load_acquire(&conn->sndbuf_desc);
+		}
+		if (sndbuf_desc) {
 			/* Calculate consumed data and
 			 * increment free send buffer space.
 			 */
-			diff_tx = smc_curs_diff(conn->sndbuf_desc->len,
+			diff_tx = smc_curs_diff(sndbuf_desc->len,
 						&conn->tx_curs_fin,
 						&conn->local_rx_ctrl.cons);
 			/* increase local sndbuf space and fin_curs */
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 26f19a00c159..9322c27145df 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -2603,9 +2603,10 @@ int smcd_buf_attach(struct smc_sock *smc)
 	buf_desc->cpu_addr =
 		(u8 *)buf_desc->cpu_addr + sizeof(struct smcd_cdc_msg);
 	buf_desc->len -= sizeof(struct smcd_cdc_msg);
-	conn->sndbuf_desc = buf_desc;
-	conn->sndbuf_desc->used = 1;
-	atomic_set(&conn->sndbuf_space, conn->sndbuf_desc->len);
+	buf_desc->used = 1;
+	atomic_set(&conn->sndbuf_space, buf_desc->len);
+	/* Set last; pairs with the acquire in smc_cdc_msg_recv_action() */
+	smp_store_release(&conn->sndbuf_desc, buf_desc);
 	return 0;
 
 free:

-- 
2.56.0

Re: [PATCH net v5 2/2] net/smc: skip the DMB nocopy accounting until the send buffer is set

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net/smc: state corruption from reused DMB peer CDC messages
--

commit 9ad2bd35a07a32a00da8818fbbfd176e9bdd1557
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/smc: skip the DMB nocopy accounting until the send buffer is set

This patch mitigates a null pointer dereference in smc-d dmb nocopy when an
old peer reusing a dmb sends a cdc message before the new connection sets
sndbuf_desc. It skips nocopy accounting while the send buffer is unset.

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index 7bf902ebd7ab6..d6825f48f5688 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c
> @@ -333,6 +333,7 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>  	union smc_host_cursor cons_old, prod_old;
>  	struct smc_connection *conn = &smc->conn;
>  	int diff_cons, diff_prod, diff_tx;
> +	struct smc_buf_desc *sndbuf_desc;
>  
>  	smc_curs_copy(&prod_old, &conn->local_rx_ctrl.prod, conn);
>  	smc_curs_copy(&cons_old, &conn->local_rx_ctrl.cons, conn);

[Severity: High]
Does this code unconditionally overwrite the new connection's receive control
block with the old peer's cursors?

Because the old peer of a reused dmb can still send cdc messages, and these
messages lack sequence numbers or token fields, old updates are blindly
processed.

Looking right after this block in smc_cdc_msg_recv_action(),
smc_cdc_msg_to_host() is called to update conn->local_rx_ctrl. Then
diff_cons is calculated from the old cursors and added to
conn->peer_rmbe_space.

Could this regression corrupt the peer's receive window space by adding a
bogus difference?

[ ... ]

> @@ -353,12 +354,19 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>  		 * peer RMB, then update tx_curs_fin and sndbuf_space
>  		 * here since peer has already consumed the data.
>  		 */
> +		sndbuf_desc = NULL;
>  		if (conn->lgr->is_smcd &&
>  		    smc_ism_support_dmb_nocopy(conn->lgr->smcd)) {
> +			/* NULL until smcd_buf_attach(), but the old peer of
> +			 * a reused DMB can already signal us.
> +			 */
> +			sndbuf_desc = smp_load_acquire(&conn->sndbuf_desc);
> +		}
> +		if (sndbuf_desc) {
>  			/* Calculate consumed data and
>  			 * increment free send buffer space.
>  			 */
> -			diff_tx = smc_curs_diff(conn->sndbuf_desc->len,
> +			diff_tx = smc_curs_diff(sndbuf_desc->len,
>  						&conn->tx_curs_fin,
>  						&conn->local_rx_ctrl.cons);

[Severity: High]
If the old peer sends an update after the new connection has already set
sndbuf_desc, is this check bypassed?

If so, could diff_tx be calculated from the old peer's cursors and added
to conn->sndbuf_space, corrupting the send buffer space?

Furthermore, looking at the end of smc_cdc_msg_recv_action(), diff_prod is
calculated and added to conn->bytes_to_rcv, and the socket is woken up via
sk_data_ready(). By avoiding the null pointer dereference, does this
regression instead corrupt the available data size and trick the new
connection into reading garbage data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-b4-disp-2a8d85ef-v5-0-6b795e8263c9@proton.me?part=2