[PATCH net v2 0/2] net/smc: fix use-after-free in smc_rx_pipe_buf_release()

[PATCH net v2 0/2] net/smc: fix use-after-free in smc_rx_pipe_buf_release()

[Hidayath Khan]

smc_rx_pipe_buf_release() tests sk_state before taking the socket lock
and then dereferences conn->rmb_desc and conn->lgr. A concurrent close
runs smc_conn_free() in between, which releases those structures. On the
is_reg_err path smcr_buf_unuse() frees the descriptor outright, so this
is a use-after-free.

Patch 2/2 fixes this by taking the socket lock first and testing
conn->freed instead. smc_conn_free() sets that flag before releasing
anything, under the same lock, so the two paths exclude each other.

Patch 1/2 is a prerequisite. conn->freed shares a byte with killed and
out_of_sync as single-bit bitfields. out_of_sync is written from the
receive tasklet without the socket lock, so a concurrent store to freed
from process context can be lost in the read-modify-write. Patch 1/2
gives each flag its own byte so stores do not interfere.

Hidayath Khan (2):
  net/smc: stop killed, freed and out_of_sync sharing a byte
  net/smc: fix use-after-free in smc_rx_pipe_buf_release()

 net/smc/smc.h    |  6 +++---
 net/smc/smc_rx.c | 11 +++++------
 2 files changed, 8 insertions(+), 9 deletions(-)

---
v2:
- Patch 1/2 is new. It is a prerequisite for 2/2.
- Patch 2/2 replaces the conn->rmb_desc = NULL approach from v1 with a
  conn->freed check. The NULL store exposed smc_cdc_msg_recv_action()
  and the sock_diag dump to NULL dereferences on paths that do not take
  the socket lock.

-- 
2.52.0

[PATCH net v2 1/2] net/smc: stop killed, freed and out_of_sync sharing a byte

[Hidayath Khan]

The three connection state flags are single-bit bitfields, so they occupy
one byte of struct smc_connection and every store to one is a
read-modify-write of the other two:

    u8  killed : 1;
    u8  freed : 1;
    u8  out_of_sync : 1;

They are not written under a common lock. smc_cdc_msg_validate() sets
out_of_sync from the receive tasklet, while smc_conn_kill() sets killed
from process context under lock_sock(), and the receive path does not defer
to the backlog when the socket is owned -- smc_cdc_msg_recv() takes only
bh_lock_sock().

Give each flag its own byte so a store no longer touches its neighbours.
All readers test them as booleans and are unchanged. struct smc_connection
grows by two bytes.

Fixes: b286a0651e44 ("net/smc: handle incoming CDC validation message")
Cc: stable@vger.kernel.org
Reviewed-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
---
New in v2.

 net/smc/smc.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/smc/smc.h b/net/smc/smc.h
index 52145df83f6e..427b6d63b993 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -277,9 +277,9 @@ struct smc_connection {
 						 * 0 for SMC-R, 32 for SMC-D
 						 */
 	u64			peer_token;	/* SMC-D token of peer */
-	u8			killed : 1;	/* abnormal termination */
-	u8			freed : 1;	/* normal termination */
-	u8			out_of_sync : 1; /* out of sync with peer */
+	u8			killed;		/* abnormal termination */
+	u8			freed;		/* normal termination */
+	u8			out_of_sync;	/* out of sync with peer */
 };
 
 struct smc_sock {				/* smc sock container */
-- 
2.52.0

[PATCH net v2 2/2] net/smc: fix use-after-free in smc_rx_pipe_buf_release()

[Hidayath Khan]

smc_rx_splice() hands RMB pages to a pipe and takes a socket reference
per entry so the smc_sock stays alive until the reader finishes. The
connection does not: a concurrent close runs smc_conn_free(), which
releases the receive buffer back to the link group pool.

smc_rx_pipe_buf_release() tests sk_state before taking the socket lock.
The state can change between the test and the lock, and
smc_rx_update_cons() then dereferences conn->rmb_desc and walks
conn->lgr, which smc_conn_free() has already released. On the
is_reg_err path smcr_buf_unuse() frees the descriptor outright, so
this is a use-after-free.

Take the socket lock first and test conn->freed instead.
smc_conn_free() sets that flag before releasing anything, and every
caller holds the socket lock. The two paths exclude each other: either
the pipe release runs first with everything valid, or it sees the flag
and skips the update.

Fixes: 9014db202cb7 ("smc: add support for splice()")
Cc: stable@vger.kernel.org
Reviewed-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
---
v2: Replace conn->rmb_desc = NULL with a conn->freed check under the
    socket lock. The NULL store exposed two paths without a NULL check:
    smc_cdc_msg_recv_action() in softirq and the sock_diag dump without
    the socket lock.

 net/smc/smc_rx.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
index 5c9e4d8b57de..197fddc6271f 100644
--- a/net/smc/smc_rx.c
+++ b/net/smc/smc_rx.c
@@ -115,16 +115,15 @@ static void smc_rx_pipe_buf_release(struct pipe_inode_info *pipe,
 				    struct pipe_buffer *buf)
 {
 	struct smc_spd_priv *priv = (struct smc_spd_priv *)buf->private;
+	struct smc_connection *conn = &priv->smc->conn;
 	struct smc_sock *smc = priv->smc;
-	struct smc_connection *conn;
 	struct sock *sk = &smc->sk;
 
-	if (sk->sk_state == SMC_CLOSED ||
-	    sk->sk_state == SMC_PEERFINCLOSEWAIT ||
-	    sk->sk_state == SMC_APPFINCLOSEWAIT)
-		goto out;
-	conn = &smc->conn;
 	lock_sock(sk);
+	if (conn->freed) {
+		release_sock(sk);
+		goto out;
+	}
 	smc_rx_update_cons(smc, priv->len);
 	release_sock(sk);
 	if (atomic_sub_and_test(priv->len, &conn->splice_pending))
-- 
2.52.0

Re: [PATCH net v2 1/2] net/smc: stop killed, freed and out_of_sync sharing a byte

[sashiko-bot]

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

Pre-existing issues:
- [High] Concurrent Read-Modify-Write data race on `struct smc_link_group` bitfields (`sync_err`, `terminating`, `freeing`).
- [High] Read-modify-write (RMW) data race on `conn_state_flags` and `prod_flags` bitfields in `struct smc_connection`.
--

--- Patch [1]: [PATCH net v2 1/2] net/smc: stop killed, freed and out_of_sync sharing a byte ---
commit a4fa7f6a3c11e3e694e6c9235dfa2ed1bd680cdf
Author: Hidayath Khan <hidayath@linux.ibm.com>

net/smc: stop killed, freed and out_of_sync sharing a byte

The commit changes three connection state flags from single-bit bitfields to
separate u8 fields. This prevents read-modify-write data races between
process context and the receive tasklet.

> diff --git a/net/smc/smc.h b/net/smc/smc.h
> index 52145df83f6e7..427b6d63b9931 100644
> --- a/net/smc/smc.h
> +++ b/net/smc/smc.h
> @@ -277,9 +277,9 @@ struct smc_connection {
>  						 * 0 for SMC-R, 32 for SMC-D
>  						 */
>  	u64			peer_token;	/* SMC-D token of peer */
> -	u8			killed : 1;	/* abnormal termination */
> -	u8			freed : 1;	/* normal termination */
> -	u8			out_of_sync : 1; /* out of sync with peer */
> +	u8			killed;		/* abnormal termination */
> +	u8			freed;		/* normal termination */
> +	u8			out_of_sync;	/* out of sync with peer */
>  };

[Severity: High]
This is a pre-existing issue, but does a similar RMW data race exist for the
conn_state_flags and prod_flags bitfields embedded in struct smc_connection?

These bitfields are defined in net/smc/smc.h:

struct smc_cdc_conn_state_flags {
#if defined(__BIG_ENDIAN_BITFIELD)
	u8	peer_done_writing : 1;
	u8	peer_conn_closed : 1;
	u8	peer_conn_abort : 1;

Process context modifies these fields holding only lock_sock(), which doesn't
disable softirqs. For instance, smc_close_wr() in net/smc/smc_close.c sets
peer_done_writing = 1:

    conn->local_tx_ctrl.conn_state_flags.peer_done_writing = 1;

And smc_tx_consumer_update() in net/smc/smc_tx.c sets write_blocked = 0:

    conn->local_rx_ctrl.prod_flags.write_blocked = 0;

Meanwhile, tasklet context modifies the same bytes. smc_cdc_msg_validate()
in net/smc/smc_cdc.c sets peer_conn_abort = 1 under send_lock:

    conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;

And smc_cdc_msg_to_host() writes the entire prod_flags byte.

Since lock_sock() does not serialize with bh_lock_sock() or send_lock, could a
softirq interrupt the process context's RMW operation, resulting in dropped
connection state changes or stalled connections when the stale byte is written
back?

[Severity: High]
This is a pre-existing issue, but are there similar data races in
struct smc_link_group in net/smc/smc_core.h?

The sync_err, terminating, and freeing flags share a single byte:

struct smc_link_group {
    ...
    u8 sync_err : 1;
    u8 terminating : 1;
    u8 freeing : 1;
    ...
};

Could a read-modify-write data race occur here since they are modified from
different contexts without a common lock?

For example, smc_clc_wait_msg() in net/smc/smc_clc.c sets sync_err = 1
holding only the socket lock:

    smc->conn.lgr->sync_err = 1;

While __smc_lgr_terminate() in net/smc/smc_core.c sets terminating = 1
without holding any link group lock:

    lgr->terminating = 1;

And smc_lgr_terminate_sched() sets freeing = 1 under smc_lgr_list.lock:

    lgr->freeing = 1;

If an RMW operation is interrupted, could the terminating flag update be lost,
potentially leading to __smc_lgr_terminate() executing twice concurrently and
causing a double-free of the link group?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820074642.966856-1-hidayath@linux.ibm.com?part=1

Re: [PATCH net v2 2/2] net/smc: fix use-after-free in smc_rx_pipe_buf_release()

[sashiko-bot]

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

Pre-existing issues:
- [High] The Receive Message Buffer (RMB) pages are recycled to the link group pool without waiting for pending `splice()` readers to finish, leading to cross-connection data leaks.
- [High] Leaving `conn->rmb_desc` and `conn->sndbuf_desc` non-NULL after freeing causes exploitable Use-After-Free vulnerabilities in concurrent softirq and sock_diag paths.
--

--- Patch [2]: [PATCH net v2 2/2] net/smc: fix use-after-free in smc_rx_pipe_buf_release() ---
commit 05af7c6044c758c4b405394cc407668af77828fd
Author: Hidayath Khan <hidayath@linux.ibm.com>
Subject: net/smc: fix use-after-free in smc_rx_pipe_buf_release()

This patch fixes a use-after-free in smc_rx_pipe_buf_release() by checking 
if the connection has been freed while holding the socket lock. The approach 
replaces a previous version that set conn->rmb_desc to NULL.

Cc: stable@vger.kernel.org

> v2: Replace conn->rmb_desc = NULL with a conn->freed check under the
>     socket lock. The NULL store exposed two paths without a NULL check:
>     smc_cdc_msg_recv_action() in softirq and the sock_diag dump without
>     the socket lock.

[Severity: High]
This isn't a bug introduced by this patch, but does leaving conn->rmb_desc
and conn->sndbuf_desc non-NULL after freeing them cause a use-after-free in
these exposed paths?

In smcr_buf_unuse(), the buffer descriptor is unconditionally freed on the
is_reg_err path:

net/smc/smc_core.c:smcr_buf_unuse() {
    ...
    if (buf_desc->is_reg_err) {
        ...
        smc_buf_free(lgr, is_rmb, buf_desc);
    }
    ...
}

If conn->rmb_desc is not set to NULL, it seems concurrent paths could access
the freed memory. For instance, in __smc_diag_dump():

net/smc/smc_diag.c:__smc_diag_dump() {
    ...
    .sndbuf_size = conn->sndbuf_desc ? conn->sndbuf_desc->len : 0,
    .rmbe_size = conn->rmb_desc ? conn->rmb_desc->len : 0,
    ...
}

And similarly in smc_cdc_msg_recv_action():

net/smc/smc_cdc.c:smc_cdc_msg_recv_action() {
    ...
    diff_prod = smc_curs_diff(conn->rmb_desc->len, &prod_old,
                              &conn->local_rx_ctrl.prod);
    ...
}

Could this lead to memory corruption when these lockless or softirq paths
dereference the dangling pointer?

> diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
> index 5c9e4d8b57de4..197fddc6271f0 100644
> --- a/net/smc/smc_rx.c
> +++ b/net/smc/smc_rx.c
> @@ -115,16 +115,15 @@ static void smc_rx_pipe_buf_release(struct pipe_inode_info *pipe,
>  				    struct pipe_buffer *buf)
>  {
>  	struct smc_spd_priv *priv = (struct smc_spd_priv *)buf->private;
> +	struct smc_connection *conn = &priv->smc->conn;
>  	struct smc_sock *smc = priv->smc;
> -	struct smc_connection *conn;
>  	struct sock *sk = &smc->sk;
>  
> -	if (sk->sk_state == SMC_CLOSED ||
> -	    sk->sk_state == SMC_PEERFINCLOSEWAIT ||
> -	    sk->sk_state == SMC_APPFINCLOSEWAIT)
> -		goto out;
> -	conn = &smc->conn;
>  	lock_sock(sk);
> +	if (conn->freed) {
> +		release_sock(sk);
> +		goto out;
> +	}
>  	smc_rx_update_cons(smc, priv->len);
>  	release_sock(sk);
>  	if (atomic_sub_and_test(priv->len, &conn->splice_pending))
>  		smc_rx_wake_up(sk);

[Severity: High]
This is a pre-existing issue, but does the connection cleanup wait for
splice_pending to drop to zero before recycling the Receive Message Buffer
(RMB) pages?

In smc_rx_splice(), the buffer pages are pinned and passed to the pipe,
incrementing splice_pending:

net/smc/smc_rx.c:smc_rx_splice() {
    ...
    for (i = 0; i < nr_pages; i++) {
        get_page(pages[i]);
        sock_hold(&smc->sk);
    }
    ...
    if (bytes > 0)
        atomic_add(bytes, &smc->conn.splice_pending);
    ...
}

However, in smc_conn_free(), the buffer is released back to the link group
pool without waiting for the pipe readers to finish:

net/smc/smc_core.c:smc_conn_free() {
    ...
    smc_buf_unuse(conn, lgr); /* allow buffer reuse */
    smc_lgr_unregister_conn(conn);
    ...
}

If a socket is closed while a splice() operation is pending, could this lead
to a cross-connection data leak when another connection reuses the same
buffer and the pipe reader consumes the new payload?

>  out:
>  	kfree(priv);
>  	put_page(buf->page);
>  	sock_put(sk);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820074642.966856-1-hidayath@linux.ibm.com?part=2