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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message misdescribes the fix.

Pre-existing issues:
- [High] Nothing drains local->conn_attend_q after the I/O thread leaves
  its main loop, so a poke that lands in that window leaks the…

--

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

This patch makes rxrpc_poke_conn() take its poke reference with
rxrpc_get_connection_maybe() in place of rxrpc_get_connection(). If the
refcount is already zero, the connection is no longer added to
local->conn_attend_q. This closes a use-after-free where the I/O thread
touched a freed connection that was still queued.

> 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: Low]
Is this paragraph accurate? rxrpc_queue_conn() checks only the active count
and takes no reference:

net/rxrpc/conn_object.c:rxrpc_queue_conn() {
	if (atomic_read(&conn->active) >= 0 &&
	    rxrpc_queue_work(&conn->processor))
		rxrpc_see_connection(conn, why);
}

The patch never reads conn->active. rxrpc_get_connection_maybe() checks
only the refcount:

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

That matches what rxrpc_poke_call() does with rxrpc_try_get_call(), not
what rxrpc_queue_conn() does.

Could the message say that the poke reference is now taken with
inc_not_zero, and drop the claim that this copies the conn->active check
in rxrpc_queue_conn()?

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

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but what
happens if a poke arrives after the I/O thread has left its main loop?

rxrpc_io_thread() breaks out of the loop once kthread_should_stop() is set
and conn_attend_q is empty. It then calls rxrpc_destroy_local():

net/rxrpc/io_thread.c:rxrpc_io_thread() {
    ...
		if (should_stop)
			break;
    ...
	rxrpc_destroy_local(local);
    ...
}

rxrpc_destroy_local() sets local->dead and cleans up the client conns, the
socket and rx_queue. It never touches local->conn_attend_q.

A client connection can still have conn->timer armed at that point.
rxrpc_disconnect_client_call() arms the timer for jiffies+2, then puts the
connection on the idle list with a reference held:

net/rxrpc/conn_client.c:rxrpc_disconnect_client_call() {
    ...
		set_bit(RXRPC_CONN_FINAL_ACK_0 + channel, &conn->flags);
		rxrpc_reduce_conn_timer(conn, final_ack_at);
    ...
}

rxrpc_clean_up_local_conns() cancels only local->client_conn_reap_timer
synchronously. It doesn't cancel each connection's own timer before it
drops the unbundle and idle references.

If conn->timer fires in that window, the refcount is still above zero:

rxrpc_connection_timer()
  rxrpc_poke_conn()
    rxrpc_get_connection_maybe()   <- succeeds
    list_add_tail(&conn->attend_link, &local->conn_attend_q)

The only thing that consumes conn_attend_q is the loop that has already
exited. Does this leak the connection and its poke reference, along with
its references on local, peer and bundle and its key?

If so, rxnet->nr_conns never reaches zero. On netns exit,
rxrpc_destroy_all_connections() would then wait forever here:

	wait_var_event(&rxnet->nr_conns, !atomic_read(&rxnet->nr_conns));

AF_RXRPC sockets can be created without privileges. An unprivileged user
could reach this window by closing the socket right after a client call
succeeds.

The old unconditional rxrpc_get_connection() had the same behaviour. The
new inc_not_zero guard doesn't help here because the refcount is nonzero.

Would rxrpc_poke_conn() need a local->dead check under local->lock to close
this? Alternatively, rxrpc_destroy_local() could drain conn_attend_q and
drop the poke references.

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