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

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

[David Howells]

    
Fix modification of the received skbuff in rxrpc_input_soft_acks() and a
potential incorrect access of the buffer in a fragmented UDP packet (the
packet would probably have to be deliberately pre-generated as fragmented)
when AF_RXRPC tries to extract the contents of the SACK table by copying
out the contents of the SACK table into a buffer before attempting to parse
it.

AF_RXRPC assumes that it can just call skb_condense() and then validly
access the SACK table from skb->data and that it will be a flat buffer -
but skb_condense() can silently fail to do anything under some
circumstances.

Note that whilst rxrpc_input_soft_acks() should be able to parse extended
ACKs, the rest of AF_RXRPC doesn't currently support that.

Further, there's then no need to call skb_condense() in rxrpc_input_ack(),
so don't.

Fixes: d57a3a151660 ("rxrpc: Save last ACK's SACK table rather than marking txbufs")
Reported-by: Michael Bommarito <michael.bommarito@gmail.com>
Link: https://lore.kernel.org/r/20260513180907.2061972-1-michael.bommarito@gmail.com
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Jeffrey Altman <jaltman@auristor.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Eric Dumazet <edumazet@google.com>
cc: "David S. Miller" <davem@davemloft.net>
cc: Jakub Kicinski <kuba@kernel.org>
cc: Paolo Abeni <pabeni@redhat.com>
cc: Simon Horman <horms@kernel.org>
cc: linux-afs@lists.infradead.org
cc: netdev@vger.kernel.org
cc: stable@kernel.org
---
 input.c |   22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
index 24aceb183c2c..0c15ebf19eae 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;
 
 	_enter("%x,%x,%u", tq->qbase, seq, sp->ack.nr_acks);
 
 	while (after(seq, tq->qbase + RXRPC_NR_TXQUEUE - 1))
 		tq = tq->next;
 
+	/* Extract an individual SACK table.  A normal SACK table is up to 255
+	 * bytes with 1 ACK flag per byte, but an extended SACK table can be up
+	 * to 256 bytes with up to 8 ACK/NACK flags per byte.  The ACK flags go
+	 * across all bit 0's then all bit 1's, then all bit 2's, ...
+	 */
+	memset(sack, 0, sizeof(sack));
+	nsack = umin(sp->ack.nr_acks, 256);
+	if (skb_copy_bits(skb,
+			  sizeof(struct rxrpc_wire_header) + sizeof(struct rxrpc_ackpacket),
+			  sack, nsack) < 0)
+		return;
+
 	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;
 		}
 		seq++;
@@ -1117,9 +1130,6 @@ static void rxrpc_input_ack(struct rxrpc_call *call, struct sk_buff *skb)
 	    skb_copy_bits(skb, ioffset, &trailer, sizeof(trailer)) < 0)
 		return rxrpc_proto_abort(call, 0, rxrpc_badmsg_short_ack_trailer);
 
-	if (nr_acks > 0)
-		skb_condense(skb);
-
 	call->acks_latest_ts = ktime_get_real();
 	call->acks_hard_ack = hard_ack;
 	call->acks_prev_seq = prev_pkt;

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

[Jeffrey E Altman]

On 5/18/2026 6:44 PM, David Howells wrote:
>      
> Fix modification of the received skbuff in rxrpc_input_soft_acks() and a
> potential incorrect access of the buffer in a fragmented UDP packet (the
> packet would probably have to be deliberately pre-generated as fragmented)
> when AF_RXRPC tries to extract the contents of the SACK table by copying
> out the contents of the SACK table into a buffer before attempting to parse
> it.
>
> AF_RXRPC assumes that it can just call skb_condense() and then validly
> access the SACK table from skb->data and that it will be a flat buffer -
> but skb_condense() can silently fail to do anything under some
> circumstances.
>
> Note that whilst rxrpc_input_soft_acks() should be able to parse extended
> ACKs, the rest of AF_RXRPC doesn't currently support that.
>
> Further, there's then no need to call skb_condense() in rxrpc_input_ack(),
> so don't.
>
> Fixes: d57a3a151660 ("rxrpc: Save last ACK's SACK table rather than marking txbufs")
> Reported-by: Michael Bommarito <michael.bommarito@gmail.com>
> Link: https://lore.kernel.org/r/20260513180907.2061972-1-michael.bommarito@gmail.com
> Signed-off-by: David Howells <dhowells@redhat.com>
> Reviewed-by: Jeffrey Altman <jaltman@auristor.com>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: Eric Dumazet <edumazet@google.com>
> cc: "David S. Miller" <davem@davemloft.net>
> cc: Jakub Kicinski <kuba@kernel.org>
> cc: Paolo Abeni <pabeni@redhat.com>
> cc: Simon Horman <horms@kernel.org>
> cc: linux-afs@lists.infradead.org
> cc: netdev@vger.kernel.org
> cc: stable@kernel.org
> ---
>   input.c |   22 ++++++++++++++++------
>   1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
> index 24aceb183c2c..0c15ebf19eae 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;
>   
>   	_enter("%x,%x,%u", tq->qbase, seq, sp->ack.nr_acks);
>   
>   	while (after(seq, tq->qbase + RXRPC_NR_TXQUEUE - 1))
>   		tq = tq->next;
>   
> +	/* Extract an individual SACK table.  A normal SACK table is up to 255
> +	 * bytes with 1 ACK flag per byte, but an extended SACK table can be up
> +	 * to 256 bytes with up to 8 ACK/NACK flags per byte.  The ACK flags go
> +	 * across all bit 0's then all bit 1's, then all bit 2's, ...
> +	 */
> +	memset(sack, 0, sizeof(sack));
> +	nsack = umin(sp->ack.nr_acks, 256);
> +	if (skb_copy_bits(skb,
> +			  sizeof(struct rxrpc_wire_header) + sizeof(struct rxrpc_ackpacket),
> +			  sack, nsack) < 0)
> +		return;
> +
>   	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;
>   		}
>   		seq++;
> @@ -1117,9 +1130,6 @@ static void rxrpc_input_ack(struct rxrpc_call *call, struct sk_buff *skb)
>   	    skb_copy_bits(skb, ioffset, &trailer, sizeof(trailer)) < 0)
>   		return rxrpc_proto_abort(call, 0, rxrpc_badmsg_short_ack_trailer);
>   
> -	if (nr_acks > 0)
> -		skb_condense(skb);
> -
>   	call->acks_latest_ts = ktime_get_real();
>   	call->acks_hard_ack = hard_ack;
>   	call->acks_prev_seq = prev_pkt;
>
Reviewed-by: Jeffrey Altman <jaltman@auristor.com>

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

[Jakub Kicinski]

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

Re: [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