[PATCH net] net: ax25: fix integer overflow in ax25_rx_fragment()

[PATCH net] net: ax25: fix integer overflow in ax25_rx_fragment()

[Mashiro Chen]

The ax25_cb fragmentation reassembly accumulator:

  ax25->fraglen += skb->len;

operates on the unsigned short field 'fraglen' declared in ax25_cb:

  unsigned short  paclen, fragno, fraglen;

When fragments accumulate with a combined payload exceeding 65535
bytes, fraglen wraps to near zero.  The subsequent allocation:

  skb = alloc_skb(AX25_MAX_HEADER_LEN + ax25->fraglen, GFP_ATOMIC);

then allocates a tiny buffer.  Every skb_put() call in the copy loop
that follows writes far beyond the allocated headroom, corrupting
the kernel heap.

An attacker on an AX.25 link that supports multi-fragment I-frames
(AX25_SEG_FIRST / AX25_SEG_REM mechanism) can trigger this by
sending enough continuation fragments to wrap the 16-bit counter.
With AX.25 segment numbers limited to 6 bits (max 63 continuation
fragments), a fragment payload of ~1040 bytes per fragment is
sufficient to overflow.

Fix mirrors the identical bug fixed in NET/ROM (nr_in.c): check for
overflow before adding skb->len to fraglen, and abort fragment
reassembly cleanly if the limit would be exceeded.

Cc: stable@vger.kernel.org
Cc: linux-hams@vger.kernel.org
Signed-off-by: Mashiro Chen <mashiro.chen@mailbox.org>
---
 net/ax25/ax25_in.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/ax25/ax25_in.c b/net/ax25/ax25_in.c
index d75b3e9ed93de8..68202c19b19e3f 100644
--- a/net/ax25/ax25_in.c
+++ b/net/ax25/ax25_in.c
@@ -41,6 +41,11 @@ static int ax25_rx_fragment(ax25_cb *ax25, struct sk_buff *skb)
 				/* Enqueue fragment */
 				ax25->fragno = *skb->data & AX25_SEG_REM;
 				skb_pull(skb, 1);	/* skip fragno */
+				if ((unsigned int)ax25->fraglen + skb->len > USHRT_MAX) {
+					skb_queue_purge(&ax25->frag_queue);
+					ax25->fragno = 0;
+					return 1;
+				}
 				ax25->fraglen += skb->len;
 				skb_queue_tail(&ax25->frag_queue, skb);
 
-- 
2.53.0

Re: [PATCH net] net: ax25: fix integer overflow in ax25_rx_fragment()

[Joerg Reuter]

Am Thu, Apr 09, 2026 at 01:25:21AM +0800 schrieb Mashiro Chen:
> An attacker on an AX.25 link that supports multi-fragment I-frames
> (AX25_SEG_FIRST / AX25_SEG_REM mechanism) can trigger this by
> sending enough continuation fragments to wrap the 16-bit counter.
> With AX.25 segment numbers limited to 6 bits (max 63 continuation
> fragments), a fragment payload of ~1040 bytes per fragment is
> sufficient to overflow.

Even worse, it's 7 bits: https://www.ax25.net/AX25.2.2-Jul%2098-2.pdf
Figure 6.2 "Segment Header Format". Sigh.

Thanks,
     Joerg

Acked-by: Joerg Reuter <jreuter@yaina.de>
> Cc: stable@vger.kernel.org
> Cc: linux-hams@vger.kernel.org
> Signed-off-by: Mashiro Chen <mashiro.chen@mailbox.org>
> ---
>  net/ax25/ax25_in.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/ax25/ax25_in.c b/net/ax25/ax25_in.c
> index d75b3e9ed93de8..68202c19b19e3f 100644
> --- a/net/ax25/ax25_in.c
> +++ b/net/ax25/ax25_in.c
> @@ -41,6 +41,11 @@ static int ax25_rx_fragment(ax25_cb *ax25, struct sk_buff *skb)
>  				/* Enqueue fragment */
>  				ax25->fragno = *skb->data & AX25_SEG_REM;
>  				skb_pull(skb, 1);	/* skip fragno */
> +				if ((unsigned int)ax25->fraglen + skb->len > USHRT_MAX) {
> +					skb_queue_purge(&ax25->frag_queue);
> +					ax25->fragno = 0;
> +					return 1;
> +				}
>  				ax25->fraglen += skb->len;
>  				skb_queue_tail(&ax25->frag_queue, skb);
>  
> -- 
> 2.53.0
> 

-- 
Joerg Reuter                                    http://yaina.de/jreuter
And I make my way to where the warm scent of soil fills the evening air. 
Everything is waiting quietly out there....                 (Anne Clark)

Re: [PATCH net] net: ax25: fix integer overflow in ax25_rx_fragment()

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net: ax25: fix integer overflow in ax25_rx_fragment()

This commit addresses an integer overflow in the ax25_cb fragmentation
reassembly accumulator. It adds a check to ensure the combined payload
of fragments does not exceed USHRT_MAX. If the limit is exceeded, the
fragment queue is purged to prevent allocating a tiny buffer that leads
to a panic.

> Every skb_put() call in the copy loop
> that follows writes far beyond the allocated headroom, corrupting
> the kernel heap.

Is this description accurate?

skb_put() explicitly performs strict bounds checking and triggers a
deterministic kernel panic via skb_over_panic() if exceeded, rather than
silently corrupting the heap.

> Fix mirrors the identical bug fixed in NET/ROM (nr_in.c):

This isn't a bug, but is this the correct file reference? The check for
USHRT_MAX actually appears to exist in X.25 (x25_in.c), not NET/ROM.

> diff --git a/net/ax25/ax25_in.c b/net/ax25/ax25_in.c
> index d75b3e9ed93de..68202c19b19e3 100644
> --- a/net/ax25/ax25_in.c
> +++ b/net/ax25/ax25_in.c
> @@ -41,6 +41,11 @@ static int ax25_rx_fragment(ax25_cb *ax25, struct sk_buff *skb)
>  				/* Enqueue fragment */
>  				ax25->fragno = *skb->data & AX25_SEG_REM;
>  				skb_pull(skb, 1);	/* skip fragno */
> +				if ((unsigned int)ax25->fraglen + skb->len > USHRT_MAX) {

Is there a race condition with ax25->fraglen here?

If fragments for the same connection arrive concurrently on different CPUs,
both could evaluate this bounds check as safe and then concurrently add
their lengths.

> +					skb_queue_purge(&ax25->frag_queue);
> +					ax25->fragno = 0;
> +					return 1;

Does this code leak the skb?

When returning 1 here, the caller might skip freeing the skb because it
assumes the packet was successfully consumed or queued.

Could we explicitly call kfree_skb(skb) before returning 1, or return 0 to
instruct the caller to free it?

> +				}
>  				ax25->fraglen += skb->len;
>  				skb_queue_tail(&ax25->frag_queue, skb);

[ ... ]

>  					/* Copy data from the fragments */
>  					while ((skbo = skb_dequeue(&ax25->frag_queue)) != NULL) {
>  						skb_copy_from_linear_data(skbo,
>  							  skb_put(skbn, skbo->len),
>  									  skbo->len);

Could this read past the end of the skbo->data buffer if the fragment is
non-linear?

If the skb has paged data, skbo->len exceeds the linearly mapped data size,
which might trigger a panic or leak adjacent memory into the reassembled
packet.

Should this use skb_copy_bits() instead?

[ ... ]

>  	} else {
>  		/* First fragment received */
>  		if (*skb->data & AX25_SEG_FIRST) {
>  			skb_queue_purge(&ax25->frag_queue);
>  			ax25->fragno = *skb->data & AX25_SEG_REM;
>  			skb_pull(skb, 1);		/* skip fragno */
>  			ax25->fraglen = skb->len;

Is there a possibility of truncating the length here?

Since ax25->fraglen is an unsigned short, if a first fragment arrives with
skb->len > USHRT_MAX, it might be silently truncated.

Subsequent continuation fragments will be validated against this artificially
small fraglen, bypassing the new USHRT_MAX check.

Should the bounds check be applied to the first fragment as well?

>  			skb_queue_tail(&ax25->frag_queue, skb);
>  			return 1;
>  		}
>  	}