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

[Mashiro Chen <mashiro.chen@mailbox.org>]

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@kernel.org> 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
>