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

[Jeffrey E Altman]

On 5/20/2026 10:19 PM, Jakub Kicinski wrote:
> 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?

Due to the lack of support for Extended SACK within the current rxrpc 
implementation the value of ack.nr_acks cannot exceed 255. The value of 
ack.nr_acks is assigned in rxrpc_extract_header() from the struct 
rxrpc_ackpacket.nAcks field which is an unsigned 8-bit value whose 
maximum is 255.

When support for Extended SACK tables is added the processing in 
rxrpc_input_soft_acks() must be updated.

Jeffrey Altman