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

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 <[email protected]>
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: [email protected]

> 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/[email protected]?part=2