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

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

[Juan Agustin Niccolini]

Since commit 03fc55adf876 ("rxrpc: Only disconnect calls in the I/O
thread"), only the I/O thread disconnects a call from its connection
channel, and hence, code that completes a call elsewhere must poke it
with rxrpc_poke_call(), which queues the call for the I/O thread.

rxrpc_abort_eproto(), which aborts a call when a DATA packet fails
security verification in recvmsg() or rxrpc_kernel_recv_data(), marks
the call as complete but doesn't poke it.  The call is then never
disconnected, not even when its socket is closed, because
rxrpc_propose_abort() skips calls that are already complete.  It stays
in conn->channels[], and its call->socket pointer, which holds no
reference on the socket, dangles once the socket is closed and freed.

If the peer then sends an rxgk CHALLENGE on that connection,
rxrpc_post_challenge() finds the call and reads the freed socket's
flags.  If the socket had enabled RXRPC_MANAGE_RESPONSE, the call is
then passed to rxrpc_notify_socket_oob(), which takes the freed
socket's lock:

  BUG: KASAN: slab-use-after-free in rxrpc_input_conn_packet+0x3b2/0x820
  Read of size 8 at addr ff1100011b343328 by task krxrpcio/21029/1030
  ...
  BUG: KASAN: slab-use-after-free in _raw_spin_lock_irq+0x72/0xe0
  Write of size 4 at addr ff1100011b343308 by task krxrpcio/21029/1030
  Call Trace:
   _raw_spin_lock_irq+0x72/0xe0
   rxrpc_notify_socket_oob+0x54/0x3f0
   rxrpc_input_conn_packet+0x682/0x820
   rxrpc_input_packet_on_conn.part.0+0x5f4/0xc30
   rxrpc_input_packet.constprop.0+0x8a2/0xea0
   rxrpc_io_thread+0x944/0x1570

rxrpc_post_challenge() doesn't appear in the reports because the
compiler inlined it into rxrpc_input_conn_packet().

Depending on the freed socket's contents, rxrpc_notify_socket_oob()
can also call a function pointer obtained from it (sk->sk_data_ready()
or rx->app_ops->notify_oob()).  The use-after-free is deterministic, an
unprivileged process can trigger it, and since the attacker controls
when the CHALLENGE is sent, they can reclaim the freed memory first.
These conditions make the bug reliably exploitable to hijack kernel
control flow and escalate privileges.

Note that without rxgk the stranded call still leaks.  A client call
pins its connection, so destroying the local endpoint hits the BUG()
in rxrpc_destroy_client_conn_ids(); and for any stranded call, netns
teardown hangs in rxrpc_destroy_all_calls() waiting for it to be
freed.

Fix this by poking the call from rxrpc_abort_eproto(), as
rxrpc_abort_calls() already does.

Fixes: 03fc55adf876 ("rxrpc: Only disconnect calls in the I/O thread")
Cc: stable@vger.kernel.org
Signed-off-by: Juan Agustin Niccolini <juanagusnicco@gmail.com>
---
A deterministic reproducer for the KASAN report above is available on
request.  A working privilege escalation exploit was sent to
security@kernel.org on 20 October, and I can share it with the rxrpc
and netdev maintainers upon request.  The bug was first reported to
the rxrpc maintainers and the security team on 11 October.  rxrpc
autoloads on Ubuntu, Fedora, and Arch when any process opens an
AF_RXRPC socket.

 net/rxrpc/ar-internal.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 865f05fe37ab..31528b5e34cb 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);
 	return -EPROTO;
 }
 

base-commit: fafbc4051123d5459ea6950899cec08ed946e278
-- 
2.53.0

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

[netdev-bot+sashiko]

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

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

[Juan Agustin Niccolini]

> 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

#2 is a real issue but wildly exaggerated.  #1 is real.  Neither was
caused by my patch, and I was already aware of both.

#1, the "critical" one, was found by my LLMs on October 29 and raised
with the maintainers and security@kernel.org on a private thread, along
with its patch and reproducer.  The reproducer triggers a UAF on
mainline without touching any of the code paths my patch touches,
which shows it was not introduced by it.  I will send that patch to
netdev now, since Sashiko has made the issue public.

#2 is a race that is nanoseconds wide, cannot be meaningfully widened
on non-RT kernels, and yields nothing exploitable if won; its severity
is low at best.

My patch did not cause either issue, nor did it make them worse than
they already are.  It is compatible with the patch that addresses #1.
Note that the issue my patch fixes is significantly worse, since that
UAF is deterministic and has a reliable LPE exploit.  I recommend
addressing it soon; the patch has been available since October 11.

Best,
Agustin