Re: [PATCH net v2 07/10] rxrpc: Fix oob challenge leak in cleanup after notification failure

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
rxrpc: Fix oob challenge leak in cleanup after notification failure

Fix rxrpc_notify_socket_oob() to return an indication of failure in the
event that it failed to queue a packet and fix rxrpc_post_challenge() to
clean up the connection ref in such an event.

Link: https://sashiko.dev/#/patchset/20260616155749.2125907-1-dhowells%40redhat.com

> diff --git a/net/rxrpc/conn_event.c b/net/rxrpc/conn_event.c
> index c96ca615b787c..611c790bc6d0c 100644
> --- a/net/rxrpc/conn_event.c
> +++ b/net/rxrpc/conn_event.c
> @@ -436,7 +436,7 @@ static bool rxrpc_post_challenge(struct rxrpc_connection *conn,
>  	struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
>  	struct rxrpc_call *call = NULL;
>  	struct rxrpc_sock *rx;
> -	bool respond = false;
> +	bool respond = false, queued = false;

[Severity: Critical]
This is a pre-existing issue, but can the lockless read of
conn->channels[i].call in net/rxrpc/conn_event.c lead to a NULL pointer
dereference?

Looking at rxrpc_post_challenge(), the code does:

    if (conn->channels[i].call) {
        call = conn->channels[i].call;
        rx = rcu_dereference(call->socket);

Without READ_ONCE(), the compiler could issue two separate loads. If a
concurrent thread clears the channel's call pointer after the if-check but
before the assignment, call becomes NULL.

Does this cause the subsequent rcu_dereference(call->socket) to panic by
dereferencing NULL->socket?

[Severity: High]
This isn't a bug introduced by this patch, but does taking a connection
reference here in rxrpc_post_challenge() lead to a leak if the packet is
later discarded due to connection abort or teardown?

If rxrpc_process_event() in net/rxrpc/conn_event.c encounters 
conn->state == RXRPC_CONN_ABORTED, it returns -ECONNABORTED early:

    if (conn->state == RXRPC_CONN_ABORTED)
        return -ECONNABORTED;

This bypasses the cleanup, and the skb is consumed by
rxrpc_do_process_connection() without dropping the sp->chall.conn reference.

Similarly, during teardown, rxrpc_purge_queue() in net/rxrpc/skbuff.c
consumes skbs via:

    while ((skb = skb_dequeue((list))) != NULL) {
        ...
        consume_skb(skb);
    }

Since it doesn't inspect the cb scratch space, doesn't this permanently leak
the connection references stored in sp->chall.conn?

[ ... ]

>  	if (call)
> -		rxrpc_notify_socket_oob(call, skb);
> +		queued = rxrpc_notify_socket_oob(call, skb);

[Severity: High]
This is a pre-existing issue, but does failing to set skb->mark to
RXRPC_OOB_CHALLENGE here in rxrpc_post_challenge() break OOB challenge
processing?

The skb->mark remains RXRPC_SKB_MARK_PACKET as set during ingress.
When the application attempts to respond via rxrpc_respond_to_oob() or read
via rxrpc_recvmsg_oob(), won't the operation fail with -EPROTO because it
expects skb->mark == RXRPC_OOB_CHALLENGE?

Additionally, when the OOB message is freed via rxrpc_kernel_free_oob(),
doesn't the switch statement on oob->mark fail to match, bypassing
rxrpc_put_connection() and leaking the connection reference stored in
sp->chall.conn?

>  	rcu_read_unlock();
> +	if (call && !queued) {
> +		rxrpc_put_connection(conn, rxrpc_conn_put_challenge_input);
> +		sp->chall.conn = NULL;
> +		return false;
> +	}
>  
>  	if (!call)
>  		rxrpc_post_packet_to_conn(conn, skb);
> diff --git a/net/rxrpc/oob.c b/net/rxrpc/oob.c
> index 3318c8bd82ad5..c80ee2487d094 100644
> --- a/net/rxrpc/oob.c
> +++ b/net/rxrpc/oob.c

[ ... ]

> @@ -49,6 +50,7 @@ void rxrpc_notify_socket_oob(struct rxrpc_call *call, struct sk_buff *skb)
>  			skb->skb_mstamp_ns = rx->oob_id_counter++;
>  			rxrpc_get_skb(skb, rxrpc_skb_get_post_oob);
>  			skb_queue_tail(&rx->recvmsg_oobq, skb);
> +			queued = true;

[Severity: High]
This isn't a bug introduced by this patch, but does unbounded queuing of
challenge packets onto recvmsg_oobq risk a kernel OOM?

In net/rxrpc/oob.c, rxrpc_notify_socket_oob() uses skb_queue_tail() without
checking memory limits (like sk_rmem_alloc vs sk_rcvbuf) or restricting the
total number of challenges.

Since rxrpc_encap_rcv() intercepts these packets before standard UDP memory
accounting occurs, could a remote attacker flood the host with valid
CHALLENGE packets?

If the application is slow to process them or ignores them, will these
accumulate infinitely in recvmsg_oobq and cause a Denial of Service?