[PATCH] usb: r8a66597-hcd: fix buffer overflow on odd-length FIFO reads

[PATCH] usb: r8a66597-hcd: fix buffer overflow on odd-length FIFO reads

[Karl Mehltretter]

For an external R8A66597 (pdata->on_chip is false), the driver accesses
the FIFO 16 bits at a time. It rounds an odd byte count up to the next
word and passes that word count to ioread16_rep(), which stores both bytes
of every word in the caller's buffer. The final word therefore writes one
byte beyond an odd-length read, past the end of the buffer when the read
fills it.

This is visible while enumerating a USB device on an SH7785LCR. The USB
core allocates nine bytes for the configuration descriptor header, and
the controller driver stores ten bytes in it. SLUB reports the first
redzone byte changing from 0xcc to 0x09 in usb_get_configuration().
Odd-sized HID report descriptors trigger the same overwrite.

Section 2.8.5 of the R8A66597 datasheet requires software to discard the
excess byte after a 16-bit FIFO read when DTLN is odd. Read the trailing
byte through a temporary word and copy only that byte.

Fixes: 5d3043586db4 ("USB: r8a66597-hcd: host controller driver for R8A66597")
Cc: stable@vger.kernel.org
Reported-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
Link: https://lore.kernel.org/all/3bd32eaf159db61ed1d423e1d52a869b3689c682.camel@physik.fu-berlin.de/
Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---

Reproduced with 32-bit and 29-bit SH7785LCR kernels against a local
R8A66597 QEMU model. Before this patch, slub_debug=FZPU reports overflows
for the 9-byte configuration header and the 63-byte HID report descriptor.
An A/B test of this patch with the 32-bit kernel enumerates the keyboard
and removes both reports.
Testing the fix on real hardware is welcome.

 drivers/usb/host/r8a66597.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/r8a66597.h b/drivers/usb/host/r8a66597.h
--- a/drivers/usb/host/r8a66597.h
+++ b/drivers/usb/host/r8a66597.h
@@ -178,8 +178,15 @@ static inline void r8a66597_read_fifo(struct r8a66597 *r8a66597,
 			       len & 0x03);
 		}
 	} else {
-		len = (len + 1) / 2;
-		ioread16_rep(fifoaddr, buf, len);
+		count = len / 2;
+		ioread16_rep(fifoaddr, buf, count);
+
+		if (len & 0x00000001) {
+			u16 tmp;
+
+			ioread16_rep(fifoaddr, &tmp, 1);
+			memcpy((unsigned char *)buf + count * 2, &tmp, 1);
+		}
 	}
 }
 
-- 
2.53.0

Re: [PATCH] usb: r8a66597-hcd: fix buffer overflow on odd-length FIFO reads

[John Paul Adrian Glaubitz]

Hi,

On Fri, 2026-10-02 at 23:32 +0200, Karl Mehltretter wrote:
> For an external R8A66597 (pdata->on_chip is false), the driver accesses
> the FIFO 16 bits at a time. It rounds an odd byte count up to the next
> word and passes that word count to ioread16_rep(), which stores both bytes
> of every word in the caller's buffer. The final word therefore writes one
> byte beyond an odd-length read, past the end of the buffer when the read
> fills it.
> 
> This is visible while enumerating a USB device on an SH7785LCR. The USB
> core allocates nine bytes for the configuration descriptor header, and
> the controller driver stores ten bytes in it. SLUB reports the first
> redzone byte changing from 0xcc to 0x09 in usb_get_configuration().
> Odd-sized HID report descriptors trigger the same overwrite.
> 
> Section 2.8.5 of the R8A66597 datasheet requires software to discard the
> excess byte after a 16-bit FIFO read when DTLN is odd. Read the trailing
> byte through a temporary word and copy only that byte.
> 
> Fixes: 5d3043586db4 ("USB: r8a66597-hcd: host controller driver for R8A66597")
> Cc: stable@vger.kernel.org
> Reported-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> Link: https://lore.kernel.org/all/3bd32eaf159db61ed1d423e1d52a869b3689c682.camel@physik.fu-berlin.de/
> Assisted-by: LLM
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
> ---
> 
> Reproduced with 32-bit and 29-bit SH7785LCR kernels against a local
> R8A66597 QEMU model. Before this patch, slub_debug=FZPU reports overflows
> for the 9-byte configuration header and the 63-byte HID report descriptor.
> An A/B test of this patch with the 32-bit kernel enumerates the keyboard
> and removes both reports.
> Testing the fix on real hardware is welcome.
> 
>  drivers/usb/host/r8a66597.h | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/r8a66597.h b/drivers/usb/host/r8a66597.h
> --- a/drivers/usb/host/r8a66597.h
> +++ b/drivers/usb/host/r8a66597.h
> @@ -178,8 +178,15 @@ static inline void r8a66597_read_fifo(struct r8a66597 *r8a66597,
>  			       len & 0x03);
>  		}
>  	} else {
> -		len = (len + 1) / 2;
> -		ioread16_rep(fifoaddr, buf, len);
> +		count = len / 2;
> +		ioread16_rep(fifoaddr, buf, count);
> +
> +		if (len & 0x00000001) {
> +			u16 tmp;
> +
> +			ioread16_rep(fifoaddr, &tmp, 1);
> +			memcpy((unsigned char *)buf + count * 2, &tmp, 1);
> +		}
>  	}
>  }
>  

Without the patch, the following memory leaks are reported:

[    9.676000] [kmalloc Redzone overwritten] 0x820c4229-0x820c4229 @offset=553. First byte 0x9 instead of 0xcc
[    9.676000] =============================================================================
[    9.676000] BUG kmalloc-32 (Not tainted): Object corrupt
[    9.676000] -----------------------------------------------------------------------------
[    9.676000] 
[    9.676000] Allocated in usb_get_configuration+0x12c/0x11d8 age=51 cpu=0 pid=10
[    9.676000]  _raw_spin_lock_irqsave+0x20/0x38
[    9.676000]  ___slab_alloc+0x21e/0x44c
[    9.676000]  _raw_spin_unlock_irqrestore+0xe/0x44
[    9.676000]  __alloc_object+0xaa/0x19c
[    9.676000]  memset+0x0/0x8c
[    9.676000]  __kmalloc_noprof+0xb0/0x1c0
[    9.676000]  memset+0x0/0x8c
[    9.676000]  _kzalloc_noprof.constprop.0+0xc/0x1c
[    9.676000]  usb_get_configuration+0x12c/0x11d8
[    9.676000]  usb_get_configuration+0x12c/0x11d8
[    9.676000]  _kzalloc_noprof.constprop.0+0x0/0x1c
[    9.676000]  set_next_task_fair+0x190/0x350
[    9.676000]  __schedule+0x5aa/0x6bc
[    9.676000]  _raw_spin_lock_irqsave+0x20/0x38
[    9.676000]  _raw_spin_unlock_irqrestore+0xe/0x44
[    9.676000]  __try_to_del_timer_sync+0x4a/0x88
[    9.676000] Slab 0x9ff41880 objects=32 used=6 fp=0x820c4320 flags=0x40000200(workingset|section=16|zone=0)
[    9.676000] Object 0x820c4220 @offset=544 fp=0x820c42a0
[    9.676000] 
[    9.676000] Redzone  820c4200: cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc  ................
[    9.676000] Redzone  820c4210: cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc  ................
[    9.676000] Object   820c4220: 09 02 20 00 01 01 00 80 fa 09 cc cc cc cc cc cc  .. .............
[    9.676000] Object   820c4230: cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc  ................
[    9.676000] Redzone  820c4240: cc cc cc cc                                      ....
[    9.676000] Padding  820c4274: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a              ZZZZZZZZZZZZ
[    9.676000] Disabling lock debugging due to kernel taint
[    9.676000] ------------[ cut here ]------------
[    9.676000] WARNING: mm/slub.c:1257 at object_err+0x46/0x158, CPU#0: kworker/0:1/10
[    9.676000] Modules linked in:
[    9.676000] 
[    9.676000] CPU: 0 UID: 0 PID: 10 Comm: kworker/0:1 Tainted: G    B               7.3.0-rc5-00340-ge767a4ea70a3 #6 PREEMPT 
[    9.676000] Tainted: [B]=BAD_PAGE
[    9.676000] Workqueue: usb_hub_wq hub_event
[    9.676000] PC is at object_err+0x46/0x158
[    9.676000] PR is at object_err+0x46/0x158
[    9.676000] PC  : 80004e3a SP  : 810cdc20 SR  : 400081f1 TEA : c00d0008
[    9.676000] R0  : 00000020 R1  : 8074959c R2  : 00000000 R3  : 00000020
[    9.676000] R4  : 00000001 R5  : ff623224 R6  : 00000000 R7  : 00000000
[    9.676000] R8  : 810023e0 R9  : 820c4220 R10 : 00000054 R11 : 80004d48
[    9.676000] R12 : 0000808f R13 : 80004bd4 R14 : 810cdc20
[    9.676000] MACH: 00000043 MACL: 0002bfa8 GBR : 2958a4c0 PR  : 80004e3a
[    9.676000] 
[    9.676000] Call trace:
[    9.676000]  [<800ff726>] check_bytes_and_report+0xa2/0xfc
[    9.676000]  [<800ff826>] check_object+0xa6/0x204
[    9.676000]  [<800ff684>] check_bytes_and_report+0x0/0xfc
[    9.676000]  [<80100276>] free_to_partial_list+0x9a/0x2b8
[    9.676000]  [<800788ce>] __timer_delete_sync+0x2a/0x50
[    9.676000]  [<80078804>] __try_to_del_timer_sync+0x0/0x88
[    9.676000]  [<80078900>] timer_delete_sync+0xc/0x18
[    9.676000]  [<801004de>] __slab_free+0x4a/0x19c
[    9.676000]  [<80009264>] _dev_notice+0x0/0x5c
[    9.676000]  [<8032b626>] usb_get_configuration+0x1a2/0x11d8
[    9.676000]  [<80112d6c>] delete_object_full+0x40/0x68
[    9.676000]  [<80101c4a>] kfree+0x112/0x1a4
[    9.676000]  [<80009264>] _dev_notice+0x0/0x5c
[    9.676000]  [<8032b626>] usb_get_configuration+0x1a2/0x11d8
[    9.676000]  [<8032b626>] usb_get_configuration+0x1a2/0x11d8
[    9.676000]  [<8032b626>] usb_get_configuration+0x1a2/0x11d8
[    9.676000]  [<80009264>] _dev_notice+0x0/0x5c
[    9.676000]  [<80044eb4>] set_next_task_fair+0x190/0x350
[    9.676000]  [<800788ce>] __timer_delete_sync+0x2a/0x50
[    9.676000]  [<80078804>] __try_to_del_timer_sync+0x0/0x88
[    9.676000]  [<80078900>] timer_delete_sync+0xc/0x18
[    9.676000]  [<804931f0>] schedule_timeout+0x98/0xe4
[    9.676000]  [<80323952>] usb_new_device+0x46/0x2ac
[    9.676000]  [<80493294>] schedule_timeout_uninterruptible+0x14/0x20
[    9.676000]  [<803249d0>] hub_event+0xbf0/0xdf4
[    9.676000]  [<8025667c>] _find_next_zero_bit+0x0/0x6c
[    9.676000]  [<80323470>] hub_init_func3+0x10/0x20
[    9.676000]  [<8002f078>] process_scheduled_works+0x148/0x25c
[    9.676000]  [<8003062c>] wq_worker_sleeping+0x14/0x88
[    9.676000]  [<8002ccbe>] assign_work+0x6c/0x82
[    9.676000]  [<8002f34c>] worker_thread+0xe4/0x1a8
[    9.676000]  [<80493e18>] _raw_spin_lock_irq+0x0/0x34
[    9.676000]  [<8002cc52>] assign_work+0x0/0x82
[    9.676000]  [<8003615c>] kthread+0xdc/0x114
[    9.676000]  [<8002f268>] worker_thread+0x0/0x1a8
[    9.676000]  [<8001d45c>] do_exit+0x0/0x798
[    9.676000]  [<80010200>] ret_from_kernel_thread+0xc/0x14
[    9.676000]  [<8003f0b8>] schedule_tail+0x0/0x78
[    9.676000]  [<80036080>] kthread+0x0/0x114
[    9.676000] 
[    9.676000] ---[ end trace 0000000000000000 ]---
[    9.676000] FIX kmalloc-32: Restoring kmalloc Redzone 0x820c4229-0x820c4229=0xcc
[    9.676000] FIX kmalloc-32: Object at 0x820c4220 not freed
[   10.596000] kmemleak: Kernel memory leak detector initialized (mem pool available: 15907)
[   10.608000] kmemleak: Automatic memory scanning thread started

With the patch applied, there are no memory leaks reported during boot:

[   10.236000] kmemleak: Kernel memory leak detector initialized (mem pool available: 15907)
[   10.244000] kmemleak: Automatic memory scanning thread started

Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] usb: r8a66597-hcd: fix buffer overflow on odd-length FIFO reads

[Geert Uytterhoeven]

Hi Karl,

On Fri, 2 Oct 2026 at 23:32, Karl Mehltretter <kmehltretter@gmail.com> wrote:
> For an external R8A66597 (pdata->on_chip is false), the driver accesses
> the FIFO 16 bits at a time. It rounds an odd byte count up to the next
> word and passes that word count to ioread16_rep(), which stores both bytes
> of every word in the caller's buffer. The final word therefore writes one
> byte beyond an odd-length read, past the end of the buffer when the read
> fills it.
>
> This is visible while enumerating a USB device on an SH7785LCR. The USB
> core allocates nine bytes for the configuration descriptor header, and
> the controller driver stores ten bytes in it. SLUB reports the first
> redzone byte changing from 0xcc to 0x09 in usb_get_configuration().
> Odd-sized HID report descriptors trigger the same overwrite.
>
> Section 2.8.5 of the R8A66597 datasheet requires software to discard the
> excess byte after a 16-bit FIFO read when DTLN is odd. Read the trailing
> byte through a temporary word and copy only that byte.
>
> Fixes: 5d3043586db4 ("USB: r8a66597-hcd: host controller driver for R8A66597")
> Cc: stable@vger.kernel.org
> Reported-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> Link: https://lore.kernel.org/all/3bd32eaf159db61ed1d423e1d52a869b3689c682.camel@physik.fu-berlin.de/
> Assisted-by: LLM
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>

Thanks for your patch!

> --- a/drivers/usb/host/r8a66597.h
> +++ b/drivers/usb/host/r8a66597.h
> @@ -178,8 +178,15 @@ static inline void r8a66597_read_fifo(struct r8a66597 *r8a66597,
>                                len & 0x03);
>                 }
>         } else {
> -               len = (len + 1) / 2;
> -               ioread16_rep(fifoaddr, buf, len);
> +               count = len / 2;
> +               ioread16_rep(fifoaddr, buf, count);

Or just:

    ioread16_rep(fifoaddr, buf, len / 2);

> +
> +               if (len & 0x00000001) {

"len & 1"?

> +                       u16 tmp;
> +
> +                       ioread16_rep(fifoaddr, &tmp, 1);

Seriously: a *_rep() function for a single iteration?

> +                       memcpy((unsigned char *)buf + count * 2, &tmp, 1);

Likewise, this is a just a single byte.

> +               }

Oh, most of this was based on r8a66597_read_fifo() in
drivers/usb/host/r8a66597.h, which has to handle 1 to 3 extra bytes.

>         }
>  }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] usb: r8a66597-hcd: fix buffer overflow on odd-length FIFO reads

[Karl Mehltretter]

Hi Geert,

On Sat, Oct 03, 2026 at 02:42:48PM +0100, Geert Uytterhoeven wrote:
> > +               count = len / 2;
> > +               ioread16_rep(fifoaddr, buf, count);
> 
> Or just:
> 
>     ioread16_rep(fifoaddr, buf, len / 2);
> 
> > +
> > +               if (len & 0x00000001) {
> 
> "len & 1"?
> 

Thanks for suggesting that! I will use len / 2 and len & 1, and a
plain byte store instead of the memcpy().

> 
> Seriously: a *_rep() function for a single iteration?
> 

I'd like to keep this one. ioread16_rep() does not byte swap.
ioread16() goes through readw(), which swaps on big endian on most
architectures.

The words before the last one are read with ioread16_rep(), so reading
the last word the same way puts its first FIFO byte first in memory on
every architecture.

Thanks,
Karl

Re: [PATCH] usb: r8a66597-hcd: fix buffer overflow on odd-length FIFO reads

[Geert Uytterhoeven]

Hi Karl,

On Mon, 5 Oct 2026 at 06:25, Karl Mehltretter <kmehltretter@gmail.com> wrote:
> > Seriously: a *_rep() function for a single iteration?
>
> I'd like to keep this one. ioread16_rep() does not byte swap.
> ioread16() goes through readw(), which swaps on big endian on most
> architectures.
>
> The words before the last one are read with ioread16_rep(), so reading
> the last word the same way puts its first FIFO byte first in memory on
> every architecture.

Hmm, the version in drivers/usb/gadget/udc/r8a66597-udc.h does use
a single ioread16() or ioread32().

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] usb: r8a66597-hcd: fix buffer overflow on odd-length FIFO reads

[Karl Mehltretter]

On Mon, Oct 05, 2026 at 03:57:42PM +0100, Geert Uytterhoeven wrote:
> > The words before the last one are read with ioread16_rep(), so reading
> > the last word the same way puts its first FIFO byte first in memory on
> > every architecture.
> 
> Hmm, the version in drivers/usb/gadget/udc/r8a66597-udc.h does use
> a single ioread16() or ioread32().
> 

That version takes the low byte of the ioread16() value, which is the
first FIFO byte only when readw() swaps on big endian. On sh it does not
swap without SWAP_IO_SPACE. The boards with an external R8A66597 are all
little endian, so nobody hits that.

ata_sff_data_xfer() also uses ioread16_rep() with a count of 1 for its
trailing byte, and i3c_readl_fifo() uses readsl() that way since
d6ddd9beb1a5 ("i3c: fix big-endian FIFO transfers").

Thanks,
Karl