[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@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
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;
+		}
 	}
 }
 
-- 
2.53.0