Re: [PATCH] ihex: Fix 16 bit truncation in ihex_binrec_size()

[David Laight]

On Wed, 2 Sep 2026 18:35:28 +0200
Vasileios Almpanis <vasilisalmpanis@xxxxxxxxx> wrote:

> On 9/2/26 5:31 PM, David Laight wrote:
> > On Wed, 02 Sep 2026 12:21:19 +0200
> > Vasileios Almpanis <vasilisalmpanis@xxxxxxxxx> wrote:
> >  
> >> ihex_binrec_size() returns uint16_t while computing be16_to_cpu(p->len) +
> >> sizeof(struct ihex_binrec), so record lengths of 65530 and above wrap.
> >> __ihex_next_binrec() uses the result as the offset to the next record. A
> >> length of 65530 gives an advance of zero, so ihex_validate_fw() spins on
> >> the same record forever. Lengths of 65531 to 65535 advance by 4 or 8
> >> instead of 65544, so a 14 byte image passes validation while its first
> >> record claims 65535 bytes of payload. emi26_load_firmware() passes it to
> >> emi26_writememory(), which kmemdup()s 65535 bytes out of a 14 byte buffer
> >> producing the following splat:
> >>
> >>    BUG: KASAN: vmalloc-out-of-bounds in kmemdup_noprof+0x3b/0x50
> >>    Read of size 65535 at addr ffffc90000075006 by task kworker/11:1/174
> >>    Workqueue: usb_hub_wq hub_event
> >>    Call Trace:
> >>     <TASK>
> >>     kasan_check_range+0x10f/0x1e0
> >>     __asan_memcpy+0x23/0x60
> >>     kmemdup_noprof+0x3b/0x50
> >>     emi26_writememory+0x29/0xd0
> >>     emi26_probe+0x2d1/0xb64
> >>
> >> Return size_t so neither the addition nor the following ALIGN() can wrap.  
> > The ALIGN() can't wrap, the u16 value is promoted to 'int' before anything
> > is done with it.
> > What it does save is the pointless '&= 0xffff' after the add.
> >
> > Since the result is added to a pointer it will need promoting to 'long'.
> > But the compiler can assume that adding sizeof(*p) will zero the high
> > bits and nothing extra is generated.
> > (Not that this is a super-hot path...)
> >  
> >> Fixes: 9fb4ab4d3dd6 ("ihex: Simplify next record offset calculation")
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> Signed-off-by: Vasileios Almpanis <vasilisalmpanis@xxxxxxxxx>
> >> ---
> >>   include/linux/ihex.h | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/include/linux/ihex.h b/include/linux/ihex.h
> >> index b824877e6d1b..0da1c4e3e693 100644
> >> --- a/include/linux/ihex.h
> >> +++ b/include/linux/ihex.h
> >> @@ -21,7 +21,7 @@ struct ihex_binrec {
> >>   	uint8_t data[];
> >>   } __attribute__((packed));
> >>   
> >> -static inline uint16_t ihex_binrec_size(const struct ihex_binrec *p)
> >> +static inline size_t ihex_binrec_size(const struct ihex_binrec *p)
> >>   {
> >>   	return be16_to_cpu(p->len) + sizeof(*p);  
> > I'd always put those in the other order - matching the memory contents.
> > (But changing it would be churn.)
> >
> > David  
> Hi David,
> 
> Thanks for taking the time to review my patch.
> I had already posted a v2 before your mail arrived.
> https://lore.kernel.org/all/20260902-ihex-v2-1-30bb117cfc77@xxxxxxxxx/T/#u
> So the problem in this case is the fact that I misplace where the wrap 
> happens.

The wrap happens in the function return - as you said.
But there isn't a second wrap 'opportunity' in the ALIGN().

> Do you think it makes sense to send a v3 to fix the commit message?

Probably doesn't matter that much.

David

> 
> Kind regards,
> 
> Vasileios Almpanis <vasilisalmpanis@xxxxxxxxx>
>