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

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

[Karl Mehltretter]

An external R8A66597 (pdata->on_chip is not set) is read through a 16-bit
FIFO port. r8a66597_read_fifo() rounds the byte count up to whole words
and passes that word count to ioread16_rep(), which stores two bytes for
every word. For an odd byte count the last word puts one byte more into
the buffer than packet_read() asked for. When the data ends at the end of
the URB's transfer buffer, that byte is written behind the buffer.

Enumerating a device is enough to trigger it. The USB core reads the
configuration descriptor header into a 9-byte kmalloc() buffer and the
driver stores 10 bytes. An SH7785LCR board booted with slub_debug=FZPU
reports:

  [kmalloc Redzone overwritten] 0x820c4229-0x820c4229 @offset=553. First byte 0x9 instead of 0xcc
  BUG kmalloc-32 (Not tainted): Object corrupt
  Allocated in usb_get_configuration+0x12c/0x11d8 age=51 cpu=0 pid=10
  Object   820c4220: 09 02 20 00 01 01 00 80 fa 09 cc cc cc cc cc cc

Odd-sized HID report descriptors are overrun in the same way. Without
slab debugging the byte ends up in the unused tail of a kmalloc() object.
A transfer buffer that is embedded in a larger object has the byte after
it overwritten.

Section 2.8.5 of the R8A66597 datasheet tells software to discard the
excess byte of the last 16-bit FIFO read when DTLN is odd. Read only the
whole words into the buffer. Read the last word into a temporary and
store only its first byte. Use ioread16_rep() for that word as well, so
that the byte is taken in the same byte order as the words before it.

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>
Closes: https://lore.kernel.org/linux-sh/3bd32eaf159db61ed1d423e1d52a869b3689c682.camel@physik.fu-berlin.de/
Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---

v2:
- Simplify the handling of the last byte as suggested by Geert: no count
  variable, len & 1, and a plain byte store instead of a 1-byte memcpy().
- Reword the commit message and add the SLUB report from Adrian's board.
- Add Adrian's Tested-by. It was given for v1. The FIFO accesses are the
  same in v2, only the copy of the last byte changed.

v1: https://lore.kernel.org/r/20261002213225.27834-1-kmehltretter@gmail.com/

Adrian tested v1 on an SH7785LCR board. I tested v2 on a custom QEMU
model of the board with 32-bit and 29-bit kernels and slub_debug=FZPU.
Both report the overflow without the patch and not with it.

 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
@@ -179,8 +179,15 @@ static inline void r8a66597_read_fifo(struct r8a66597 *r8a66597,
 			       len & 0x03);
 		}
 	} else {
-		len = (len + 1) / 2;
-		ioread16_rep(fifoaddr, buf, len);
+		ioread16_rep(fifoaddr, buf, len / 2);
+
+		if (len & 1) {
+			u16 tmp;
+
+			/* Keep the byte order of the string read above. */
+			ioread16_rep(fifoaddr, &tmp, 1);
+			((u8 *)buf)[len - 1] = *(u8 *)&tmp;
+		}
 	}
 }
 
-- 
2.53.0

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

[John Paul Adrian Glaubitz]

Hi Karl,

On Mon, 2026-10-05 at 23:28 +0200, Karl Mehltretter wrote:
> An external R8A66597 (pdata->on_chip is not set) is read through a 16-bit
> FIFO port. r8a66597_read_fifo() rounds the byte count up to whole words
> and passes that word count to ioread16_rep(), which stores two bytes for
> every word. For an odd byte count the last word puts one byte more into
> the buffer than packet_read() asked for. When the data ends at the end of
> the URB's transfer buffer, that byte is written behind the buffer.
> 
> Enumerating a device is enough to trigger it. The USB core reads the
> configuration descriptor header into a 9-byte kmalloc() buffer and the
> driver stores 10 bytes. An SH7785LCR board booted with slub_debug=FZPU
> reports:
> 
>   [kmalloc Redzone overwritten] 0x820c4229-0x820c4229 @offset=553. First byte 0x9 instead of 0xcc
>   BUG kmalloc-32 (Not tainted): Object corrupt
>   Allocated in usb_get_configuration+0x12c/0x11d8 age=51 cpu=0 pid=10
>   Object   820c4220: 09 02 20 00 01 01 00 80 fa 09 cc cc cc cc cc cc
> 
> Odd-sized HID report descriptors are overrun in the same way. Without
> slab debugging the byte ends up in the unused tail of a kmalloc() object.
> A transfer buffer that is embedded in a larger object has the byte after
> it overwritten.
> 
> Section 2.8.5 of the R8A66597 datasheet tells software to discard the
> excess byte of the last 16-bit FIFO read when DTLN is odd. Read only the
> whole words into the buffer. Read the last word into a temporary and
> store only its first byte. Use ioread16_rep() for that word as well, so
> that the byte is taken in the same byte order as the words before it.
> 
> 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>
> Closes: https://lore.kernel.org/linux-sh/3bd32eaf159db61ed1d423e1d52a869b3689c682.camel@physik.fu-berlin.de/
> Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> Assisted-by: LLM

I was wondering: Would it maybe be better if you disclosed what LLM you used?
I have seen that many others have done so and I'm not sure if it's actually
prepared to have the model disclosed.

Adrian

> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
> ---
> 
> v2:
> - Simplify the handling of the last byte as suggested by Geert: no count
>   variable, len & 1, and a plain byte store instead of a 1-byte memcpy().
> - Reword the commit message and add the SLUB report from Adrian's board.
> - Add Adrian's Tested-by. It was given for v1. The FIFO accesses are the
>   same in v2, only the copy of the last byte changed.
> 
> v1: https://lore.kernel.org/r/20261002213225.27834-1-kmehltretter@gmail.com/
> 
> Adrian tested v1 on an SH7785LCR board. I tested v2 on a custom QEMU
> model of the board with 32-bit and 29-bit kernels and slub_debug=FZPU.
> Both report the overflow without the patch and not with it.
> 
>  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
> @@ -179,8 +179,15 @@ static inline void r8a66597_read_fifo(struct r8a66597 *r8a66597,
>  			       len & 0x03);
>  		}
>  	} else {
> -		len = (len + 1) / 2;
> -		ioread16_rep(fifoaddr, buf, len);
> +		ioread16_rep(fifoaddr, buf, len / 2);
> +
> +		if (len & 1) {
> +			u16 tmp;
> +
> +			/* Keep the byte order of the string read above. */
> +			ioread16_rep(fifoaddr, &tmp, 1);
> +			((u8 *)buf)[len - 1] = *(u8 *)&tmp;
> +		}
>  	}
>  }
>  

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

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

[Karl Mehltretter]

On Wed, Oct 07, 2026 at 11:12:46AM +0100, John Paul Adrian Glaubitz wrote:
> > 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>
> > Closes: https://lore.kernel.org/linux-sh/3bd32eaf159db61ed1d423e1d52a869b3689c682.camel@physik.fu-berlin.de/
> > Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> > Assisted-by: LLM
> 
> I was wondering: Would it maybe be better if you disclosed what LLM you used?
> I have seen that many others have done so and I'm not sure if it's actually
> prepared to have the model disclosed.
> 

The generic Assisted-by: LLM format is fairly new. Christian Brauner
proposed simplifying the attribution in July [1], and it is now the
documented format [2]. I expect we may see more patches using it as
people adopt the change.

I prefer the generic Assisted-by: LLM tag. It acknowledges the
assistance without advertising a particular vendor. 

A model name alone also leaves out the harness and settings, and I
sometimes use different models for implementation and review. Tracking
and listing all of that would make the attribution rather cumbersome.

[1] https://lore.kernel.org/r/20260701-work-coding-assistants-v1-1-a20a94d1d606@kernel.org/
[2] https://docs.kernel.org/process/coding-assistants.html#attribution

Thanks,
Karl

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

[John Paul Adrian Glaubitz]

Hi Karl,

On Thu, 2026-10-08 at 08:18 +0200, Karl Mehltretter wrote:
> On Wed, Oct 07, 2026 at 11:12:46AM +0100, John Paul Adrian Glaubitz wrote:
> > > 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>
> > > Closes: https://lore.kernel.org/linux-sh/3bd32eaf159db61ed1d423e1d52a869b3689c682.camel@physik.fu-berlin.de/
> > > Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> > > Assisted-by: LLM
> > 
> > I was wondering: Would it maybe be better if you disclosed what LLM you used?
> > I have seen that many others have done so and I'm not sure if it's actually
> > prepared to have the model disclosed.
> > 
> 
> The generic Assisted-by: LLM format is fairly new. Christian Brauner
> proposed simplifying the attribution in July [1], and it is now the
> documented format [2]. I expect we may see more patches using it as
> people adopt the change.
> 
> I prefer the generic Assisted-by: LLM tag. It acknowledges the
> assistance without advertising a particular vendor. 
> 
> A model name alone also leaves out the harness and settings, and I
> sometimes use different models for implementation and review. Tracking
> and listing all of that would make the attribution rather cumbersome.
> 
> [1] https://lore.kernel.org/r/20260701-work-coding-assistants-v1-1-a20a94d1d606@kernel.org/
> [2] https://docs.kernel.org/process/coding-assistants.html#attribution

Thanks a lot for the pointer! I will read this thread and unless I find
anything speaking against the proposal, I'll adopt it and continue with
the reviews.

Adrian

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