Re: [PATCH net v12 15/15] rxrpc: fix use-after-free in rxrpc_poke_conn()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message describes the new guard wrongly.

--

rxrpc: fix use-after-free in rxrpc_poke_conn()

Before this patch, rxrpc_poke_conn() took a reference on the connection
without checking whether the refcount had already reached zero. That
could saturate the refcount and leave a freed connection linked on
local->conn_attend_q. The patch replaces rxrpc_get_connection() with
rxrpc_get_connection_maybe(), so the poke is dropped if no reference
can be taken.

> Guard rxrpc_poke_conn() with the same liveness/refcount check the sibling
> rxrpc_queue_conn() uses before taking the poke reference, so a connection
> past its last-active point is not poked/requeued after teardown began.

[Severity: Medium]
Is this description accurate? The patch and rxrpc_queue_conn() seem to
check different things.

rxrpc_queue_conn() in net/rxrpc/conn_object.c gates only on the active
count and takes no reference:

    if (atomic_read(&conn->active) >= 0 &&
        rxrpc_queue_work(&conn->processor))
            rxrpc_see_connection(conn, why);

The patched rxrpc_poke_conn() calls rxrpc_get_connection_maybe(). That
function only does an increment-if-nonzero on the refcount and never reads
conn->active:

    if (__refcount_inc_not_zero(&conn->ref, &r))

The two predicates are not equivalent. rxrpc_service_connection_reaper()
moves active from 0 to -1 on its own, before it drops its reference:

    active = 0;
    if (!atomic_try_cmpxchg(&conn->active, &active, -1))
            continue;

So a connection with active == -1 and ref > 0 can still be poked after this
patch. Also, a client connection can reach ref == 0 without active ever
being set to -1.

The refcount check looks like the right fix for the use-after-free. Only
the wording seems off.

Could the commit message say instead that the poke reference is now taken
with refcount_inc_not_zero(), and that the poke is dropped once the
refcount has reached zero?

As written, "the same liveness/refcount check the sibling
rxrpc_queue_conn() uses" and "past its last-active point" suggest that an
active-count gate was added. An active-count check alone would not close
this use-after-free, and a stable backporter might copy that check instead.

> diff --git a/net/rxrpc/conn_object.c b/net/rxrpc/conn_object.c
> index 0ece717db0f85..1be50e0c9cee5 100644
> --- a/net/rxrpc/conn_object.c
> +++ b/net/rxrpc/conn_object.c
> @@ -34,7 +34,10 @@ void rxrpc_poke_conn(struct rxrpc_connection *conn, enum rxrpc_conn_trace why)
>  	spin_lock_irq(&local->lock);
>  	busy = !list_empty(&conn->attend_link);
>  	if (!busy) {
> -		rxrpc_get_connection(conn, why);
> +		if (!rxrpc_get_connection_maybe(conn, why)) {
> +			spin_unlock_irq(&local->lock);
> +			return;
> +		}
>  		list_add_tail(&conn->attend_link, &local->conn_attend_q);
>  	}
>  	spin_unlock_irq(&local->lock);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006133011.531806-1-dhowells%40redhat.com