Re: [PATCH net] rxrpc: Fix rxrpc_sock UAF caused by missing poke in rxrpc_abort_eproto()

[netdev-bot+sashiko@kernel.org]

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

Critical: 1 · High: 1 · Medium: 0 · 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).

- [Critical] The patch only makes the stranded-call UAF a race.
- [High] rxrpc: race on call->_state in rxrpc_abort_eproto() outside I/O
  thread

--

rxrpc: Fix rxrpc_sock UAF caused by missing poke in rxrpc_abort_eproto()

This commit adds a call to rxrpc_poke_call() in rxrpc_abort_eproto(). A
call aborted with EPROTO from recvmsg() or rxrpc_kernel_recv_data() then
gets queued for the I/O thread, which disconnects it from its connection
channel. Without the poke the call stays in conn->channels[] and its
call->socket pointer dangles after the socket is freed.

> diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
> index 865f05fe37ab9..31528b5e34cbe 100644
> --- a/net/rxrpc/ar-internal.h
> +++ b/net/rxrpc/ar-internal.h
> @@ -1455,6 +1455,7 @@ static inline int rxrpc_abort_eproto(struct rxrpc_call *call,
>  	struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
>  
>  	rxrpc_abort_call(call, sp->hdr.seq, abort_code, -EPROTO, why);
> +	rxrpc_poke_call(call, rxrpc_call_poke_abort);

[Severity: High]
Is it safe for rxrpc_abort_eproto() to complete the call itself here?
The alternative is to ask the I/O thread to do it.

Since commit a343b174b4bd ("rxrpc: Only set/transmit aborts in the I/O
thread") and commit 96b4059f43ce ("rxrpc: Remove call->state_lock"),
rxrpc_set_call_completion() takes no lock when it writes:

  call->_state
  call->completion
  call->abort_code
  call->error

This relies on only the I/O thread changing call state. Other contexts
are meant to use rxrpc_propose_abort(), which sets call->send_abort and
pokes the call.

rxrpc_abort_eproto() runs in process context and holds only
call->user_mutex, which the I/O thread never takes:

rxrpc_recvmsg() / rxrpc_kernel_recv_data()
  rxrpc_recvmsg_data()
    rxrpc_verify_data()
      call->security->verify_packet()
        rxrpc_abort_eproto()
          rxrpc_abort_call()
            rxrpc_set_call_completion()  /* _state = COMPLETE */

For a multi-packet service request, the I/O thread can be handling the
DATA packet with RXRPC_LAST_PACKET at the same moment:

rxrpc_input_queue_data()
  rxrpc_end_rx_phase()
    /* reads _state == RXRPC_CALL_SERVER_RECV_REQUEST */
    rxrpc_set_call_state(call, RXRPC_CALL_SERVER_ACK_REQUEST);

Suppose rxrpc_end_rx_phase() reads the old state before the abort and
writes after it. RXRPC_CALL_COMPLETE is then overwritten with
RXRPC_CALL_SERVER_ACK_REQUEST. call->completion and call->error still
say the call failed.

The poke added here then runs rxrpc_input_call_event(). call->send_abort
is 0 and __rxrpc_call_is_complete() is false, so the call never reaches
rxrpc_disconnect_call().

Userspace may have seen rxrpc_call_has_failed() before the overwrite and
released the call. In that case the call is gone from rx->sock_calls, so
rxrpc_release_calls_on_socket() won't abort it at close either.

Doesn't that leave the call stranded in conn->channels[] with a dangling
call->socket? That is the situation this patch is meant to fix. The
direct rxrpc_abort_call() predates this patch, but the poke relies on
the state staying RXRPC_CALL_COMPLETE.

There is a second problem even without the race. The poke wakes
rxrpc_input_call_event() with send_abort == 0 and often an empty
call->rx_queue. __rxrpc_call_is_complete() is only checked inside the
if (skb) block. So the timer expiry, delayed ACK/ping and retransmission
code all run on the aborted call before it reaches the out: label that
disconnects it. Is that intended?

Would it be better for rxrpc_abort_eproto() to use
rxrpc_propose_abort(call, abort_code, -EPROTO, why)? The state change
would then happen in the I/O thread, as for other aborts made outside
it. That changes when recvmsg() sees the call as complete, though. What
should the caller observe after the -EPROTO return?

[Severity: Critical]
Does this close the use-after-free in rxrpc_post_challenge(), or does it
turn it into a race?

rxrpc_poke_call() only adds the call to local->call_attend_q. The call
leaves conn->channels[] only later, when the I/O thread runs
rxrpc_input_call_event()->rxrpc_disconnect_call(). Nothing in the close
path waits for that to happen:

close()
  rxrpc_release_sock()
    rxrpc_release_calls_on_socket(rx)
      rxrpc_release_call()   /* sets RXRPC_CALL_RELEASED, call->socket kept */
    flush_workqueue(rxrpc_workqueue)   /* not the I/O thread */
    rxrpc_unuse_local()   /* I/O thread keeps running if another
                             client socket shares this rxrpc_local */
    sock_put(sk)   /* SOCK_RCU_FREE, freed after a grace period */

call->socket is set with rcu_assign_pointer() and never set back to
NULL, and the call holds no reference on the socket.

rxrpc_io_thread() also handles the whole rx batch before it splices and
processes local->call_attend_q. No RCU read section covers the batch, so
a grace period can end partway through it.

The peer can make the batch longer by flooding packets. Suppose a
CHALLENGE in that batch is handled after the socket has been freed but
before the poked call is disconnected:

rxrpc_io_thread()
  rxrpc_input_packet()
    rxrpc_input_packet_on_conn()
      rxrpc_input_conn_packet()
        rxrpc_post_challenge()

net/rxrpc/conn_event.c:rxrpc_post_challenge() {
    ...
	rcu_read_lock();

	for (int i = 0; i < ARRAY_SIZE(conn->channels); i++) {
		if (conn->channels[i].call) {
			call = conn->channels[i].call;
			rx = rcu_dereference(call->socket);
			if (!rx) {
    ...
			if (test_bit(RXRPC_SOCK_MANAGE_RESPONSE, &rx->flags))
				break;
    ...
	if (call)
		queued = rxrpc_notify_socket_oob(call, skb);
    ...
}

Wouldn't this still read rx->flags from the freed rxrpc_sock? Couldn't
it then reach rxrpc_notify_socket_oob(), which takes rx->recvmsg_lock and
may call function pointers from the freed object? That is the same
use-after-free the commit message describes.

The same window looks like it exists in the generic close path too,
where rxrpc_propose_abort() pokes calls that are not yet complete. That
predates this patch, but it is the same rxrpc_post_challenge() access
this patch is meant to fix.

Would it make sense to remove the stale pointer as well? One option is
RCU_INIT_POINTER(call->socket, NULL) in rxrpc_release_call(), since
rxrpc_post_challenge(), rxrpc_notify_socket_oob() and
rxrpc_notify_socket() already handle a NULL rx. Another is to have
rxrpc_post_challenge() skip calls that are RXRPC_CALL_RELEASED or
complete. A third is to make release wait for the I/O thread to
disconnect the call.

The poke itself still looks needed for the call leak and the
rxrpc_destroy_client_conn_ids() BUG() and netns teardown hang described
in the commit message.

>  	return -EPROTO;
>  }
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106011550.59902-1-juanagusnicco%40gmail.com