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

[Jeffrey E Altman <jaltman@auristor.com>]

[-- 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 --]