Skip to contentSkip to conversation

ihex: Fix 16 bit truncation in ihex_binrec_size()

PATCH Superseded Vasileios Almpanis 3 participants 8 messages Sep 2

Subsystems (2)
Stable branch USB

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Vasileios Almpanis

author
#0 lore raw

PATCH 1/1 ihex: Fix 16 bit truncation in ihex_binrec_size()

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 [email protected]
Signed-off-by Vasileios Almpanis <[email protected]>
---
 include/linux/ihex.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/ihex.h +1 -1
index b824877e6d1b..0da1c4e3e693 100644
@@ -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,

Signature · 1 line
Vasileios Almpanis <[email protected]>
maintainer
#1 lore raw
Quoted 10 lines · 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:

Quoted 10 lines
  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?

Quoted 15 lines

Return size_t so neither the addition nor the following ALIGN() can wrap.

Fixes 9fb4ab4d3dd6 ("ihex: Simplify next record offset calculation")
Cc [email protected]
Signed-off-by Vasileios Almpanis <[email protected]>
---
 include/linux/ihex.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/ihex.h +0 -0
index b824877e6d1b..0da1c4e3e693 100644

include/linux/ihex.h struct ihex_binrec { · line 24

Quoted 5 lines
include/linux/ihex.h +1 -1
@@ -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

author
#2 lore raw
Quoted 23 lines · Greg Kroah-Hartman
Quoted 10 lines · 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:

Quoted 10 lines
  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
Quoted 2 lines

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

Quoted 1 line

And root triggers this, right?

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

Quoted 18 lines
Quoted 18 lines

Return size_t so neither the addition nor the following ALIGN() can wrap.

Fixes 9fb4ab4d3dd6 ("ihex: Simplify next record offset calculation")
Cc [email protected]
Signed-off-by Vasileios Almpanis <[email protected]>
---
 include/linux/ihex.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/ihex.h +0 -0
index b824877e6d1b..0da1c4e3e693 100644
@@ -21,7 +21,7 @@ struct ihex_binrec {
 	uint8_t data[];
 } __attribute__((packed));
Quoted 5 lines

include/linux/ihex.h struct ihex_binrec { · line 24

Quoted 2 lines
include/linux/ihex.h +1 -1
-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.

Quoted 6 lines

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).

Quoted 3 lines

thanks,

greg k-h

Kind regards,

maintainer
#3 lore raw
Quoted 24 lines · Vasileios Almpanis
Quoted 23 lines · Greg Kroah-Hartman
Quoted 20 lines · 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
Quoted 5 lines
Quoted 2 lines

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? :)

Quoted 35 lines

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

Quoted 1 line

And root triggers this, right?

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

Quoted 18 lines
Quoted 18 lines

Return size_t so neither the addition nor the following ALIGN() can wrap.

Fixes 9fb4ab4d3dd6 ("ihex: Simplify next record offset calculation")
Cc [email protected]
Signed-off-by Vasileios Almpanis <[email protected]>
---
 include/linux/ihex.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/ihex.h +0 -0
index b824877e6d1b..0da1c4e3e693 100644
@@ -21,7 +21,7 @@ struct ihex_binrec {
 	uint8_t data[];
 } __attribute__((packed));
Quoted 5 lines

include/linux/ihex.h struct ihex_binrec { · line 24

Quoted 2 lines
include/linux/ihex.h +1 -1
-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?

Quoted 3 lines

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

author
#4 lore raw
Quoted 8 lines · Greg Kroah-Hartman
Quoted 4 lines · Vasileios Almpanis

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

Quoted 12 lines
Quoted 12 lines

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.

Quoted 6 lines
Quoted 3 lines

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.

Quoted 4 lines

thanks,

greg k-h

Kind regard,
Vasileios Almpanis

#5 lore raw
In reply to Vasileios Almpanis
Quoted 21 lines · On Wed, 02 Sep 2026 12:21:19 +0200 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
Quoted 1 line

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...)

Quoted 12 lines
Fixes 9fb4ab4d3dd6 ("ihex: Simplify next record offset calculation")
Cc [email protected]
Signed-off-by Vasileios Almpanis <[email protected]>
---
 include/linux/ihex.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/ihex.h +0 -0
index b824877e6d1b..0da1c4e3e693 100644

include/linux/ihex.h struct ihex_binrec { · line 26

Quoted 7 lines
include/linux/ihex.h +1 -1
@@ -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

Quoted 11 lines
include/linux/ihex.h +0 -1
 }
 
---

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

Best regards,

Signature · 1 line
Vasileios Almpanis <[email protected]>
author
#6 lore raw
Quoted 49 lines · David Laight
Quoted 21 lines · On Wed, 02 Sep 2026 12:21:19 +0200 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
Quoted 1 line

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...)

Quoted 15 lines
Fixes 9fb4ab4d3dd6 ("ihex: Simplify next record offset calculation")
Cc [email protected]
Signed-off-by Vasileios Almpanis <[email protected]>
---
  include/linux/ihex.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/ihex.h +0 -0
index b824877e6d1b..0da1c4e3e693 100644
@@ -21,7 +21,7 @@ struct ihex_binrec {
  	uint8_t data[];
  } __attribute__((packed));
Quoted 8 lines

include/linux/ihex.h struct ihex_binrec { · line 26

Quoted 4 lines
include/linux/ihex.h +1 -1
-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/[email protected]/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 <[email protected]>

#7 lore raw
Quoted 57 lines · On Wed, 2 Sep 2026 18:35:28 +0200 Vasileios Almpanis…
Quoted 56 lines · David Laight
Quoted 21 lines · On Wed, 02 Sep 2026 12:21:19 +0200 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
Quoted 1 line

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...)

Quoted 12 lines
Fixes 9fb4ab4d3dd6 ("ihex: Simplify next record offset calculation")
Cc [email protected]
Signed-off-by Vasileios Almpanis <[email protected]>
---
  include/linux/ihex.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/ihex.h +0 -0
index b824877e6d1b..0da1c4e3e693 100644

include/linux/ihex.h struct ihex_binrec { · line 26

Quoted 7 lines
include/linux/ihex.h +1 -1
@@ -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.)

Quoted 8 lines
Quoted 1 line

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/[email protected]/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().

Quoted 1 line

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

Probably doesn't matter that much.

David

Quoted 5 lines

Kind regards,

Vasileios Almpanis <[email protected]>