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

[Mashiro Chen]

Hi Jakub, Simon

v3 has addressed the review comments on v2:
1. Add pskb_may_pull(skb, 1) before dereferencing skb->data
2. Remove the unnecessary (unsigned int) cast on fraglen
3. Fix skb leak in overflow path that kfree_skb(skb) before return 1
4. Reset ax25->fraglen = 0 after purge


P.S.:
the reassembly copy loop at ax25_in.c:75 uses 
skb_copy_from_linear_data(skbo, dst, skbo->len), which is equivalent to 
memcpy(skbo->data, dst, skbo->len).
If a queued skbo contains non-linear data, which means data_len > 0, 
this silently reads only the linear head and copies stale data for the 
remainder.
In practice, all AX.25 lower-layer drivers like mkiss and 6pack allocate 
fully linear skbs via dev_alloc_skb(), so this is not currently 
reachable, I think there should be a separated patch to fix this.

73s,
Mashiro Chen

On 4/13/26 05:05, David Laight wrote:
> On Sun, 12 Apr 2026 13:17:51 -0700
> Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
> > On Thu,  9 Apr 2026 10:50:26 +0800 Mashiro Chen wrote:
> > > 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.
> > Same problem as reported by Simon on the netrom patch applies here.
> >
> > nit: I don't think you need to cast ax25->fraglen to unsigned int
> > in the comparison. since it's added with skb->len it should get
> > auto-prompted to unsigned int.
> It wouldn't matter if that comparison were signed.
>
> Or change the type of ax25->fraglen to be 32bits and do the
> sanity check for overlong packets later in the code.
> I had a quick look at the header and the structure hasn't
> been size-optimised...
>
> 	David