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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
> Reported-by: John Paul Adrian Glaubitz <glaubitz@xxxxxxxxxxxxxxxxxxx>
> Link: https://lore.kernel.org/all/3bd32eaf159db61ed1d423e1d52a869b3689c682.camel@xxxxxxxxxxxxxxxxxxx/
> Assisted-by: LLM
> Signed-off-by: Karl Mehltretter <kmehltretter@xxxxxxxxx>

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@xxxxxxxxxxxxxx

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