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

[Hidayath Khan <hidayath@linux.ibm.com>]

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.
>