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

[Simon Horman <horms@kernel.org>]

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;
>  		}
>  	}