[PATCH net v2 0/2] net/smc: close the SMC-D teardown window around the ghost send buffer

[PATCH net v2 0/2] net/smc: close the SMC-D teardown window around the ghost send buffer

[Bryam Vargas via B4 Relay]

Both patches only matter on the SMC-D DMB-nocopy path, where the ghost send
buffer exists, and the only in-tree provider of support_mmapped_rdmb is
dibs_loopback. CONFIG_DIBS_LO is default n and its help calls it a testing aid,
so on a stock config neither bug is reachable.

v1 moved smcd_buf_detach() after the drain. Dust Li replied that it does not
fully eliminate the race and asked whether RCU is the better shape. He is right
about the first part; I built both and measured them.

An SMC-D loopback KASAN rig, one module binary, teardown form selected at
runtime. "path" counts connections reaching either teardown site with the link
group already unlinked, "armable" how many of those still had both gates in
smcd_handle_irq() open when the drain returned, "re-armed" the device arming
the tasklet again afterwards:

  form                                   path  armable  re-armed
  upstream                                169       73        29
  v1 (drain, then detach)                 172       78        33
  unregister first, then drain             31        0         0
  v1 + RCU                                 24        9         3

Two caveats on that table. The last two arms ran far shorter than the first two,
so compare the armable/re-armed ratios rather than the absolute path counts. And
the third row also forced tasklet_kill() in the !soft path, which 1/2 does not;
that was inert here because smc_lgr_terminate_work() passes soft=true, so the
same call ran either way.

The reorder alone leaves the window open, which is what Dust saw. RCU doesn't
close it either: smcd_buf_detach() both frees the descriptor and clears the
field, and RCU defers only the free, so a re-armed tasklet still runs and still
finds conn->sndbuf_desc NULL. The gate has to be shut before the drain, and
that is 1/2. RCU on the descriptor would still be a reasonable thing to want
for the free itself; it just doesn't substitute for 1/2, so I didn't fold it
in. Your call if you want it anyway.

Caveat on 1/2: the two changes the table covers -- unconditional
smc_ism_unset_conn(), and drain before detach -- were measured together, not
separately. It also clears conn->sndbuf_desc before freeing it, so a reader
that samples the pointer cannot get one that is already freed; that part is
by inspection.

2/2 is a second dereference the same teardown reaches, found while running the
above. smc_close_stream_wait() calls smc_tx_prepared_sends() from inside
sk_wait_event(), which evaluates its condition once with the socket lock
released, and a terminating link group clears conn->sndbuf_desc right there.
SIOCOUTQ reads the same field by hand, and smc_close_cancel_work() drops the
socket lock across two cancel_*_sync() calls, so 2/2 bounds that too. Eight
faults across three boots, the earliest 89 seconds in:

  RIP: smc_close_stream_wait+0x66d [smc]
   smc_close_active -> __smc_release -> smc_release -> __x64_sys_close

The faulting address is NULL plus offsetof(struct smc_buf_desc, len), nothing
there is attacker-chosen, and the value read never reaches userspace, so there
is no memory-safety primitive and no leak oracle -- it is an oops. The task dies
inside close() holding the socket lock, so I would expect the socket to leak with
it, but I didn't isolate that from the rig's own effects and I'm not claiming it.

Reaching either bug needs a link-group teardown while a socket is parked in that
wait. smc_lgr_cleanup_early() off a failed first-contact handshake gets there, as
does smc_clc_wait_msg() on a peer DECLINE with FIRST_CONTACT -- both by
inspection. The rig instead drove smc_lgr_terminate_sched() from a debug module
parameter, so only the initiation is synthetic; the unlink, the deferred worker,
smc_conn_free() and smcd_handle_irq() are the unmodified path. Logs and the rig
on request.

I haven't touched tasklet_unlock_wait() in the !soft path of smc_conn_kill().
It waits out TASKLET_STATE_RUN without clearing TASKLET_STATE_SCHED, but I have
no measurement showing that reachable here, so it stays as it is.

v2:
- 1/2: unregister the connection unconditionally rather than only while the
  link group is still on its device list. That guard, not the ordering, is why
  the tasklet can be re-armed after tasklet_kill().
- 2/2: new.
- v1: https://lore.kernel.org/all/20260726-b4-disp-a135b4e5-v1-1-705aaec79986@proton.me/

Bryam Vargas (2):
  net/smc: unregister the connection before draining the rx tasklet
  net/smc: do not dereference an unset send buffer on the SMC-D teardown path

 net/smc/af_smc.c   |  3 ++-
 net/smc/smc_core.c | 13 +++++++------
 net/smc/smc_tx.h   |  6 +++++-
 3 files changed, 14 insertions(+), 8 deletions(-)

base-commit: 3dbb44d88b1e
---
Bryam Vargas (2):
      net/smc: unregister the connection before draining the rx tasklet
      net/smc: do not dereference an unset send buffer on the SMC-D teardown path

 net/smc/af_smc.c   |  3 ++-
 net/smc/smc_core.c | 13 +++++++------
 net/smc/smc_tx.h   |  6 +++++-
 3 files changed, 14 insertions(+), 8 deletions(-)
---
base-commit: 3dbb44d88b1e94dd31fe43588af7437b34b44d56
change-id: 20260808-b4-disp-22f119e6-b3009a5443c1

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

[PATCH net v2 0/2] net/smc: close the SMC-D teardown window around the ghost send buffer

[Bryam Vargas]

Both patches only matter on the SMC-D DMB-nocopy path, where the ghost send
buffer exists, and the only in-tree provider of support_mmapped_rdmb is
dibs_loopback. CONFIG_DIBS_LO is default n and its help calls it a testing aid,
so on a stock config neither bug is reachable.

v1 moved smcd_buf_detach() after the drain. Dust Li replied that it does not
fully eliminate the race and asked whether RCU is the better shape. He is right
about the first part; I built both and measured them.

An SMC-D loopback KASAN rig, one module binary, teardown form selected at
runtime. "path" counts connections reaching either teardown site with the link
group already unlinked, "armable" how many of those still had both gates in
smcd_handle_irq() open when the drain returned, "re-armed" the device arming
the tasklet again afterwards:

  form                                   path  armable  re-armed
  upstream                                169       73        29
  v1 (drain, then detach)                 172       78        33
  unregister first, then drain             31        0         0
  v1 + RCU                                 24        9         3

Two caveats on that table. The last two arms ran far shorter than the first two,
so compare the armable/re-armed ratios rather than the absolute path counts. And
the third row also forced tasklet_kill() in the !soft path, which 1/2 does not;
that was inert here because smc_lgr_terminate_work() passes soft=true, so the
same call ran either way.

The reorder alone leaves the window open, which is what Dust saw. RCU doesn't
close it either: smcd_buf_detach() both frees the descriptor and clears the
field, and RCU defers only the free, so a re-armed tasklet still runs and still
finds conn->sndbuf_desc NULL. The gate has to be shut before the drain, and
that is 1/2. RCU on the descriptor would still be a reasonable thing to want
for the free itself; it just doesn't substitute for 1/2, so I didn't fold it
in. Your call if you want it anyway.

Caveat on 1/2: the two changes the table covers -- unconditional
smc_ism_unset_conn(), and drain before detach -- were measured together, not
separately. It also clears conn->sndbuf_desc before freeing it, so a reader
that samples the pointer cannot get one that is already freed; that part is
by inspection.

2/2 is a second dereference the same teardown reaches, found while running the
above. smc_close_stream_wait() calls smc_tx_prepared_sends() from inside
sk_wait_event(), which evaluates its condition once with the socket lock
released, and a terminating link group clears conn->sndbuf_desc right there.
SIOCOUTQ reads the same field by hand, and smc_close_cancel_work() drops the
socket lock across two cancel_*_sync() calls, so 2/2 bounds that too. Eight
faults across three boots, the earliest 89 seconds in:

  RIP: smc_close_stream_wait+0x66d [smc]
   smc_close_active -> __smc_release -> smc_release -> __x64_sys_close

The faulting address is NULL plus offsetof(struct smc_buf_desc, len), nothing
there is attacker-chosen, and the value read never reaches userspace, so there
is no memory-safety primitive and no leak oracle -- it is an oops. The task dies
inside close() holding the socket lock, so I would expect the socket to leak with
it, but I didn't isolate that from the rig's own effects and I'm not claiming it.

Reaching either bug needs a link-group teardown while a socket is parked in that
wait. smc_lgr_cleanup_early() off a failed first-contact handshake gets there, as
does smc_clc_wait_msg() on a peer DECLINE with FIRST_CONTACT -- both by
inspection. The rig instead drove smc_lgr_terminate_sched() from a debug module
parameter, so only the initiation is synthetic; the unlink, the deferred worker,
smc_conn_free() and smcd_handle_irq() are the unmodified path. Logs and the rig
on request.

I haven't touched tasklet_unlock_wait() in the !soft path of smc_conn_kill().
It waits out TASKLET_STATE_RUN without clearing TASKLET_STATE_SCHED, but I have
no measurement showing that reachable here, so it stays as it is.

v2:
- 1/2: unregister the connection unconditionally rather than only while the
  link group is still on its device list. That guard, not the ordering, is why
  the tasklet can be re-armed after tasklet_kill().
- 2/2: new.
- v1: https://lore.kernel.org/all/20260726-b4-disp-a135b4e5-v1-1-705aaec79986@proton.me/

Bryam Vargas (2):
  net/smc: unregister the connection before draining the rx tasklet
  net/smc: do not dereference an unset send buffer on the SMC-D teardown path

 net/smc/af_smc.c   |  3 ++-
 net/smc/smc_core.c | 13 +++++++------
 net/smc/smc_tx.h   |  6 +++++-
 3 files changed, 14 insertions(+), 8 deletions(-)

base-commit: 3dbb44d88b1e
---
Bryam Vargas (2):
      net/smc: unregister the connection before draining the rx tasklet
      net/smc: do not dereference an unset send buffer on the SMC-D teardown path

 net/smc/af_smc.c   |  3 ++-
 net/smc/smc_core.c | 13 +++++++------
 net/smc/smc_tx.h   |  6 +++++-
 3 files changed, 14 insertions(+), 8 deletions(-)
---
base-commit: 3dbb44d88b1e94dd31fe43588af7437b34b44d56
change-id: 20260808-b4-disp-22f119e6-b3009a5443c1

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

[PATCH net v2 1/2] net/smc: unregister the connection before draining the rx tasklet

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

smc_conn_free() calls smc_ism_unset_conn() only while the link group is
still on its device list, and never sets conn->killed.
smc_lgr_terminate_sched() unlinks the group immediately and defers killing
its connections to a work item, so a connection freed in that window keeps
its smcd->conn[] slot with both gates in smcd_handle_irq() open, and the
device can re-arm the receive tasklet after tasklet_kill() has returned. On
the DMB-nocopy path the ghost send buffer is freed right after that drain,
so the re-armed tasklet dereferences it.

Unregister unconditionally and drain before the detach at both teardown
sites, mirroring rmb_desc, which smc_buf_unuse() releases after the drain.
Clear conn->sndbuf_desc before freeing it as well, so a reader that samples
the pointer cannot get one that is already freed.

Fixes: ae2be35cbed2 ("net/smc: {at|de}tach sndbuf to peer DMB if supported")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/smc/smc_core.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index b4208cb186c5..181647982490 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1209,14 +1209,16 @@ static void smcd_buf_detach(struct smc_connection *conn)
 {
 	struct smcd_dev *smcd = conn->lgr->smcd;
 	u64 peer_token = conn->peer_token;
+	struct smc_buf_desc *buf_desc;
 
 	if (!conn->sndbuf_desc)
 		return;
 
 	smc_ism_detach_dmb(smcd, peer_token);
 
-	kfree(conn->sndbuf_desc);
+	buf_desc = conn->sndbuf_desc;
 	conn->sndbuf_desc = NULL;
+	kfree(buf_desc);
 }
 
 static void smc_buf_unuse(struct smc_connection *conn,
@@ -1268,11 +1270,10 @@ void smc_conn_free(struct smc_connection *conn)
 		goto lgr_put;
 
 	if (lgr->is_smcd) {
-		if (!list_empty(&lgr->list))
-			smc_ism_unset_conn(conn);
+		smc_ism_unset_conn(conn);
+		tasklet_kill(&conn->rx_tsklet);
 		if (smc_ism_support_dmb_nocopy(lgr->smcd))
 			smcd_buf_detach(conn);
-		tasklet_kill(&conn->rx_tsklet);
 	} else {
 		smc_cdc_wait_pend_tx_wr(conn);
 		if (current_work() != &conn->abort_work)
@@ -1525,12 +1526,12 @@ static void smc_conn_kill(struct smc_connection *conn, bool soft)
 	smc_sk_wake_ups(smc);
 	if (conn->lgr->is_smcd) {
 		smc_ism_unset_conn(conn);
-		if (smc_ism_support_dmb_nocopy(conn->lgr->smcd))
-			smcd_buf_detach(conn);
 		if (soft)
 			tasklet_kill(&conn->rx_tsklet);
 		else
 			tasklet_unlock_wait(&conn->rx_tsklet);
+		if (smc_ism_support_dmb_nocopy(conn->lgr->smcd))
+			smcd_buf_detach(conn);
 	} else {
 		smc_cdc_wait_pend_tx_wr(conn);
 	}

-- 
2.55.0

[PATCH net v2 1/2] net/smc: unregister the connection before draining the rx tasklet

[Bryam Vargas]

smc_conn_free() calls smc_ism_unset_conn() only while the link group is
still on its device list, and never sets conn->killed.
smc_lgr_terminate_sched() unlinks the group immediately and defers killing
its connections to a work item, so a connection freed in that window keeps
its smcd->conn[] slot with both gates in smcd_handle_irq() open, and the
device can re-arm the receive tasklet after tasklet_kill() has returned. On
the DMB-nocopy path the ghost send buffer is freed right after that drain,
so the re-armed tasklet dereferences it.

Unregister unconditionally and drain before the detach at both teardown
sites, mirroring rmb_desc, which smc_buf_unuse() releases after the drain.
Clear conn->sndbuf_desc before freeing it as well, so a reader that samples
the pointer cannot get one that is already freed.

Fixes: ae2be35cbed2 ("net/smc: {at|de}tach sndbuf to peer DMB if supported")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/smc/smc_core.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index b4208cb186c5..181647982490 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1209,14 +1209,16 @@ static void smcd_buf_detach(struct smc_connection *conn)
 {
 	struct smcd_dev *smcd = conn->lgr->smcd;
 	u64 peer_token = conn->peer_token;
+	struct smc_buf_desc *buf_desc;
 
 	if (!conn->sndbuf_desc)
 		return;
 
 	smc_ism_detach_dmb(smcd, peer_token);
 
-	kfree(conn->sndbuf_desc);
+	buf_desc = conn->sndbuf_desc;
 	conn->sndbuf_desc = NULL;
+	kfree(buf_desc);
 }
 
 static void smc_buf_unuse(struct smc_connection *conn,
@@ -1268,11 +1270,10 @@ void smc_conn_free(struct smc_connection *conn)
 		goto lgr_put;
 
 	if (lgr->is_smcd) {
-		if (!list_empty(&lgr->list))
-			smc_ism_unset_conn(conn);
+		smc_ism_unset_conn(conn);
+		tasklet_kill(&conn->rx_tsklet);
 		if (smc_ism_support_dmb_nocopy(lgr->smcd))
 			smcd_buf_detach(conn);
-		tasklet_kill(&conn->rx_tsklet);
 	} else {
 		smc_cdc_wait_pend_tx_wr(conn);
 		if (current_work() != &conn->abort_work)
@@ -1525,12 +1526,12 @@ static void smc_conn_kill(struct smc_connection *conn, bool soft)
 	smc_sk_wake_ups(smc);
 	if (conn->lgr->is_smcd) {
 		smc_ism_unset_conn(conn);
-		if (smc_ism_support_dmb_nocopy(conn->lgr->smcd))
-			smcd_buf_detach(conn);
 		if (soft)
 			tasklet_kill(&conn->rx_tsklet);
 		else
 			tasklet_unlock_wait(&conn->rx_tsklet);
+		if (smc_ism_support_dmb_nocopy(conn->lgr->smcd))
+			smcd_buf_detach(conn);
 	} else {
 		smc_cdc_wait_pend_tx_wr(conn);
 	}

-- 
2.55.0

[PATCH net v2 2/2] net/smc: do not dereference an unset send buffer on the SMC-D teardown path

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

smc_close_stream_wait() calls smc_tx_prepared_sends() from inside its
sk_wait_event() condition, and sk_wait_event() evaluates that condition
once with the socket lock released. smcd_buf_detach() clears
conn->sndbuf_desc from smc_conn_kill() under lock_sock(), so a link group
terminating while a socket waits there leaves the helper dereferencing
NULL, faulting out of close(). SIOCOUTQ reads the field by hand, and
smc_close_cancel_work() drops the lock across two cancel_*_sync() calls.

Sample the pointer once in the helper, report nothing prepared while it is
unset, and bound the ioctl the same way. The receive tasklet dereferences
the field directly in smc_cdc_msg_recv_action(), not through this helper;
1/2 is what keeps it from running that late.

Fixes: ae2be35cbed2 ("net/smc: {at|de}tach sndbuf to peer DMB if supported")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/smc/af_smc.c | 3 ++-
 net/smc/smc_tx.h | 6 +++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 00403175b740..cff910cedbfc 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -3233,7 +3233,8 @@ int smc_ioctl(struct socket *sock, unsigned int cmd,
 			return -EINVAL;
 		}
 		if (smc->sk.sk_state == SMC_INIT ||
-		    smc->sk.sk_state == SMC_CLOSED)
+		    smc->sk.sk_state == SMC_CLOSED ||
+		    !READ_ONCE(smc->conn.sndbuf_desc))
 			answ = 0;
 		else
 			answ = smc->conn.sndbuf_desc->len -
diff --git a/net/smc/smc_tx.h b/net/smc/smc_tx.h
index a59f370b8b43..610a945aefd6 100644
--- a/net/smc/smc_tx.h
+++ b/net/smc/smc_tx.h
@@ -20,11 +20,15 @@
 
 static inline int smc_tx_prepared_sends(struct smc_connection *conn)
 {
+	struct smc_buf_desc *sndbuf_desc = READ_ONCE(conn->sndbuf_desc);
 	union smc_host_cursor sent, prep;
 
+	if (!sndbuf_desc)
+		return 0;
+
 	smc_curs_copy(&sent, &conn->tx_curs_sent, conn);
 	smc_curs_copy(&prep, &conn->tx_curs_prep, conn);
-	return smc_curs_diff(conn->sndbuf_desc->len, &sent, &prep);
+	return smc_curs_diff(sndbuf_desc->len, &sent, &prep);
 }
 
 void smc_tx_pending(struct smc_connection *conn);

-- 
2.55.0

[PATCH net v2 2/2] net/smc: do not dereference an unset send buffer on the SMC-D teardown path

[Bryam Vargas]

smc_close_stream_wait() calls smc_tx_prepared_sends() from inside its
sk_wait_event() condition, and sk_wait_event() evaluates that condition
once with the socket lock released. smcd_buf_detach() clears
conn->sndbuf_desc from smc_conn_kill() under lock_sock(), so a link group
terminating while a socket waits there leaves the helper dereferencing
NULL, faulting out of close(). SIOCOUTQ reads the field by hand, and
smc_close_cancel_work() drops the lock across two cancel_*_sync() calls.

Sample the pointer once in the helper, report nothing prepared while it is
unset, and bound the ioctl the same way. The receive tasklet dereferences
the field directly in smc_cdc_msg_recv_action(), not through this helper;
1/2 is what keeps it from running that late.

Fixes: ae2be35cbed2 ("net/smc: {at|de}tach sndbuf to peer DMB if supported")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 net/smc/af_smc.c | 3 ++-
 net/smc/smc_tx.h | 6 +++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 00403175b740..cff910cedbfc 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -3233,7 +3233,8 @@ int smc_ioctl(struct socket *sock, unsigned int cmd,
 			return -EINVAL;
 		}
 		if (smc->sk.sk_state == SMC_INIT ||
-		    smc->sk.sk_state == SMC_CLOSED)
+		    smc->sk.sk_state == SMC_CLOSED ||
+		    !READ_ONCE(smc->conn.sndbuf_desc))
 			answ = 0;
 		else
 			answ = smc->conn.sndbuf_desc->len -
diff --git a/net/smc/smc_tx.h b/net/smc/smc_tx.h
index a59f370b8b43..610a945aefd6 100644
--- a/net/smc/smc_tx.h
+++ b/net/smc/smc_tx.h
@@ -20,11 +20,15 @@
 
 static inline int smc_tx_prepared_sends(struct smc_connection *conn)
 {
+	struct smc_buf_desc *sndbuf_desc = READ_ONCE(conn->sndbuf_desc);
 	union smc_host_cursor sent, prep;
 
+	if (!sndbuf_desc)
+		return 0;
+
 	smc_curs_copy(&sent, &conn->tx_curs_sent, conn);
 	smc_curs_copy(&prep, &conn->tx_curs_prep, conn);
-	return smc_curs_diff(conn->sndbuf_desc->len, &sent, &prep);
+	return smc_curs_diff(sndbuf_desc->len, &sent, &prep);
 }
 
 void smc_tx_pending(struct smc_connection *conn);

-- 
2.55.0

Re: [PATCH net v2 1/2] net/smc: unregister the connection before draining the rx tasklet

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch attempts to prevent concurrent readers from accessing a freed `sndbuf_desc` by setting the pointer to `NULL` before `kfree()`. However, this is insufficient to protect lockless readers like the Netlink diagnostic dump (`__smc_diag_dump`), leaving a Time-of-Check to Time-of-Use (TOCTOU) NULL pointer dereference or Use-After-Free vulnerability.
--

--- Patch [1]: [PATCH net v2 1/2] net/smc: unregister the connection before draining the rx tasklet ---
commit 776266687d64ab630969455645f01d4387577460
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/smc: unregister the connection before draining the rx tasklet

This commit modifies the teardown path to unregister the connection and
drain the receive tasklet before detaching the buffers. It also clears
conn->sndbuf_desc before freeing it to prevent readers from getting a
freed pointer.

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index b4208cb186c5e..181647982490b 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1209,14 +1209,16 @@ static void smcd_buf_detach(struct smc_connection *conn)
>  {
>  	struct smcd_dev *smcd = conn->lgr->smcd;
>  	u64 peer_token = conn->peer_token;
> +	struct smc_buf_desc *buf_desc;
>  
>  	if (!conn->sndbuf_desc)
>  		return;
>  
>  	smc_ism_detach_dmb(smcd, peer_token);
>  
> -	kfree(conn->sndbuf_desc);
> +	buf_desc = conn->sndbuf_desc;
>  	conn->sndbuf_desc = NULL;
> +	kfree(buf_desc);

[Severity: High]
This is a pre-existing issue, but is setting the pointer to NULL before
kfree() sufficient to protect concurrent lockless readers?

Looking at __smc_diag_dump() in net/smc/smc_diag.c, which executes without
holding the socket lock, it evaluates the pointer like this:

    .sndbuf_size = conn->sndbuf_desc ?
                   conn->sndbuf_desc->len : 0,

Since it doesn't use READ_ONCE(), couldn't the compiler generate two fetches?

If the first fetch passes the NULL check but the second fetch gets NULL after
the pointer is cleared here, would it lead to a NULL pointer dereference?

Alternatively, if it only fetches once, couldn't it dereference memory that
was just synchronously freed via kfree(), causing a Use-After-Free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260808-b4-disp-22f119e6-v2-0-61647601a6f3@proton.me?part=1

Re: [PATCH net v2 2/2] net/smc: do not dereference an unset send buffer on the SMC-D teardown path

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch prevents a NULL pointer dereference but leaves a Use-After-Free (UAF) vulnerability in `smc_tx_prepared_sends()` by locklessly dereferencing `sndbuf_desc->len` without object lifetime protection.
--

--- Patch [2]: [PATCH net v2 2/2] net/smc: do not dereference an unset send buffer on the SMC-D teardown path ---
commit 08550d7abeeecb4d3f4b5d472d83dee49a54e0ba
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/smc: do not dereference an unset send buffer on the SMC-D teardown path

This patch addresses a NULL pointer dereference during SMC-D teardown by
using READ_ONCE to cache the send buffer descriptor and checking it for
NULL. It ensures that lockless evaluations inside sk_wait_event() do not
fault when the descriptor is cleared concurrently.

> diff --git a/net/smc/smc_tx.h b/net/smc/smc_tx.h
> index a59f370b8b432..610a945aefd69 100644
> --- a/net/smc/smc_tx.h
> +++ b/net/smc/smc_tx.h
> @@ -20,11 +20,15 @@
>  
>  static inline int smc_tx_prepared_sends(struct smc_connection *conn)
>  {
> +	struct smc_buf_desc *sndbuf_desc = READ_ONCE(conn->sndbuf_desc);
>  	union smc_host_cursor sent, prep;
>  
> +	if (!sndbuf_desc)
> +		return 0;
> +
>  	smc_curs_copy(&sent, &conn->tx_curs_sent, conn);
>  	smc_curs_copy(&prep, &conn->tx_curs_prep, conn);
> -	return smc_curs_diff(conn->sndbuf_desc->len, &sent, &prep);

[Severity: High]
This is a pre-existing issue, but does this lockless dereference leave a
use-after-free vulnerability?

If CPU1 evaluates this condition via sk_wait_event() in
smc_close_stream_wait(), it explicitly drops the socket lock. CPU1 might
cache the pointer here:

CPU1:
smc_tx_prepared_sends()
    struct smc_buf_desc *sndbuf_desc = READ_ONCE(conn->sndbuf_desc);
    if (!sndbuf_desc)
        // bypasses this check

If CPU1 is preempted and CPU2 concurrently terminates the connection, it
acquires the socket lock and could call smcd_buf_detach() to free the buffer:

CPU2:
net/smc/smc_core.c:smcd_buf_detach()
    buf_desc = conn->sndbuf_desc;
    conn->sndbuf_desc = NULL;
    kfree(buf_desc);

Wouldn't CPU1 then dereference the freed descriptor without any object
lifetime protection when it eventually resumes, calls smc_curs_diff(), and
accesses sndbuf_desc->len?

> +	return smc_curs_diff(sndbuf_desc->len, &sent, &prep);
>  }
>  
>  void smc_tx_pending(struct smc_connection *conn);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260808-b4-disp-22f119e6-v2-0-61647601a6f3@proton.me?part=2