[PATCH net v2] net/smc: order the CDC receive path against buffer publication

[PATCH net v2] net/smc: order the CDC receive path against buffer publication

[Bryam Vargas]

The SMC CDC receive handlers dereference conn->rmb_desc, and on the
SMC-D DMB-nocopy path conn->sndbuf_desc, but both are published after
the connection is already reachable to a peer: rmb_desc after
smc_conn_create() registers it in the link group's token tree,
sndbuf_desc after __smc_buf_create() arms the ISM tasklet via
smc_ism_set_conn().  A CDC in that window reaches the handlers with the
buffer unset.  The store is plain, so a handler can load it as NULL, or
on a weakly ordered CPU see it non-NULL while its buffer is still
uninitialised -- a host crash or a stale-buffer read.

Publish both buffers with smp_store_release() and consume them with
smp_load_acquire(), bailing while unset as the handlers already do for
a killed or out-of-sync connection.  Conforming peers are unaffected.

Closes: https://sashiko.dev/#/patchset/20260711-b4-disp-c36a9798-v1-1-340b0c6053fb@proton.me?part=1
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
v2: order both CDC-reachable buffers with smp_store_release()/smp_load_acquire()
    instead of the plain NULL guard v1 used.  The rmb_desc ordering is what the
    Sashiko review of v1 asked for (the Closes: link above); the sndbuf_desc case
    (SMC-D DMB-nocopy, smc_cdc_msg_recv_action()) is the same-window sibling found
    by inspection -- smcd_buf_attach() sets the ghost sndbuf_desc after
    smc_ism_set_conn() already armed the tasklet.  Release/acquire closes the NULL
    deref on all arches and the stale-buffer read on weakly ordered ones.
    v1: https://lore.kernel.org/all/20260711-b4-disp-c36a9798-v1-1-340b0c6053fb@proton.me/

Happy to split this: the sndbuf_desc hunks only apply where the DMB-nocopy path
exists and can carry their own Fixes: tag for a cleaner stable backport, while the
rmb_desc ordering predates the git history here.  No Fixes: added -- please add
whichever you prefer.

Both orderings are modelled with LKMM message-passing litmus tests (herd7): plain
accesses allow the "pointer published, buffer stale" outcome and flag a data race;
smp_store_release()/smp_load_acquire() forbid it.  The patched build was exercised
over an SMC-D loopback under KASAN with no regression; the rmb_desc NULL-deref arm
is reproduced with an in-kernel KASAN model faulting at the ->cpu_addr / ->len
offsets.  af_smc runs over an RDMA fabric or an ISM device, so the weak-memory arm
is model-level; litmus tests and reproducer available on request.
---
 net/smc/smc_cdc.c  | 29 +++++++++++++++++++++++++----
 net/smc/smc_core.c | 16 ++++++++++++++--
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index 32d6d03df321..2cd0ee7b51c2 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -332,6 +332,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;
+	struct smc_buf_desc *sndbuf_desc;
 	int diff_cons, diff_prod, diff_tx;
 
 	smc_curs_copy(&prod_old, &conn->local_rx_ctrl.prod, conn);
@@ -353,12 +354,20 @@ 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.
 		 */
+		/* Pair with smp_store_release() in smcd_buf_attach(): the ghost
+		 * sndbuf_desc is attached after the connection is reachable to
+		 * the ISM device, so acquire it and skip the update while it is
+		 * unset -- avoids a NULL deref and a load of an uninitialised
+		 * buffer.
+		 */
+		sndbuf_desc = smp_load_acquire(&conn->sndbuf_desc);
 		if (conn->lgr->is_smcd &&
-		    smc_ism_support_dmb_nocopy(conn->lgr->smcd)) {
+		    smc_ism_support_dmb_nocopy(conn->lgr->smcd) &&
+		    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 */
@@ -443,13 +452,21 @@ static void smcd_cdc_rx_tsklet(struct tasklet_struct *t)
 {
 	struct smc_connection *conn = from_tasklet(conn, t, rx_tsklet);
 	struct smcd_cdc_msg *data_cdc;
+	struct smc_buf_desc *rmb_desc;
 	struct smcd_cdc_msg cdc;
 	struct smc_sock *smc;
 
 	if (!conn || conn->killed)
 		return;
+	/* Pair with smp_store_release() in __smc_buf_create(): the connection
+	 * is published before its RMB is allocated, so bail while rmb_desc is
+	 * unset to avoid a NULL deref and a load of an uninitialised buffer.
+	 */
+	rmb_desc = smp_load_acquire(&conn->rmb_desc);
+	if (!rmb_desc)
+		return;
 
-	data_cdc = (struct smcd_cdc_msg *)conn->rmb_desc->cpu_addr;
+	data_cdc = (struct smcd_cdc_msg *)rmb_desc->cpu_addr;
 	smcd_curs_copy(&cdc.prod, &data_cdc->prod, conn);
 	smcd_curs_copy(&cdc.cons, &data_cdc->cons, conn);
 	smc = container_of(conn, struct smc_sock, conn);
@@ -483,7 +500,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) {
+	/* Pair with smp_store_release() in __smc_buf_create(): bail while the
+	 * RMB is unset (smc_cdc_msg_recv_action() dereferences it) to avoid a
+	 * NULL deref and a stale-buffer read in the connection setup window.
+	 */
+	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 cf6b620fef05..d94b728c0d68 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -2499,7 +2499,13 @@ static int __smc_buf_create(struct smc_sock *smc, bool is_smcd, bool is_rmb)
 	}
 
 	if (is_rmb) {
-		conn->rmb_desc = buf_desc;
+		/* Publish with release semantics: the connection is already in
+		 * the link group's token tree, so a concurrent CDC receive
+		 * handler must observe a fully initialised buffer once it sees
+		 * a non-NULL rmb_desc.  Pairs with the smp_load_acquire() in
+		 * the CDC receive path.
+		 */
+		smp_store_release(&conn->rmb_desc, buf_desc);
 		conn->rmbe_size_comp = bufsize_comp;
 		smc->sk.sk_rcvbuf = bufsize * 2;
 		atomic_set(&conn->bytes_to_rcv, 0);
@@ -2599,7 +2605,13 @@ 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;
+	/* Publish with release semantics: the connection is already reachable
+	 * to the ISM device (smc_ism_set_conn() ran in __smc_buf_create()), so
+	 * the CDC receive tasklet must observe a fully initialised ghost buffer
+	 * once it sees a non-NULL sndbuf_desc.  Pairs with smp_load_acquire()
+	 * in smc_cdc_msg_recv_action().
+	 */
+	smp_store_release(&conn->sndbuf_desc, buf_desc);
 	conn->sndbuf_desc->used = 1;
 	atomic_set(&conn->sndbuf_space, conn->sndbuf_desc->len);
 	return 0;

---
base-commit: 3f1f755366687d051174739fb99f7d560202f60b
change-id: 20260714-b4-disp-835288a6-1f72ff8d7a71

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

[PATCH net v2] net/smc: order the CDC receive path against buffer publication

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

The SMC CDC receive handlers dereference conn->rmb_desc, and on the
SMC-D DMB-nocopy path conn->sndbuf_desc, but both are published after
the connection is already reachable to a peer: rmb_desc after
smc_conn_create() registers it in the link group's token tree,
sndbuf_desc after __smc_buf_create() arms the ISM tasklet via
smc_ism_set_conn().  A CDC in that window reaches the handlers with the
buffer unset.  The store is plain, so a handler can load it as NULL, or
on a weakly ordered CPU see it non-NULL while its buffer is still
uninitialised -- a host crash or a stale-buffer read.

Publish both buffers with smp_store_release() and consume them with
smp_load_acquire(), bailing while unset as the handlers already do for
a killed or out-of-sync connection.  Conforming peers are unaffected.

Closes: https://sashiko.dev/#/patchset/20260711-b4-disp-c36a9798-v1-1-340b0c6053fb@proton.me?part=1
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
v2: order both CDC-reachable buffers with smp_store_release()/smp_load_acquire()
    instead of the plain NULL guard v1 used.  The rmb_desc ordering is what the
    Sashiko review of v1 asked for (the Closes: link above); the sndbuf_desc case
    (SMC-D DMB-nocopy, smc_cdc_msg_recv_action()) is the same-window sibling found
    by inspection -- smcd_buf_attach() sets the ghost sndbuf_desc after
    smc_ism_set_conn() already armed the tasklet.  Release/acquire closes the NULL
    deref on all arches and the stale-buffer read on weakly ordered ones.
    v1: https://lore.kernel.org/all/20260711-b4-disp-c36a9798-v1-1-340b0c6053fb@proton.me/

Happy to split this: the sndbuf_desc hunks only apply where the DMB-nocopy path
exists and can carry their own Fixes: tag for a cleaner stable backport, while the
rmb_desc ordering predates the git history here.  No Fixes: added -- please add
whichever you prefer.

Both orderings are modelled with LKMM message-passing litmus tests (herd7): plain
accesses allow the "pointer published, buffer stale" outcome and flag a data race;
smp_store_release()/smp_load_acquire() forbid it.  The patched build was exercised
over an SMC-D loopback under KASAN with no regression; the rmb_desc NULL-deref arm
is reproduced with an in-kernel KASAN model faulting at the ->cpu_addr / ->len
offsets.  af_smc runs over an RDMA fabric or an ISM device, so the weak-memory arm
is model-level; litmus tests and reproducer available on request.
---
 net/smc/smc_cdc.c  | 29 +++++++++++++++++++++++++----
 net/smc/smc_core.c | 16 ++++++++++++++--
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index 32d6d03df321..2cd0ee7b51c2 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -332,6 +332,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;
+	struct smc_buf_desc *sndbuf_desc;
 	int diff_cons, diff_prod, diff_tx;
 
 	smc_curs_copy(&prod_old, &conn->local_rx_ctrl.prod, conn);
@@ -353,12 +354,20 @@ 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.
 		 */
+		/* Pair with smp_store_release() in smcd_buf_attach(): the ghost
+		 * sndbuf_desc is attached after the connection is reachable to
+		 * the ISM device, so acquire it and skip the update while it is
+		 * unset -- avoids a NULL deref and a load of an uninitialised
+		 * buffer.
+		 */
+		sndbuf_desc = smp_load_acquire(&conn->sndbuf_desc);
 		if (conn->lgr->is_smcd &&
-		    smc_ism_support_dmb_nocopy(conn->lgr->smcd)) {
+		    smc_ism_support_dmb_nocopy(conn->lgr->smcd) &&
+		    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 */
@@ -443,13 +452,21 @@ static void smcd_cdc_rx_tsklet(struct tasklet_struct *t)
 {
 	struct smc_connection *conn = from_tasklet(conn, t, rx_tsklet);
 	struct smcd_cdc_msg *data_cdc;
+	struct smc_buf_desc *rmb_desc;
 	struct smcd_cdc_msg cdc;
 	struct smc_sock *smc;
 
 	if (!conn || conn->killed)
 		return;
+	/* Pair with smp_store_release() in __smc_buf_create(): the connection
+	 * is published before its RMB is allocated, so bail while rmb_desc is
+	 * unset to avoid a NULL deref and a load of an uninitialised buffer.
+	 */
+	rmb_desc = smp_load_acquire(&conn->rmb_desc);
+	if (!rmb_desc)
+		return;
 
-	data_cdc = (struct smcd_cdc_msg *)conn->rmb_desc->cpu_addr;
+	data_cdc = (struct smcd_cdc_msg *)rmb_desc->cpu_addr;
 	smcd_curs_copy(&cdc.prod, &data_cdc->prod, conn);
 	smcd_curs_copy(&cdc.cons, &data_cdc->cons, conn);
 	smc = container_of(conn, struct smc_sock, conn);
@@ -483,7 +500,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) {
+	/* Pair with smp_store_release() in __smc_buf_create(): bail while the
+	 * RMB is unset (smc_cdc_msg_recv_action() dereferences it) to avoid a
+	 * NULL deref and a stale-buffer read in the connection setup window.
+	 */
+	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 cf6b620fef05..d94b728c0d68 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -2499,7 +2499,13 @@ static int __smc_buf_create(struct smc_sock *smc, bool is_smcd, bool is_rmb)
 	}
 
 	if (is_rmb) {
-		conn->rmb_desc = buf_desc;
+		/* Publish with release semantics: the connection is already in
+		 * the link group's token tree, so a concurrent CDC receive
+		 * handler must observe a fully initialised buffer once it sees
+		 * a non-NULL rmb_desc.  Pairs with the smp_load_acquire() in
+		 * the CDC receive path.
+		 */
+		smp_store_release(&conn->rmb_desc, buf_desc);
 		conn->rmbe_size_comp = bufsize_comp;
 		smc->sk.sk_rcvbuf = bufsize * 2;
 		atomic_set(&conn->bytes_to_rcv, 0);
@@ -2599,7 +2605,13 @@ 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;
+	/* Publish with release semantics: the connection is already reachable
+	 * to the ISM device (smc_ism_set_conn() ran in __smc_buf_create()), so
+	 * the CDC receive tasklet must observe a fully initialised ghost buffer
+	 * once it sees a non-NULL sndbuf_desc.  Pairs with smp_load_acquire()
+	 * in smc_cdc_msg_recv_action().
+	 */
+	smp_store_release(&conn->sndbuf_desc, buf_desc);
 	conn->sndbuf_desc->used = 1;
 	atomic_set(&conn->sndbuf_space, conn->sndbuf_desc->len);
 	return 0;

---
base-commit: 3f1f755366687d051174739fb99f7d560202f60b
change-id: 20260714-b4-disp-835288a6-1f72ff8d7a71

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

Re: [PATCH net v2] net/smc: order the CDC receive path against buffer publication

[sashiko-bot]

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

Pre-existing issues:
- [High] Premature publication of buffer descriptors (`rmb_desc` and `sndbuf_desc`) via `smp_store_release()` before the completion of their associated connection state initialization leaves a race window where concurrent state updates from CDC handlers can be silently overwritten.
- [High] The patch attempts to fix a NULL pointer dereference of `conn->sndbuf_desc` in `smc_cdc_msg_recv_action()` for the SMC-D DMB-nocopy path but misses a secondary dereference path in the same function, leaving an exploitable kernel panic.
--

--- Patch [1]: [PATCH net v2] net/smc: order the CDC receive path against buffer publication ---
commit 9b6f28a01319410a969a198788b79eb249671b6c
Author: Bryam Vargas <hexlabsecurity@proton.me>
Subject: net/smc: order the CDC receive path against buffer publication

This commit introduces acquire/release pairs for publishing rmb_desc and
sndbuf_desc to prevent NULL dereferences and stale reads. By ensuring
CDC handlers only see fully initialized buffers, it prevents crashes 
during the connection setup window.

Closes: https://sashiko.dev/#/patchset/20260711-b4-disp-c36a9798-v1-1-340b0c6053fb@proton.me?part=1

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index 32d6d03df3214..2cd0ee7b51c22 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c
> @@ -353,12 +354,20 @@ 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.
>  		 */
> +		/* Pair with smp_store_release() in smcd_buf_attach(): the ghost
> +		 * sndbuf_desc is attached after the connection is reachable to
> +		 * the ISM device, so acquire it and skip the update while it is
> +		 * unset -- avoids a NULL deref and a load of an uninitialised
> +		 * buffer.
> +		 */
> +		sndbuf_desc = smp_load_acquire(&conn->sndbuf_desc);
>  		if (conn->lgr->is_smcd &&
> -		    smc_ism_support_dmb_nocopy(conn->lgr->smcd)) {
> +		    smc_ism_support_dmb_nocopy(conn->lgr->smcd) &&
> +		    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]
This isn't a bug introduced by this patch, but does this function have a
secondary NULL pointer dereference path?

While this patch adds a guard for sndbuf_desc here, further down in this
same function (smc_cdc_msg_recv_action), if an early CDC message arrives
with an advanced cons cursor, diff_cons will be > 0.

This triggers the following code block:

    if ((diff_cons && smc_tx_prepared_sends(conn)) || ...

Looking at smc_tx_prepared_sends() in net/smc/smc_tx.h, it unconditionally
dereferences sndbuf_desc:

static inline int smc_tx_prepared_sends(struct smc_connection *conn)
{
    ...
    return smc_curs_diff(conn->sndbuf_desc->len, &sent, &prep);
}

Could this cause a panic if conn->sndbuf_desc is still NULL during the race
window?

[ ... ]

> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index cf6b620fef05f..d94b728c0d68c 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -2499,7 +2499,13 @@ static int __smc_buf_create(struct smc_sock *smc, bool is_smcd, bool is_rmb)
>  	}
>  
>  	if (is_rmb) {
> -		conn->rmb_desc = buf_desc;
> +		/* Publish with release semantics: the connection is already in
> +		 * the link group's token tree, so a concurrent CDC receive
> +		 * handler must observe a fully initialised buffer once it sees
> +		 * a non-NULL rmb_desc.  Pairs with the smp_load_acquire() in
> +		 * the CDC receive path.
> +		 */
> +		smp_store_release(&conn->rmb_desc, buf_desc);
>  		conn->rmbe_size_comp = bufsize_comp;
>  		smc->sk.sk_rcvbuf = bufsize * 2;
>  		atomic_set(&conn->bytes_to_rcv, 0);

[Severity: High]
This is a pre-existing issue, but could this ordering cause concurrent
handler updates to be silently clobbered?

By publishing rmb_desc via smp_store_release() before bytes_to_rcv is
initialized, could a CDC receive handler observe the non-NULL descriptor,
process a message, and update bytes_to_rcv via atomic_add(), only for this
thread to immediately overwrite it with atomic_set(&conn->bytes_to_rcv, 0)?

[ ... ]

> @@ -2599,7 +2605,13 @@ 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;
> +	/* Publish with release semantics: the connection is already reachable
> +	 * to the ISM device (smc_ism_set_conn() ran in __smc_buf_create()), so
> +	 * the CDC receive tasklet must observe a fully initialised ghost buffer
> +	 * once it sees a non-NULL sndbuf_desc.  Pairs with smp_load_acquire()
> +	 * in smc_cdc_msg_recv_action().
> +	 */
> +	smp_store_release(&conn->sndbuf_desc, buf_desc);
>  	conn->sndbuf_desc->used = 1;
>  	atomic_set(&conn->sndbuf_space, conn->sndbuf_desc->len);
>  	return 0;

[Severity: High]
This is also a pre-existing issue, but does this have the same clobbering
risk as the rmb_desc publication above?

If a CDC handler observes the published sndbuf_desc and updates sndbuf_space
via atomic_add(), wouldn't the atomic_set(&conn->sndbuf_space, ...) here
unconditionally overwrite the handler's update, leading to corrupted space
accounting?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-b4-disp-835288a6-v2-1-581555ef2145@proton.me?part=1