[PATCH] rxrpc: Fix read+write past skb_headlen in soft-ACK parser

[PATCH] rxrpc: Fix read+write past skb_headlen in soft-ACK parser

[Michael Bommarito]

rxrpc_input_soft_acks() builds a raw `u8 *acks = skb->data + ...`
pointer and walks it for `sp->ack.nr_acks` iterations, performing a
read-modify-write (shiftr_adv_rotr) on each byte.

The caller rxrpc_input_ack() only validates that the bytes exist
somewhere in the skb (`offset > skb->len - nr_acks`) and best-effort
linearises the head with skb_condense().  skb_condense() returns
without pulling when the skb is cloned, when paged data exceeds the
linear-head tailroom, or when frags are unreadable.  On a nonlinear
skb that survives the condense step (cloned by AF_PACKET capture,
frag_list-style after IP-fragment reassembly, or paged-frag receive
on real NICs), skb->data covers only the linear head.  The parser
then walks past skb_headlen(skb) into skb tailroom, skb_shared_info,
or the next slab object, doing in-place 1-byte shifts on up to 255
attacker-controlled offsets per ACK packet.

Sibling parsers in the same file already use the safe pattern:
rxrpc_extract_header(), rxrpc_extract_abort(), rxrpc_input_split_jumbo(),
and the rxrpc_input_ack_trailer() call site all use skb_copy_bits()
with explicit length checks.  The soft-ACK call path is the lone
direct-deref site.

Add an explicit pskb_may_pull() check before invoking the parser so
that the linear head is guaranteed to cover the SACK bitmap.  On
allocation failure return rxrpc_proto_abort() with the same
eproto_ackr_short_sack disposition the existing length check uses.
skb_condense() is retained on the path; its truesize-accounting side
effect is independent of the linearisation guarantee that
pskb_may_pull() now provides.

The bug shape was reproduced under UML+KASAN in two complementary
harnesses:

(1) A kmod that lifts the parser's inner shift loop verbatim and
    exercises it against a kmalloc(47) buffer.  KASAN reports a
    slab-out-of-bounds read on the first byte past the allocation:

      BUG: KASAN: slab-out-of-bounds in run_rxrpc_soft_acks_loop+0x52/0x74
      Read of size 1 at addr 63a7032f by task insmod/37
       which belongs to the cache kmalloc-64 of size 64
       allocated 47-byte region [63a70300, 63a7032f)

(2) A second kmod uses the in-kernel rxrpc API to allocate a real
    rxrpc_call, builds a nonlinear hostile ACK skb (linear head=46,
    paged frag=79, skb->cloned=1, nr_acks=60) and drives the
    upstream rxrpc_input_call_packet() -> rxrpc_input_ack() ->
    rxrpc_input_soft_acks() chain directly.  Sixty 0xAA sentinel
    bytes placed in the linear-head tailroom are all right-shifted
    to 0x55 by the unmodified upstream rxrpc_input_soft_acks() on
    a stock kernel.  On the patched kernel, zero of sixty shift --
    pskb_may_pull aborts the call before the parser runs.

Note: the real-path demonstration does NOT produce a literal
KASAN slab-out-of-bounds splat, because the on-wire nAcks field
is a u8 (max 255) and the OOB shift stays within the same kmalloc
slab object that holds skb_shared_info.  Per-byte corruption of
skb_shared_info and the linear-head tailroom is the actual
production effect.

A regression check on a fully-linear ACK skb confirms pskb_may_pull
is a no-op on that path; the parser continues to read in-bounds.

Fixes: d57a3a151660 ("rxrpc: Save last ACK's SACK table rather than marking txbufs")
Cc: stable@vger.kernel.org
Reported via internal source-audit pipeline on 2026-04-21.
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 net/rxrpc/input.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
index 24aceb183c2c..52ace0f98d06 100644
--- a/net/rxrpc/input.c
+++ b/net/rxrpc/input.c
@@ -1173,6 +1173,8 @@ static void rxrpc_input_ack(struct rxrpc_call *call, struct sk_buff *skb)
 	if (nr_acks > 0) {
 		if (offset > (int)skb->len - nr_acks)
 			return rxrpc_proto_abort(call, 0, rxrpc_eproto_ackr_short_sack);
+		if (!pskb_may_pull(skb, offset + nr_acks))
+			return rxrpc_proto_abort(call, 0, rxrpc_eproto_ackr_short_sack);
 		rxrpc_input_soft_acks(call, &summary, skb);
 	}
 
-- 
2.53.0

Re: [PATCH] rxrpc: Fix read+write past skb_headlen in soft-ACK parser

[Jeffrey E Altman]

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

On 5/13/2026 2:09 PM, Michael Bommarito wrote:
> rxrpc_input_soft_acks() builds a raw `u8 *acks = skb->data + ...`
> pointer and walks it for `sp->ack.nr_acks` iterations, performing a
> read-modify-write (shiftr_adv_rotr) on each byte.
>
> The caller rxrpc_input_ack() only validates that the bytes exist
> somewhere in the skb (`offset > skb->len - nr_acks`) and best-effort
> linearises the head with skb_condense().  skb_condense() returns
> without pulling when the skb is cloned, when paged data exceeds the
> linear-head tailroom, or when frags are unreadable.  On a nonlinear
> skb that survives the condense step (cloned by AF_PACKET capture,
> frag_list-style after IP-fragment reassembly, or paged-frag receive
> on real NICs), skb->data covers only the linear head.  The parser
> then walks past skb_headlen(skb) into skb tailroom, skb_shared_info,
> or the next slab object, doing in-place 1-byte shifts on up to 255
> attacker-controlled offsets per ACK packet.
>
> Sibling parsers in the same file already use the safe pattern:
> rxrpc_extract_header(), rxrpc_extract_abort(), rxrpc_input_split_jumbo(),
> and the rxrpc_input_ack_trailer() call site all use skb_copy_bits()
> with explicit length checks.  The soft-ACK call path is the lone
> direct-deref site.
>
> Add an explicit pskb_may_pull() check before invoking the parser so
> that the linear head is guaranteed to cover the SACK bitmap.  On
> allocation failure return rxrpc_proto_abort() with the same
> eproto_ackr_short_sack disposition the existing length check uses.
> skb_condense() is retained on the path; its truesize-accounting side
> effect is independent of the linearisation guarantee that
> pskb_may_pull() now provides.
>
> The bug shape was reproduced under UML+KASAN in two complementary
> harnesses:
>
> (1) A kmod that lifts the parser's inner shift loop verbatim and
>      exercises it against a kmalloc(47) buffer.  KASAN reports a
>      slab-out-of-bounds read on the first byte past the allocation:
>
>        BUG: KASAN: slab-out-of-bounds in run_rxrpc_soft_acks_loop+0x52/0x74
>        Read of size 1 at addr 63a7032f by task insmod/37
>         which belongs to the cache kmalloc-64 of size 64
>         allocated 47-byte region [63a70300, 63a7032f)
>
> (2) A second kmod uses the in-kernel rxrpc API to allocate a real
>      rxrpc_call, builds a nonlinear hostile ACK skb (linear head=46,
>      paged frag=79, skb->cloned=1, nr_acks=60) and drives the
>      upstream rxrpc_input_call_packet() -> rxrpc_input_ack() ->
>      rxrpc_input_soft_acks() chain directly.  Sixty 0xAA sentinel
>      bytes placed in the linear-head tailroom are all right-shifted
>      to 0x55 by the unmodified upstream rxrpc_input_soft_acks() on
>      a stock kernel.  On the patched kernel, zero of sixty shift --
>      pskb_may_pull aborts the call before the parser runs.
>
> Note: the real-path demonstration does NOT produce a literal
> KASAN slab-out-of-bounds splat, because the on-wire nAcks field
> is a u8 (max 255) and the OOB shift stays within the same kmalloc
> slab object that holds skb_shared_info.  Per-byte corruption of
> skb_shared_info and the linear-head tailroom is the actual
> production effect.
>
> A regression check on a fully-linear ACK skb confirms pskb_may_pull
> is a no-op on that path; the parser continues to read in-bounds.
>
> Fixes: d57a3a151660 ("rxrpc: Save last ACK's SACK table rather than marking txbufs")
> Cc: stable@vger.kernel.org
> Reported via internal source-audit pipeline on 2026-04-21.
> Assisted-by: Claude:claude-opus-4-7
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> ---
>   net/rxrpc/input.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
> index 24aceb183c2c..52ace0f98d06 100644
> --- a/net/rxrpc/input.c
> +++ b/net/rxrpc/input.c
> @@ -1173,6 +1173,8 @@ static void rxrpc_input_ack(struct rxrpc_call *call, struct sk_buff *skb)
>   	if (nr_acks > 0) {
>   		if (offset > (int)skb->len - nr_acks)
>   			return rxrpc_proto_abort(call, 0, rxrpc_eproto_ackr_short_sack);
> +		if (!pskb_may_pull(skb, offset + nr_acks))
> +			return rxrpc_proto_abort(call, 0, rxrpc_eproto_ackr_short_sack);
>   		rxrpc_input_soft_acks(call, &summary, skb);
>   	}
>   

Aborting the call because skb_condense() was unable to consolidate the 
rx ack packet data is an unfriendly thing to do.

As suggested by the commit message, copying the data before processing 
would be a friendlier solution to the identified problem.

Jeffrey Altman




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

Re: [PATCH] rxrpc: Fix read+write past skb_headlen in soft-ACK parser

[David Howells]

Jeffrey E Altman <jaltman@auristor.com> wrote:

> Aborting the call because skb_condense() was unable to consolidate the rx ack
> packet data is an unfriendly thing to do.
> 
> As suggested by the commit message, copying the data before processing would
> be a friendlier solution to the identified problem.

Agreed.  Something along the lines of the attached.  It still needs a bit more
polishing, though.

David
---
diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
index 24aceb183c2c..dde87b058a23 100644
--- a/net/rxrpc/input.c
+++ b/net/rxrpc/input.c
@@ -963,21 +963,30 @@ 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 a SACK table.  A SACK table can hold up to 256*8 ACK bits. */
+	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++;