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

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

[Bryam Vargas via B4 Relay]

A peer can make smc_cdc_msg_recv_action() deref a NULL pointer in
softirq with a CDC that reaches a connection before its RMB, or its
SMC-D nocopy send buffer, is set: an SMC-R peer in the link group
predicts the token (1/3), and the old peer of a reused DMB still
holds its token (3/3). 1/3 and 3/3 are v5's two patches: each sets
its pointer last with a release store, and the rx path drops the CDC
(1/3) or skips the nocopy accounting (3/3) while the pointer is unset.

2/3 is new. Sashiko found [1] that the NULL test of 1/3 misses a
handshake that retries: when the SMC-Rv2 attempt of the listen worker
fails in smc_listen_rdma_reg(), smc_conn_abort() releases the buffers
(to the link group's pool or, after a failed MR registration, to the
allocator) and leaves rmb_desc and sndbuf_desc pointing at them, and
the SMC-Rv1 retry registers the connection under a new token with
those stale pointers. A CDC with that token is then handled on a
buffer the connection no longer owns. The NULL test of 1/3 does not
see it, and the bug is there without 1/3 too. A peer fails the
SMC-Rv2 attempt with a negative CONFIRM_RKEY response. 2/3 clears
both pointers in smc_conn_create() under the socket spinlock, and
tests rmb_desc again under that lock in smc_cdc_msg_recv_action(),
for a handler that found the connection before the clear. 2/3 needs
1/3, and 3/3 has a line of 2/3 as context.

The clear is at registration and not at release: every registration
goes through smc_conn_create(), while smc_conn_free() skips the
release when the link group is already unlinked and would leave the
pointers set there.

Both NULL tests are needed. The one in smc_cdc_rx_handler() (1/3)
also keeps the CDC out of smc_cdc_msg_validate(), which runs before
the socket spinlock is taken; the one in smc_cdc_msg_recv_action()
(2/3) is the one under that lock.

Not fixed here, all pre-existing:
 - A handler that found the connection before smc_conn_free()
   unregistered it still runs on the released RMB (the window of
   Weiming Shi's patch [2]). 2/3 does not close that window: its
   clear comes at the next smc_conn_create(). Such a handler can also
   still reach smc_cdc_msg_validate() on the retried connection, as
   Sashiko noted on Hidayath Khan's patch [3]. 2/3 clears the two
   pointers only; whatever else the failed attempt left in the
   connection stays.
 - The old peer of a reused DMB, rated [Critical] by Sashiko on v5
   2/2 [4]: its CDCs are still applied to the new connection. They
   can close it, leave their consumer cursor in it before the attach
   or, once the sndbuf is attached, add to its sndbuf_space. 3/3
   removes the NULL deref only. 3/6 of my v9 cursor series [5] bounds
   the send() copy to the send buffer; it does not correct
   sndbuf_space.
 - The unbounded producer cursor: 1/6 of [5].

1/3 conflicts in __smc_buf_create() with 2/6 of [5]: keep [5]'s
rmb_len store at the top of the is_rmb block, before the release
store, and the tree is the same in either order.

Tested with KASAN on x86 over rxe: v7.4-rc2 plus the two net/smc commits
net has on top (d7900fa51642, e227b5115828), plus the series. rxe has no
RoCEv1 GID, so the SMC-Rv1 retry is not reachable on it unmodified: a
test-only change lets the SMC-Rv1 lookup take the RoCEv2 GID. The retry
then joins the link group of the failed attempt, which is what lets the
peer's CDC reach it. On a device with both GIDs the retry joins a link
group the peer has a link in when the SMC-Rv1 part of the proposal names
the GID and MAC of a link of that group: of the failed attempt's SMC-Rv2
link (smcr_lgr_match() does not compare the version), or of a link of an
SMC-Rv1 link group of the same peer. By inspection, not run on such a
device.

Test-only knobs fail the SMC-Rv2 attempt in one of two ways (the
client answers CONFIRM_RKEY negatively, or the server's MR
registration fails), fail the retry's RMB allocation, and delay the
retry between its registration and smc_buf_create(); the client
sends a CDC forged with the retried connection's token in that
window.

Without 2/3 the forged CDC gets past the NULL test of 1/3. After the
negative CONFIRM_RKEY, the case a peer forces, the descriptor is
back in the link group's pool: KASAN is silent and test-only
counters show smc_cdc_msg_recv_action() running on it. After the
failed MR registration, which the test forces locally and I have
not shown a peer forcing, KASAN reports a slab-use-after-free read
in smc_cdc_msg_recv_action(). With the retry's RMB allocation failed
as well, and no forged CDC, it reports one in smc_conn_free(),
followed by a GPF in smcr_buf_unuse(). With the series the forged
CDC is dropped, KASAN is silent, and the retried connection
completes and carries data (over TCP when its RMB allocation is
failed).

Three variants of the series, counters only (no KASAN boot of them):
 - the clear removed, the second test kept: as without 2/3;
 - the second test removed: a handler delayed between the lookup and
   the socket spinlock across the retry finds rmb_desc NULL where
   smc_cdc_msg_recv_action() loads it (the access was not run);
 - the test in smc_cdc_rx_handler() removed: a forged CDC with the
   failover_validation flag reaches smc_cdc_msg_validate(), at the
   first registration and at the retry. The series drops it in both.
The clear of sndbuf_desc is covered by a counter at the registration
only: no run fails the retry's send buffer create.

On the client, a counter shows a non-blocking connect() that failed
after smc_buf_create() (a test-only knob shuts the server's CLC
socket down) entering smc_conn_create() again on the second
connect() after the failure, with the old pointers before 2/3 and
NULL after. Nothing else was measured on the client: no CDC was sent
at it.

v5's cases give the same results with the series: a CDC forged with the
next token is dropped at the first registration, and the old peer's
update to a reused DMB (loopback) skips the accounting.

The retry and client cases also ran on s390x under QEMU TCG (rxe, two
vCPUs, KASAN and lockdep), by the counters only (the boots that give the
two KASAN reports were not run there), once with deterministic
scheduling and once with the vCPUs on host threads: the same results as
on x86. With the vCPUs on host threads the delayed handler missed its
window in three of nine boots, none of them of the series; those cells
are not measured. Not run on s390 hardware. v5's cases were not repeated
on s390x: 1/3 and 3/3 are v5's code.

Under lockdep (the x86 tree with PROVE_LOCKING, one boot per case, and
the s390x runs) the series is silent in the retry and client cases and,
on x86, in v5's cases; the tree without it is silent in the retry cases.
With spin_lock() in place of spin_lock_bh() in smc_conn_create(),
lockdep reports an inconsistent lock state on slock-AF_SMC.

Checked with herd7 against LKMM: the handler neither loads NULL after
its test nor sees the new descriptor before its length. Without the lock
or without the second test it can do either; with a plain load in the
second test, the latter. Test patch and logs on request.

v6:
 - New 2/3, for the retry Sashiko found on v5 1/2 [1].
 - 1/3: code as v5 1/2, two comments reworded: the one at the test
   no longer says that rmb_desc is never cleared.
 - 3/3: code as v5 2/2. The message now says that the rest of the
   old peer's update is still applied to the new connection, in
   answer to [4]; v5's said only that the current peer loses none.
 - Rebased on net.
v5: https://lore.kernel.org/all/20261015-b4-disp-2a8d85ef-v5-0-6b795e8263c9@proton.me/
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/179241208347.3135.18021879529736748765@kernel.org/
[2] https://lore.kernel.org/all/20261021171256.166870-2-bestswngs@gmail.com/
[3] https://lore.kernel.org/all/20261006073550.1595003-1-hidayath@linux.ibm.com/
[4] https://lore.kernel.org/all/179241208501.3135.14193534440423847161@kernel.org/
[5] https://lore.kernel.org/all/20261112-b4-disp-0287c16e-v9-0-e5e8fa688177@proton.me/

---
Bryam Vargas (3):
      net/smc: order the CDC receive path against RMB publication
      net/smc: clear the buffer pointers before registering a connection
      net/smc: skip the DMB nocopy accounting until the send buffer is set

 net/smc/smc_cdc.c  | 20 ++++++++++++++++++--
 net/smc/smc_core.c | 19 +++++++++++++++----
 2 files changed, 33 insertions(+), 6 deletions(-)
---
base-commit: 2a24ba1694f14299fbde3fa107e5d56629f2f8b6
change-id: 20261112-b4-disp-1c8d3a53-fa3dcbf64426

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

[PATCH net v6 1/3] 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>
---
The Closes tag is Sashiko's report of this window, in its review of
v6 of the cursor series. v9 of that series is at
https://lore.kernel.org/all/20261112-b4-disp-0287c16e-v9-0-e5e8fa688177@proton.me/
and its 2/6 conflicts with this patch in __smc_buf_create(): keep
its rmb_len store at the top of the is_rmb block, before the release
store. The tree is the same in either order.
---
 net/smc/smc_cdc.c  | 5 ++++-
 net/smc/smc_core.c | 3 ++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index 32d6d03df321..ee5ba6c7a5b4 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -483,7 +483,10 @@ 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.
+	 */
+	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 a3164201c142..02269cede641 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -2544,12 +2544,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; smc_cdc_rx_handler() tests it with an acquire */
+		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 v6 1/3] 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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-b4-disp-1c8d3a53-v6-0-36a38ae35021@proton.me?part=1

[PATCH net v6 2/3] net/smc: clear the buffer pointers before registering a connection

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

smc_conn_abort() releases the buffers of a failed handshake attempt
and leaves conn->rmb_desc and conn->sndbuf_desc pointing at them.
When the SMC-Rv2 attempt of the listen worker fails in
smc_listen_rdma_reg(), which a peer forces with a negative
CONFIRM_RKEY response, the SMC-Rv1 retry registers the connection
under a new token with the stale pointers; a client can do the same
with further connect() calls after a failed non-blocking one. A CDC
with that token then passes the rmb_desc test in
smc_cdc_rx_handler() and is handled on a buffer the connection no
longer owns, and a retry that cannot create its buffers releases the
stale ones a second time.

Clear both pointers in smc_conn_create(), which every registration
goes through, under the socket spinlock, and let
smc_cdc_msg_recv_action(), which runs under it, return while
rmb_desc is unset: a handler that found the connection before the
clear then does not dereference NULL.

Fixes: 35112271672a ("net/smc: Reset connection when trying to use SMCRv2 fails.")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/179241208347.3135.18021879529736748765@kernel.org/
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
The Fixes tag names the listen retry, which a remote peer reaches.
The connect() re-entry needs the local caller and is older (by
inspection); I have not dated it.

The clear takes the slock with spin_lock_bh(): smc_conn_create() runs in
process context with the socket owned, and the CDC handlers take the
same lock with bh_lock_sock(). Lockdep is silent on it, and reports an
inconsistent lock state with spin_lock() there.

After the second test the function loads rmb_desc plainly: the clear
needs the lock it runs under, so a pointer seen set stays set. The
SMC-D tasklet loads rmb_desc before that lock and the clear cannot
meet it: the connection is unpublished from the device, and its
tasklet killed or the connection marked killed, before a retry
reaches smc_conn_create().
---
 net/smc/smc_cdc.c  | 7 ++++++-
 net/smc/smc_core.c | 9 +++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index ee5ba6c7a5b4..b3bcc206efe3 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -334,6 +334,10 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
 	struct smc_connection *conn = &smc->conn;
 	int diff_cons, diff_prod, diff_tx;
 
+	/* smc_conn_create() can have cleared rmb_desc */
+	if (!smp_load_acquire(&conn->rmb_desc))
+		return;
+
 	smc_curs_copy(&prod_old, &conn->local_rx_ctrl.prod, conn);
 	smc_curs_copy(&cons_old, &conn->local_rx_ctrl.cons, conn);
 	smc_cdc_msg_to_host(&conn->local_rx_ctrl, cdc, conn);
@@ -484,7 +488,8 @@ static void smc_cdc_rx_handler(struct ib_wc *wc, void *buf)
 	read_lock_bh(&lgr->conns_lock);
 	conn = smc_lgr_find_conn(ntohl(cdc->token), lgr);
 	/* The connection is in the token tree before __smc_buf_create()
-	 * sets its RMB: drop a CDC that comes earlier.
+	 * sets its RMB: drop a CDC that comes earlier. rmb_desc is cleared
+	 * under the socket spinlock; smc_cdc_msg_recv_action() tests it there.
 	 */
 	if (!conn || conn->out_of_sync || !smp_load_acquire(&conn->rmb_desc)) {
 		read_unlock_bh(&lgr->conns_lock);
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 02269cede641..9ef9019bc497 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -2024,6 +2024,15 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
 	spinlock_t *lgr_lock;
 	int rc = 0;
 
+	/* A retried handshake comes back with the buffer pointers of the
+	 * failed attempt still set: clear them before the connection is
+	 * registered. smc_cdc_msg_recv_action() tests rmb_desc under this lock.
+	 */
+	spin_lock_bh(&smc->sk.sk_lock.slock);
+	WRITE_ONCE(conn->rmb_desc, NULL);
+	WRITE_ONCE(conn->sndbuf_desc, NULL);
+	spin_unlock_bh(&smc->sk.sk_lock.slock);
+
 	lgr_list = ini->is_smcd ? &ini->ism_dev[ini->ism_selected]->lgr_list :
 				  &smc_lgr_list.list;
 	lgr_lock = ini->is_smcd ? &ini->ism_dev[ini->ism_selected]->lgr_lock :

-- 
2.56.0

Re: [PATCH net v6 2/3] net/smc: clear the buffer pointers before registering a connection

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] net/smc: use-after-free in smc_cdc_msg_recv_action() when
  rmb_desc is cleared in smc_conn_create()
--

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

net/smc: clear the buffer pointers before registering a connection

This commit clears conn->rmb_desc and conn->sndbuf_desc under the socket
spinlock in smc_conn_create() and checks conn->rmb_desc in
smc_cdc_msg_recv_action() to avoid using stale buffer pointers after a
failed handshake attempt.

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index ee5ba6c7a5b4a..b3bcc206efe3e 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c
> @@ -334,6 +334,10 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>  	struct smc_connection *conn = &smc->conn;
>  	int diff_cons, diff_prod, diff_tx;
>  
> +	/* smc_conn_create() can have cleared rmb_desc */
> +	if (!smp_load_acquire(&conn->rmb_desc))
> +		return;
> +
>  	smc_curs_copy(&prod_old, &conn->local_rx_ctrl.prod, conn);
>  	smc_curs_copy(&cons_old, &conn->local_rx_ctrl.cons, conn);
>  	smc_cdc_msg_to_host(&conn->local_rx_ctrl, cdc, conn);
[ ... ]
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 02269cede641e..9ef9019bc4974 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -2024,6 +2024,15 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
>  	spinlock_t *lgr_lock;
>  	int rc = 0;
>  
> +	/* A retried handshake comes back with the buffer pointers of the
> +	 * failed attempt still set: clear them before the connection is
> +	 * registered. smc_cdc_msg_recv_action() tests rmb_desc under this lock.
> +	 */
> +	spin_lock_bh(&smc->sk.sk_lock.slock);
> +	WRITE_ONCE(conn->rmb_desc, NULL);
> +	WRITE_ONCE(conn->sndbuf_desc, NULL);
> +	spin_unlock_bh(&smc->sk.sk_lock.slock);
> +
>  	lgr_list = ini->is_smcd ? &ini->ism_dev[ini->ism_selected]->lgr_list :
>  				  &smc_lgr_list.list;
>  	lgr_lock = ini->is_smcd ? &ini->ism_dev[ini->ism_selected]->lgr_lock :

[Severity: High]
Can smc_cdc_msg_recv_action() still dereference freed conn->rmb_desc,
conn->sndbuf_desc and conn->lgr when the buffer pointers are cleared in
smc_conn_create() instead of smc_conn_free()?

In smc_conn_free(), smc_buf_unuse() releases or frees the buffers before
smc_lgr_unregister_conn() removes the connection from lgr->conns_all, and
smc_lgr_put() and smc_lgr_cleanup_early() then drop the link group:

net/smc/smc_core.c:smc_conn_free() {
	...
	if (!list_empty(&lgr->list))
		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
	smc_lgr_unregister_conn(conn);
	...
	smc_lgr_put(lgr); /* lgr_hold in smc_conn_create() */
}

Because bh_lock_sock(&smc->sk) in smc_cdc_msg_recv() only acquires
sk_lock.slock without waiting for process-context lock_sock(), an
incoming CDC message can run smc_cdc_msg_recv_action() before
smc_conn_create() is called (or when no retry occurs, such as when the
handshake aborts without a fallback device):

kworker                                   <softirq>
smc_listen_rdma_reg()
  ...
                                          smc_cdc_rx_handler()
                                            read_lock_bh(&lgr->conns_lock)
                                            conn = smc_lgr_find_conn(...)
                                            !smp_load_acquire(&conn->rmb_desc)
                                            sock_hold(&smc->sk)
                                            read_unlock_bh(&lgr->conns_lock)
  smc_conn_abort()
    smc_conn_free()
      smc_buf_unuse()
        smc_buf_free() -> kfree(rmb_desc)
      smc_lgr_unregister_conn()
      smc_lgr_put()
    smc_lgr_cleanup_early()
                                            smc_cdc_msg_recv()
                                              bh_lock_sock(&smc->sk)
                                              smc_cdc_msg_recv_action()
                                                !smp_load_acquire(&conn->rmb_desc)
                                                conn->rmb_desc->len
                                                  <-- use-after-free

Even when a retry does reach smc_conn_create(), if the in-flight
smc_cdc_rx_handler() acquires bh_lock_sock(&smc->sk) after the retry has
called __smc_buf_create(), conn->rmb_desc is non-NULL again and
smc_cdc_msg_recv_action() applies the stale CDC from the aborted attempt
to the new connection.

Clearing conn->rmb_desc and conn->sndbuf_desc under sk_lock.slock in
smc_conn_free() after smc_lgr_unregister_conn() and before
smc_buf_unuse() would close both windows.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-b4-disp-1c8d3a53-v6-0-36a38ae35021@proton.me?part=2

[PATCH net v6 3/3] 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. Nothing of ours can have
been consumed before the attach, so the skipped accounting has nothing
to credit. The update's cursors and flags are still applied to the new
connection, as those of a CDC that leaves the consumer cursor alone
already are. 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 b3bcc206efe3..4a4a279b405b 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_conn_create() can have cleared rmb_desc */
 	if (!smp_load_acquire(&conn->rmb_desc))
@@ -357,12 +358,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 9ef9019bc497..982cfc79ee1c 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -2654,9 +2654,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 v6 3/3] 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 to consider:
- [High] net/smc: cursor and sndbuf_space corruption in
  smc_cdc_msg_recv_action()
--

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

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

This commit publishes conn->sndbuf_desc with smp_store_release() in
smcd_buf_attach() and skips the SMC-D DMB nocopy accounting in
smc_cdc_msg_recv_action() when conn->sndbuf_desc is still NULL.

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index b3bcc206efe3e..4a4a279b405b1 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_conn_create() can have cleared rmb_desc */
>  	if (!smp_load_acquire(&conn->rmb_desc))
> @@ -357,12 +358,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 */

[Severity: High]
Does skipping the if (sndbuf_desc) block while still applying
conn->local_rx_ctrl.cons desynchronize conn->tx_curs_fin from
conn->local_rx_ctrl.cons and corrupt conn->sndbuf_space?

When a CDC message from the old peer of a reused DMB arrives before
smcd_buf_attach() with a non-zero consumer cursor, smc_cdc_msg_to_host()
advances conn->local_rx_ctrl.cons (and adds diff_cons to
conn->peer_rmbe_space, which smc_conn_save_peer_info() already set to
conn->peer_rmbe_size), while skipping the if (sndbuf_desc) block leaves
conn->tx_curs_fin at {0, 0}.

smcd_buf_attach() then initializes conn->sndbuf_space to the full
buf_desc->len without resetting conn->local_rx_ctrl.cons or matching
conn->tx_curs_fin to it.  Because diff_tx is computed against
conn->tx_curs_fin rather than cons_old:

net/smc/smc_cdc.c:smc_cdc_msg_recv_action() {
    ...
			diff_tx = smc_curs_diff(sndbuf_desc->len,
						&conn->tx_curs_fin,
						&conn->local_rx_ctrl.cons);
			/* increase local sndbuf space and fin_curs */
			smp_mb__before_atomic();
			atomic_add(diff_tx, &conn->sndbuf_space);
			/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */
			smp_mb__after_atomic();
			smc_curs_copy(&conn->tx_curs_fin,
				      &conn->local_rx_ctrl.cons, conn);
    ...
}

any later consumer update that advances past the pre-attach cursor still
credits the pre-attach delta on top of buf_desc->len, pushing
conn->sndbuf_space above sndbuf_desc->len and letting smc_tx_sendmsg()
overwrite unconsumed data or write past the end of the buffer.  Conversely,
when the new peer sends consumer updates at or below the stale
conn->local_rx_ctrl.cons, smc_curs_diff() returns 0 and never credits
conn->peer_rmbe_space or conn->sndbuf_space, stalling transmission.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-b4-disp-1c8d3a53-v6-0-36a38ae35021@proton.me?part=3