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

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: [email protected]
> Reviewed-by: Sidraya Jayagond <[email protected]>
> Signed-off-by: Hidayath Khan <[email protected]>
> ---
>  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.