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@xxxxxxxxxxxxxxx
> Reported-by: John Paul Adrian Glaubitz <glaubitz@xxxxxxxxxxxxxxxxxxx>
> Closes: https://lore.kernel.org/linux-sh/3bd32eaf159db61ed1d423e1d52a869b3689c682.camel@xxxxxxxxxxxxxxxxxxx/
> Tested-by: John Paul Adrian Glaubitz <glaubitz@xxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxx>
> ---
> 
> 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@xxxxxxxxx/
> 
> 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