Re: [PATCH net 2/4] rxrpc: Fix conn-level packet handling to unshare RESPONSE packets

[Simon Horman <horms@kernel.org>]

From: '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
---
Subject: rxrpc: Fix conn-level packet handling to unshare RESPONSE packets

This commit modifies the RxRPC connection-level packet processor to unshare
RESPONSE packets before decryption. It introduces a new rxrpc_unshare_skb()
helper to safely unshare skbuffs passed via a double pointer and replaces
direct calls to skb_unshare().

Closes: https://sashiko.dev/#/patchset/20260408121252.2249051-1-dhowells%40redhat.com

> diff --git a/net/rxrpc/conn_event.c b/net/rxrpc/conn_event.c
> index 9a41ec708aeb9..3d56a5d233697 100644
> --- a/net/rxrpc/conn_event.c
> +++ b/net/rxrpc/conn_event.c

[ ... ]

> @@ -371,7 +379,7 @@ static void rxrpc_do_process_connection(struct rxrpc_connection *conn)
>  	 * connection that each one has when we've finished with it */
>  	while ((skb = skb_dequeue(&conn->rx_queue))) {
>  		rxrpc_see_skb(skb, rxrpc_skb_see_conn_work);
> -		ret = rxrpc_process_event(conn, skb);
> +		ret = rxrpc_process_event(conn, &skb);
>  		switch (ret) {
>  		case -ENOMEM:
>  		case -EAGAIN:

Looking at the code immediately following this hunk in
rxrpc_do_process_connection():

		switch (ret) {
		case -ENOMEM:
		case -EAGAIN:
			skb_queue_head(&conn->rx_queue, skb);
			rxrpc_queue_conn(conn, rxrpc_conn_queue_retry_work);
			break;

If rxrpc_process_event() fails with -ENOMEM because rxrpc_unshare_skb()
failed, rxrpc_unshare_skb() will have set *_skb = NULL.

Does this mean skb is NULL when we reach skb_queue_head()?

Since skb_queue_head() dereferences the skb parameter, this appears to
result in a NULL pointer dereference. Furthermore, since skb_unshare()
frees the original skb on allocation failure, the packet is already gone
and requeuing it wouldn't be possible anyway.

> diff --git a/net/rxrpc/io_thread.c b/net/rxrpc/io_thread.c
> index 6979569319252..0592ce644fc3d 100644
> --- a/net/rxrpc/io_thread.c
> +++ b/net/rxrpc/io_thread.c

[ ... ]

> @@ -249,19 +249,10 @@ static bool rxrpc_input_packet(struct rxrpc_local *local, struct sk_buff **_skb)
>  		 * decryption.
>  		 */
>  		if (sp->hdr.securityIndex != 0) {
> -			skb = skb_unshare(skb, GFP_ATOMIC);
> -			if (!skb) {
> -				rxrpc_eaten_skb(*_skb, rxrpc_skb_eaten_by_unshare_nomem);
> -				*_skb = NULL;
> +			skb = rxrpc_unshare_skb(_skb, GFP_ATOMIC);
> +			if (!skb)
>  				return just_discard;
> -			}

This isn't a new bug introduced by this patch, but since we are modifying
the unshare path here: when rxrpc_unshare_skb() fails, it sets *_skb = NULL
and returns just_discard.

Back in rxrpc_io_thread(), the packet is processed like this:

net/rxrpc/io_thread.c:rxrpc_io_thread() {
...
		while ((skb = __skb_dequeue(&rx_queue))) {
			struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
			switch (skb->mark) {
			case RXRPC_SKB_MARK_PACKET:
				skb->priority = 0;
				if (!rxrpc_input_packet(local, &skb))
					rxrpc_reject_packet(local, skb);
				trace_rxrpc_rx_done(skb->mark, skb->priority);
...

If rxrpc_input_packet() sets skb to NULL and returns false (just_discard),
does this cause a NULL pointer dereference when trace_rxrpc_rx_done() tries
to access skb->mark?