Re: [PATCH net] rxrpc: Fix the ACK parser to extract the SACK table for parsing

[Jakub Kicinski <kuba@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.
---
rxrpc: Fix the ACK parser to extract the SACK table for parsing

This commit fixes modification of the received skbuff and incorrect access
of the buffer in fragmented UDP packets during SACK table parsing. It copies
out the contents of the SACK table into a flat stack buffer instead of
relying on skb_condense().

Link: https://lore.kernel.org/r/20260513180907.2061972-1-michael.bommarito@gmail.com

> diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
> index 24aceb183c2c3..0c15ebf19eae9 100644
> --- a/net/rxrpc/input.c
> +++ b/net/rxrpc/input.c
> @@ -963,21 +963,34 @@ static void rxrpc_input_soft_acks(struct rxrpc_call *call,
>  	struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
>  	struct rxrpc_txqueue *tq = call->tx_queue;
>  	unsigned long extracted = ~0UL;
> -	unsigned int nr = 0;
> +	unsigned int nr = 0, nsack;
>  	rxrpc_seq_t seq = call->acks_hard_ack + 1;
>  	rxrpc_seq_t lowest_nak = seq + sp->ack.nr_acks;
> -	u8 *acks = skb->data + sizeof(struct rxrpc_wire_header) + sizeof(struct rxrpc_ackpacket);
> +	u8 sack[256] __aligned(sizeof(unsigned long));
> +	u8 *acks = sack;

[ ... ]

>  	for (unsigned int i = 0; i < sp->ack.nr_acks; i++) {
>  		/* Decant ACKs until we hit a txqueue boundary. */
>  		shiftr_adv_rotr(acks, extracted);
>  		if (i == 256) {
> -			acks -= i;
> +			acks = sack;
>  			i = 0;
>  		}

Can this lead to an out-of-bounds access and an infinite loop if
sp->ack.nr_acks is greater than 256?

If a received packet has sp->ack.nr_acks > 256, on the 257th iteration
(where i == 256), the shiftr_adv_rotr(acks, extracted) macro will execute
before the wrap-around check resets the acks pointer.

Since the acks pointer has been incremented 256 times by previous iterations,
wouldn't it point to sack + 256 at this point, causing an out-of-bounds read
and write on the sack[] array?

Furthermore, resetting i to 0 inside the loop means the loop's i++
operation will set it to 1 for the next iteration. Because the counter is
reset without breaking out of the loop, will this ever terminate if
sp->ack.nr_acks > 256, or will it cycle infinitely and eventually cause a NULL
pointer dereference when tq->next runs out of transmit queues?
-- 
pw-bot: cr