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

[PATCH net] 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 for
each entry, so the smc_sock survives until the reader is done.  The
connection does not: a close in between runs smc_conn_free(), which
releases the link group and returns the receive buffer to the link group's
pool.

smc_rx_pipe_buf_release() tries to detect that by testing sk_state, but it
does so before taking the socket lock, and then dereferences the connection
anyway:

        if (sk->sk_state == SMC_CLOSED || ...)
                goto out;
        conn = &smc->conn;
        lock_sock(sk);
        smc_rx_update_cons(smc, priv->len);

smc_rx_update_cons() reads conn->rmb_desc->len twice and then calls
smc_tx_consumer_update(), which walks conn->lgr and conn->lnk.  The state
can change between the test and the lock, and on the is_reg_err path
smcr_buf_unuse() does not recycle the descriptor but frees it outright, so
this is a use-after-free rather than a stale read.

sk_state is also the wrong thing to test.  Take the socket lock first so
the test and the cursor update cannot be separated, and test the receive
buffer itself, which is what the code goes on to dereference.

For that test to mean anything, smc_buf_unuse() has to stop leaving a
pointer to a descriptor it has just released; clear conn->rmb_desc there.
Nothing in smc_conn_free() reads it afterwards, and smc_ism_unset_conn()
already returns early on a NULL rmb_desc, so an SMC-D teardown that reaches
it twice becomes a no-op instead of indexing smcd->conn[] with a stale
sba_idx.

Fixes: 9014db202cb7 ("smc: add support for splice()")
Cc: stable@vger.kernel.org
Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
---
 net/smc/smc_core.c |  1 +
 net/smc/smc_rx.c   | 10 ++++++----
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index c0027d2fe4e8..def65ebc0b53 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1248,6 +1248,7 @@ static void smc_buf_unuse(struct smc_connection *conn,
 			WRITE_ONCE(conn->rmb_desc->used, 0);
 		}
 		SMC_STAT_RMB_SIZE(smc, is_smcd, true, false, bufsize);
+		conn->rmb_desc = NULL;
 	}
 }
 
diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
index c1d9b923938d..c17d4757ec84 100644
--- a/net/smc/smc_rx.c
+++ b/net/smc/smc_rx.c
@@ -116,15 +116,17 @@ static void smc_rx_pipe_buf_release(struct pipe_inode_info *pipe,
 {
 	struct smc_spd_priv *priv = (struct smc_spd_priv *)buf->private;
 	struct smc_sock *smc = priv->smc;
-	struct smc_connection *conn;
+	struct smc_connection *conn = &smc->conn;
 	struct sock *sk = &smc->sk;
 
+	lock_sock(sk);
 	if (sk->sk_state == SMC_CLOSED ||
 	    sk->sk_state == SMC_PEERFINCLOSEWAIT ||
-	    sk->sk_state == SMC_APPFINCLOSEWAIT)
+	    sk->sk_state == SMC_APPFINCLOSEWAIT ||
+	    !conn->rmb_desc) {
+		release_sock(sk);
 		goto out;
-	conn = &smc->conn;
-	lock_sock(sk);
+	}
 	smc_rx_update_cons(smc, priv->len);
 	release_sock(sk);
 	if (atomic_sub_and_test(priv->len, &conn->splice_pending))

base-commit: 1c2c67f1a9009f643d1233fa6bb8f888deb797ba
-- 
2.52.0

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

[Simon Horman]

On Mon, Aug 10, 2026 at 08:40:41AM +0200, Hidayath Khan wrote:
> smc_rx_splice() hands RMB pages to a pipe and takes a socket reference for
> each entry, so the smc_sock survives until the reader is done.  The
> connection does not: a close in between runs smc_conn_free(), which
> releases the link group and returns the receive buffer to the link group's
> pool.
> 
> smc_rx_pipe_buf_release() tries to detect that by testing sk_state, but it
> does so before taking the socket lock, and then dereferences the connection
> anyway:
> 
>         if (sk->sk_state == SMC_CLOSED || ...)
>                 goto out;
>         conn = &smc->conn;
>         lock_sock(sk);
>         smc_rx_update_cons(smc, priv->len);
> 
> smc_rx_update_cons() reads conn->rmb_desc->len twice and then calls
> smc_tx_consumer_update(), which walks conn->lgr and conn->lnk.  The state
> can change between the test and the lock, and on the is_reg_err path
> smcr_buf_unuse() does not recycle the descriptor but frees it outright, so
> this is a use-after-free rather than a stale read.
> 
> sk_state is also the wrong thing to test.  Take the socket lock first so
> the test and the cursor update cannot be separated, and test the receive
> buffer itself, which is what the code goes on to dereference.
> 
> For that test to mean anything, smc_buf_unuse() has to stop leaving a
> pointer to a descriptor it has just released; clear conn->rmb_desc there.
> Nothing in smc_conn_free() reads it afterwards, and smc_ism_unset_conn()
> already returns early on a NULL rmb_desc, so an SMC-D teardown that reaches
> it twice becomes a no-op instead of indexing smcd->conn[] with a stale
> sba_idx.
> 
> Fixes: 9014db202cb7 ("smc: add support for splice()")
> Cc: stable@vger.kernel.org
> Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
> Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
> ---
>  net/smc/smc_core.c |  1 +
>  net/smc/smc_rx.c   | 10 ++++++----
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index c0027d2fe4e8..def65ebc0b53 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1248,6 +1248,7 @@ static void smc_buf_unuse(struct smc_connection *conn,
>  			WRITE_ONCE(conn->rmb_desc->used, 0);
>  		}
>  		SMC_STAT_RMB_SIZE(smc, is_smcd, true, false, bufsize);
> +		conn->rmb_desc = NULL;
>  	}
>  }

Hi,

The AI-generated review at [1] flags the following.
Could you take a look?

[1] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260810064041.720441-1-hidayath%40linux.ibm.com

AI-generated review text:

  Before this change conn->rmb_desc stayed non-NULL once it was set, so
  readers that never take the socket lock could only ever see a stale (still
  mapped) descriptor.  Can this new store turn those into NULL
  dereferences?

  Two paths look exposed.  First, the SMC-R CDC receive path.  In
  smc_conn_free() the buffer is released before the conn leaves 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);
	}
	...
  }

  So there is a window where the conn is still in lgr->conns_all with
  rmb_desc == NULL.  smc_cdc_rx_handler() finds it there, and
  smc_cdc_msg_recv() only takes bh_lock_sock():

  net/smc/smc_cdc.c:smc_cdc_msg_recv() {
	sock_hold(&smc->sk);
	bh_lock_sock(&smc->sk);
	smc_cdc_msg_recv_action(smc, cdc);
	bh_unlock_sock(&smc->sk);
	...
  }

  bh_lock_sock() takes sk->sk_lock.slock and does not exclude a
  process-context lock_sock() owner running smc_release() ->
  __smc_release() -> smc_conn_free(), which is why
  smc_cdc_msg_recv_action() itself has to check sock_owned_by_user().  That
  same function then does:

  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);
	...
  }

  and on urgent data smc_cdc_handle_urg_data_arrival() does:

  net/smc/smc_cdc.c:smc_cdc_handle_urg_data_arrival() {
	base = (char *)conn->rmb_desc->cpu_addr + conn->rx_off;
	if (conn->urg_curs.count)
		conn->urg_rx_byte = *(base + conn->urg_curs.count - 1);
	else
		conn->urg_rx_byte = *(base + conn->rmb_desc->len - 1);
	...
  }

  Neither has a NULL check, and this runs in softirq/tasklet context.  Would
  it be safer to clear rmb_desc after smc_lgr_unregister_conn(), once the
  conn can no longer be found by the CDC handler?

  Second, the sock_diag dump.  smc_diag_dump_proto() walks the sockets under
  read_lock(&prot->h.smc_hash->lock) only, never lock_sock(), and
  __smc_diag_dump() tests the pointer in the condition:

  net/smc/smc_diag.c:__smc_diag_dump() {
	...
	if (smc_conn_lgr_valid(&smc->conn) && smc->conn.lgr->is_smcd &&
	    (req->diag_ext & (1 << (SMC_DIAG_DMBINFO - 1))) &&
	    !list_empty(&smc->conn.lgr->list) && smc->conn.rmb_desc) {
	...
		dinfo.token = conn->rmb_desc->token;
	...
  }

  Can a close on another CPU clear the pointer between that test and the
  conn->rmb_desc->token dereference?  The same file also does the plain
  double load

	.rmbe_size = conn->rmb_desc ? conn->rmb_desc->len : 0,

  with no READ_ONCE, which the compiler may reload after the test.

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

[Hidayath Khan]

Thanks for the review.

Both findings are correct, and both come
from the conn->rmb_desc = NULL store.  v2 drops that store instead of
moving it, and tests conn->freed under the socket lock.

On 11/08/26 5:49 pm, Simon Horman wrote:
> On Mon, Aug 10, 2026 at 08:40:41AM +0200, Hidayath Khan wrote:
>> smc_rx_splice() hands RMB pages to a pipe and takes a socket reference for
>> each entry, so the smc_sock survives until the reader is done.  The
>> connection does not: a close in between runs smc_conn_free(), which
>> releases the link group and returns the receive buffer to the link group's
>> pool.
>>
>> smc_rx_pipe_buf_release() tries to detect that by testing sk_state, but it
>> does so before taking the socket lock, and then dereferences the connection
>> anyway:
>>
>>          if (sk->sk_state == SMC_CLOSED || ...)
>>                  goto out;
>>          conn = &smc->conn;
>>          lock_sock(sk);
>>          smc_rx_update_cons(smc, priv->len);
>>
>> smc_rx_update_cons() reads conn->rmb_desc->len twice and then calls
>> smc_tx_consumer_update(), which walks conn->lgr and conn->lnk.  The state
>> can change between the test and the lock, and on the is_reg_err path
>> smcr_buf_unuse() does not recycle the descriptor but frees it outright, so
>> this is a use-after-free rather than a stale read.
>>
>> sk_state is also the wrong thing to test.  Take the socket lock first so
>> the test and the cursor update cannot be separated, and test the receive
>> buffer itself, which is what the code goes on to dereference.
>>
>> For that test to mean anything, smc_buf_unuse() has to stop leaving a
>> pointer to a descriptor it has just released; clear conn->rmb_desc there.
>> Nothing in smc_conn_free() reads it afterwards, and smc_ism_unset_conn()
>> already returns early on a NULL rmb_desc, so an SMC-D teardown that reaches
>> it twice becomes a no-op instead of indexing smcd->conn[] with a stale
>> sba_idx.
>>
>> Fixes: 9014db202cb7 ("smc: add support for splice()")
>> Cc: stable@vger.kernel.org
>> Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
>> Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
>> ---
>>   net/smc/smc_core.c |  1 +
>>   net/smc/smc_rx.c   | 10 ++++++----
>>   2 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
>> index c0027d2fe4e8..def65ebc0b53 100644
>> --- a/net/smc/smc_core.c
>> +++ b/net/smc/smc_core.c
>> @@ -1248,6 +1248,7 @@ static void smc_buf_unuse(struct smc_connection *conn,
>>   			WRITE_ONCE(conn->rmb_desc->used, 0);
>>   		}
>>   		SMC_STAT_RMB_SIZE(smc, is_smcd, true, false, bufsize);
>> +		conn->rmb_desc = NULL;
>>   	}
>>   }
> Hi,
>
> The AI-generated review at [1] flags the following.
> Could you take a look?
>
> [1] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260810064041.720441-1-hidayath%40linux.ibm.com
>
> AI-generated review text:
>
>    Before this change conn->rmb_desc stayed non-NULL once it was set, so
>    readers that never take the socket lock could only ever see a stale (still
>    mapped) descriptor.  Can this new store turn those into NULL
>    dereferences?
>
>    Two paths look exposed.  First, the SMC-R CDC receive path.  In
>    smc_conn_free() the buffer is released before the conn leaves 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);
> 	}
> 	...
>    }
>
>    So there is a window where the conn is still in lgr->conns_all with
>    rmb_desc == NULL.  smc_cdc_rx_handler() finds it there, and
>    smc_cdc_msg_recv() only takes bh_lock_sock():
>
>    net/smc/smc_cdc.c:smc_cdc_msg_recv() {
> 	sock_hold(&smc->sk);
> 	bh_lock_sock(&smc->sk);
> 	smc_cdc_msg_recv_action(smc, cdc);
> 	bh_unlock_sock(&smc->sk);
> 	...
>    }
>
>    bh_lock_sock() takes sk->sk_lock.slock and does not exclude a
>    process-context lock_sock() owner running smc_release() ->
>    __smc_release() -> smc_conn_free(), which is why
>    smc_cdc_msg_recv_action() itself has to check sock_owned_by_user().  That
>    same function then does:
>
>    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);
> 	...
>    }
>
>    and on urgent data smc_cdc_handle_urg_data_arrival() does:
>
>    net/smc/smc_cdc.c:smc_cdc_handle_urg_data_arrival() {
> 	base = (char *)conn->rmb_desc->cpu_addr + conn->rx_off;
> 	if (conn->urg_curs.count)
> 		conn->urg_rx_byte = *(base + conn->urg_curs.count - 1);
> 	else
> 		conn->urg_rx_byte = *(base + conn->rmb_desc->len - 1);
> 	...
>    }
>
>    Neither has a NULL check, and this runs in softirq/tasklet context.  Would
>    it be safer to clear rmb_desc after smc_lgr_unregister_conn(), once the
>    conn can no longer be found by the CDC handler?
It would help, but it does not close the window: smc_cdc_rx_handler() drops
conns_lock before calling smc_cdc_msg_recv(), so an in-flight handler still
reaches the dereference.

conn->freed avoids it instead.  smc_conn_free() sets it before releasing
anything and with the socket lock held, every caller holds that lock, and
smc_rx_pipe_buf_release() takes the same lock.  So the two exclude each
other rather than racing.
>
>    Second, the sock_diag dump.  smc_diag_dump_proto() walks the sockets under
>    read_lock(&prot->h.smc_hash->lock) only, never lock_sock(), and
>    __smc_diag_dump() tests the pointer in the condition:
>
>    net/smc/smc_diag.c:__smc_diag_dump() {
> 	...
> 	if (smc_conn_lgr_valid(&smc->conn) && smc->conn.lgr->is_smcd &&
> 	    (req->diag_ext & (1 << (SMC_DIAG_DMBINFO - 1))) &&
> 	    !list_empty(&smc->conn.lgr->list) && smc->conn.rmb_desc) {
> 	...
> 		dinfo.token = conn->rmb_desc->token;
> 	...
>    }
>
>    Can a close on another CPU clear the pointer between that test and the
>    conn->rmb_desc->token dereference?
Yes, and it is not confined to rmb_desc -- conn->sndbuf_desc is already
cleared on teardown and the diag dump tests it the same way, so this is in
the tree already.  READ_ONCE() would stop the compiler reloading after
the test, but it would not stop the descriptor being freed under the walker,
so it needs its own patch.
> 	.rmbe_size = conn->rmb_desc ? conn->rmb_desc->len : 0,
>
>    with no READ_ONCE, which the compiler may reload after the test.
One dependency.  conn->freed shares a byte with killed and out_of_sync, and
out_of_sync is written from the receive tasklet without the socket lock, so
a read-modify-write there can drop a concurrent freed.  v2 therefore comes
as a two patch series, with "net/smc: stop killed, freed and out_of_sync
sharing a byte" as 1/2.
>