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

[PATCH] 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>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Jeffrey Altman <jaltman@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] rxrpc: Fix the ACK parser to extract the SACK table for parsing

[Jeffrey E Altman]

[-- Attachment #1: Type: text/plain, Size: 3849 bytes --]

On 5/17/2026 10:27 AM, 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>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: Jeffrey Altman <jaltman@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>



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4467 bytes --]