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

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

[Vasileios Almpanis]

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.

Fixes: 9fb4ab4d3dd6 ("ihex: Simplify next record offset calculation")
Cc: stable@vger.kernel.org
Signed-off-by: Vasileios Almpanis <vasilisalmpanis@gmail.com>
---
 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);
 }

---
base-commit: 89a312991dc6e638a36adc43ccb91dbc25504c04
change-id: 20260902-ihex-62b68b7e2bae

Best regards,
--  
Vasileios Almpanis <vasilisalmpanis@gmail.com>

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

[Greg Kroah-Hartman]

On Wed, Sep 02, 2026 at 12:21:19PM +0200, Vasileios Almpanis 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

Is this a real firmware image being sent?  Or a fake one?

And root triggers this, right?


> 
> Return size_t so neither the addition nor the following ALIGN() can wrap.
> 
> Fixes: 9fb4ab4d3dd6 ("ihex: Simplify next record offset calculation")
> Cc: stable@vger.kernel.org
> Signed-off-by: Vasileios Almpanis <vasilisalmpanis@gmail.com>
> ---
>  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)

but size_t changes depending on the platform, right?  Why not make it
u32 instead?

And if this hasn't shown a problem yet, in the 8 years since that commit
happened, is this really a bug?

What changed to suddenly cause this to show up?

thanks,

greg k-h

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

[Vasileios Almpanis]

On 2026-09-02 13:06:27+02:00, Greg Kroah-Hartman wrote:
> On Wed, Sep 02, 2026 at 12:21:19PM +0200, Vasileios Almpanis 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
> 
> Is this a real firmware image being sent?  Or a fake one?
> 

I used fake images. 14 bytes long for the out-of-bounds bug and 6 bytes
for the infinite loop. I passed them through the sysfs fallback loader.

Out-Of-Bounds bug image:
00 00 00 00 ff ff 00 00 00 00 00 00 00 00

Infinite-loop image:
00 00 00 00 ff fa

> And root triggers this, right?

Yes its root only. The attributes are accessible only through root
and it also needs raw-gadget.

> 
> > Return size_t so neither the addition nor the following ALIGN() can wrap.
> > 
> > Fixes: 9fb4ab4d3dd6 ("ihex: Simplify next record offset calculation")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Vasileios Almpanis <vasilisalmpanis@gmail.com>
> > ---
> >  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)
> 
> but size_t changes depending on the platform, right?  Why not make it
> u32 instead?

It does, but the value tops out at 65535 + 6 = 65541, so 17 bits, and
size_t is at least 32 bits. I chose size_t because sizeof(*p) is already
size_t. If you don't like it I can send v2 with u32.

> 
> And if this hasn't shown a problem yet, in the 8 years since that commit
> happened, is this really a bug?
> 
> What changed to suddenly cause this to show up?
> 

Actually nothing changed, I was just looking into
https://syzkaller.appspot.com/bug?extid=baf3cbba7dd980984f0d and ended
up reading ihex_validate_fw. There I noticed the 16 bit return value.
Both cases are reproduceable (infinite worker loop and out-of-bounds read).

> thanks,
> 
> greg k-h

Kind regards,

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

[Greg Kroah-Hartman]

On Wed, Sep 02, 2026 at 02:45:29PM +0200, Vasileios Almpanis wrote:
> On 2026-09-02 13:06:27+02:00, Greg Kroah-Hartman wrote:
> > On Wed, Sep 02, 2026 at 12:21:19PM +0200, Vasileios Almpanis 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
> > 
> > Is this a real firmware image being sent?  Or a fake one?
> > 
> 
> I used fake images. 14 bytes long for the out-of-bounds bug and 6 bytes
> for the infinite loop. I passed them through the sysfs fallback loader.

Cool, so root only, fake firmware, not a real issue?  :)

> Out-Of-Bounds bug image:
> 00 00 00 00 ff ff 00 00 00 00 00 00 00 00
> 
> Infinite-loop image:
> 00 00 00 00 ff fa
> 
> > And root triggers this, right?
> 
> Yes its root only. The attributes are accessible only through root
> and it also needs raw-gadget.
> 
> > 
> > > Return size_t so neither the addition nor the following ALIGN() can wrap.
> > > 
> > > Fixes: 9fb4ab4d3dd6 ("ihex: Simplify next record offset calculation")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Vasileios Almpanis <vasilisalmpanis@gmail.com>
> > > ---
> > >  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)
> > 
> > but size_t changes depending on the platform, right?  Why not make it
> > u32 instead?
> 
> It does, but the value tops out at 65535 + 6 = 65541, so 17 bits, and
> size_t is at least 32 bits. I chose size_t because sizeof(*p) is already
> size_t. If you don't like it I can send v2 with u32.

Let's be specific, otherwise people will trip over the fact that this is
different sizes.

thanks,

greg k-h

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

[Vasileios Almpanis]

On 2026-09-02 14:56:48+02:00, Greg Kroah-Hartman wrote:
> On Wed, Sep 02, 2026 at 02:45:29PM +0200, Vasileios Almpanis wrote:
> 
> > On 2026-09-02 13:06:27+02:00, Greg Kroah-Hartman wrote:
> > 
> > I used fake images. 14 bytes long for the out-of-bounds bug and 6 bytes
> > for the infinite loop. I passed them through the sysfs fallback loader.
> 
> Cool, so root only, fake firmware, not a real issue?  :)

Okay got it, i'll drop Cc: stable

> 
> > Out-Of-Bounds bug image:
> > 00 00 00 00 ff ff 00 00 00 00 00 00 00 00
> > 
> > Infinite-loop image:
> > 00 00 00 00 ff fa
> > 
> > 
> > Yes its root only. The attributes are accessible only through root
> > and it also needs raw-gadget.
> > 
> > 
> > It does, but the value tops out at 65535 + 6 = 65541, so 17 bits, and
> > size_t is at least 32 bits. I chose size_t because sizeof(*p) is already
> > size_t. If you don't like it I can send v2 with u32.
> 
> Let's be specific, otherwise people will trip over the fact that this is
> different sizes.

I'll change it to u32 in v2 shortly.

> 
> thanks,
> 
> greg k-h

Kind regard,
Vasileios Almpanis

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

[David Laight]

On Wed, 02 Sep 2026 12:21:19 +0200
Vasileios Almpanis <vasilisalmpanis@gmail.com> 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@vger.kernel.org
> Signed-off-by: Vasileios Almpanis <vasilisalmpanis@gmail.com>
> ---
>  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

>  }
> 
> ---
> base-commit: 89a312991dc6e638a36adc43ccb91dbc25504c04
> change-id: 20260902-ihex-62b68b7e2bae
> 
> Best regards,
> --  
> Vasileios Almpanis <vasilisalmpanis@gmail.com>
> 
> 

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

[Vasileios Almpanis]

On 9/2/26 5:31 PM, David Laight wrote:
> On Wed, 02 Sep 2026 12:21:19 +0200
> Vasileios Almpanis <vasilisalmpanis@gmail.com> 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@vger.kernel.org
>> Signed-off-by: Vasileios Almpanis <vasilisalmpanis@gmail.com>
>> ---
>>   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@gmail.com/T/#u
So the problem in this case is the fact that I misplace where the wrap 
happens.
Do you think it makes sense to send a v3 to fix the commit message?

Kind regards,

Vasileios Almpanis <vasilisalmpanis@gmail.com>

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@gmail.com> wrote:

> On 9/2/26 5:31 PM, David Laight wrote:
> > On Wed, 02 Sep 2026 12:21:19 +0200
> > Vasileios Almpanis <vasilisalmpanis@gmail.com> 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@vger.kernel.org
> >> Signed-off-by: Vasileios Almpanis <vasilisalmpanis@gmail.com>
> >> ---
> >>   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@gmail.com/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@gmail.com>
>