dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption

dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption

[Jonathan Bell]

As reported by one of our users here:
https://github.com/raspberrypi/linux/issues/3148

There is a bug when the dwc2 core receives USB data packets that are
between 1 and 4 bytes in length - 4 bytes are always written to memory
where the non-packet bytes are garbage.

This is easily reproducible by
- Plugging a UVC-compliant webcam into a Raspberry Pi gen 1, 2 or 3 product
- Running "v4l2-ctl -d 0 --all"

The camera's control ranges (brightness/contrast etc) are all queried
by 1- or 2-byte control IN transfers. As 4 bytes get written to the
URB's buffer, this results in the uvcvideo data struct containing the
control information getting corrupted like so:

contrast 0x00980901 (int)    : min=0 max=64 step=1 default=57343 value=32
saturation 0x00980902 (int)    : min=0 max=128 step=1 default=57343 value=105
hue 0x00980903 (int)    : min=-40 max=40 step=1 default=-8193 value=0
white_balance_temperature_auto 0x0098090c (bool)   : default=1 value=1
gamma 0x00980910 (int)    : min=72 max=500 step=1 default=57343 value=100
[etc]

We've implemented a downstream fix for dwc_otg[1] that just forces use
of the dword alignment buffer mechanism (aka DMA bounce buffer), but
dwc2 only uses a bounce buffer for split-IN transfers.

I have two questions:
1) Does this bug occur on non-Pi hardware?
2) What's the easiest way to patch this for dwc2?

[1] https://github.com/raspberrypi/linux/commit/c0e4ca17457d6669a263e86a88f0036875fc019e

Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption

[Oliver Neukum]

Am Mittwoch, den 14.08.2019, 16:59 +0100 schrieb Jonathan Bell:
> As reported by one of our users here:
> https://github.com/raspberrypi/linux/issues/3148
> 
> There is a bug when the dwc2 core receives USB data packets that are
> between 1 and 4 bytes in length - 4 bytes are always written to memory
> where the non-packet bytes are garbage.

Hi,

in which function does that happen? If your buffer cannot handle 4
bytes I cannot see how it copes with teh DMA rules.

	Regards
		Oliver

Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption

[Jonathan Bell]

On Thu, Aug 15, 2019 at 11:55 AM Oliver Neukum <oneukum@suse.com> wrote:
>
> Am Mittwoch, den 14.08.2019, 16:59 +0100 schrieb Jonathan Bell:
> > As reported by one of our users here:
> > https://github.com/raspberrypi/linux/issues/3148
> >
> > There is a bug when the dwc2 core receives USB data packets that are
> > between 1 and 4 bytes in length - 4 bytes are always written to memory
> > where the non-packet bytes are garbage.
>
> Hi,
>
> in which function does that happen? If your buffer cannot handle 4
> bytes I cannot see how it copes with teh DMA rules.
>
In drivers/media/usb/uvc/uvc_ctrl.c:uvc_ctrl_populate_cache() and friends.

The UVC driver passes in offsets into a struct uvc_control as the
"buffer" that usb_control_msg() fills.

Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption

[Lars Melin]

On 8/14/2019 22:59, Jonathan Bell wrote:
> There is a bug when the dwc2 core receives USB data packets that are
> between 1 and 4 bytes in length - 4 bytes are always written to memory
> where the non-packet bytes are garbage.

Which host controller driver, dwc2 or the out-of-tree dwc_otg driver?

Thanks
Lars

Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption

[Jonathan Bell]

On Thu, Aug 15, 2019 at 1:51 PM Lars Melin <larsm17@gmail.com> wrote:
>
> On 8/14/2019 22:59, Jonathan Bell wrote:
> > There is a bug when the dwc2 core receives USB data packets that are
> > between 1 and 4 bytes in length - 4 bytes are always written to memory
> > where the non-packet bytes are garbage.
>
> Which host controller driver, dwc2 or the out-of-tree dwc_otg driver?
>
> Thanks
> Lars

The bug was present when using either the out-of-tree dwc_otg or
upstream dwc2 driver on Raspberry Pi.

Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption

[Alan Stern]

On Thu, 15 Aug 2019, Jonathan Bell wrote:

> On Thu, Aug 15, 2019 at 11:55 AM Oliver Neukum <oneukum@suse.com> wrote:
> >
> > Am Mittwoch, den 14.08.2019, 16:59 +0100 schrieb Jonathan Bell:
> > > As reported by one of our users here:
> > > https://github.com/raspberrypi/linux/issues/3148
> > >
> > > There is a bug when the dwc2 core receives USB data packets that are
> > > between 1 and 4 bytes in length - 4 bytes are always written to memory
> > > where the non-packet bytes are garbage.
> >
> > Hi,
> >
> > in which function does that happen? If your buffer cannot handle 4
> > bytes I cannot see how it copes with teh DMA rules.
> >
> In drivers/media/usb/uvc/uvc_ctrl.c:uvc_ctrl_populate_cache() and friends.
> 
> The UVC driver passes in offsets into a struct uvc_control as the
> "buffer" that usb_control_msg() fills.

This sounds like a violation of the DMA rules.  A buffer passed to
usb_control_msg() must be allocated by kmalloc or equivalent, and it
must not share a cache line with any other data values.  Something in
the middle of a larger struct is definitely not kosher.

Alan Stern

Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption

[Oliver Neukum]

Am Donnerstag, den 15.08.2019, 12:41 +0100 schrieb Jonathan Bell:
> On Thu, Aug 15, 2019 at 11:55 AM Oliver Neukum <oneukum@suse.com> wrote:
> > 
> > Am Mittwoch, den 14.08.2019, 16:59 +0100 schrieb Jonathan Bell:
> > > As reported by one of our users here:
> > > https://github.com/raspberrypi/linux/issues/3148
> > > 
> > > There is a bug when the dwc2 core receives USB data packets that are
> > > between 1 and 4 bytes in length - 4 bytes are always written to memory
> > > where the non-packet bytes are garbage.
> > 
> > Hi,
> > 
> > in which function does that happen? If your buffer cannot handle 4
> > bytes I cannot see how it copes with teh DMA rules.
> > 
> 
> In drivers/media/usb/uvc/uvc_ctrl.c:uvc_ctrl_populate_cache() and friends.

OK, I see.

> The UVC driver passes in offsets into a struct uvc_control as the
> "buffer" that usb_control_msg() fills.

Not quite that bad. It passes a pointer into the middle of a buffer
used at different offsets for the transfer. This is technically allowed
as long as you never touch the buffer while a transfer is ongoing.

That is an accident waiting to happen. Please make a patch using
a bounce buffer allocated with knalloc() in
drivers/media/usb/uvc/uvc_ctrl.c:uvc_ctrl_populate_cache() and friends.

	Regards
		Oliver

Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption

[Jonathan Bell]

On Thu, Aug 15, 2019 at 3:52 PM Oliver Neukum <oneukum@suse.com> wrote:
> > The UVC driver passes in offsets into a struct uvc_control as the
> > "buffer" that usb_control_msg() fills.
>
> Not quite that bad. It passes a pointer into the middle of a buffer
> used at different offsets for the transfer. This is technically allowed
> as long as you never touch the buffer while a transfer is ongoing.
>
> That is an accident waiting to happen. Please make a patch using
> a bounce buffer allocated with knalloc() in
> drivers/media/usb/uvc/uvc_ctrl.c:uvc_ctrl_populate_cache() and friends.

A patch to uvcvideo will not fix the underlying bug with the host
controller hardware. There are hundreds of device drivers of varying
vintages that potentially react badly to having a rogue host
controller DMA engine writing more bytes than were reported by the
controller's interrupt status register.

So my original two questions still need answering:
1) Does the symptom seen with v4l2-ctl exist on other platforms using
dwc2 (which implies that this is not a bug specific to Raspberry Pi)
2) How do we harden upstream dwc2 against a broken controller DMA?

Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption

[Oliver Neukum]

Am Freitag, den 16.08.2019, 23:18 +0100 schrieb Jonathan Bell:
> On Thu, Aug 15, 2019 at 3:52 PM Oliver Neukum <oneukum@suse.com> wrote:

> > That is an accident waiting to happen. Please make a patch using
> > a bounce buffer allocated with knalloc() in
> > drivers/media/usb/uvc/uvc_ctrl.c:uvc_ctrl_populate_cache() and friends.
> 
> A patch to uvcvideo will not fix the underlying bug with the host
> controller hardware.

Absolutely correct.

>  There are hundreds of device drivers of varying
> vintages that potentially react badly to having a rogue host
> controller DMA engine writing more bytes than were reported by the
> controller's interrupt status register.

Then these drivers are likely to be buggy. Not guaranteed to,
it is possible to write a driver which is correct and still would
react badly to that, but it is difficult.

> So my original two questions still need answering:
> 1) Does the symptom seen with v4l2-ctl exist on other platforms using
> dwc2 (which implies that this is not a bug specific to Raspberry Pi)
> 2) How do we harden upstream dwc2 against a broken controller DMA?

Unknown and very hard to find out, because you are almost always in a
situation where you have a full cache line, which is larger than 4
bytes.

You must flush all cache lines your buffer is part of. You must
not touch them until DMA is complete. That is easiest to achieve
if you just kmalloc() each buffer separately. Using two parts
of a buffer for subsequent DMA is within the rules, but not worth
the trouble.
Using a bounce buffer in the dwc2 driver is likely not worth
the trouble, as you wouldn't get away with a single buffer and
dynamic allocation would suck (it would have to be atomic).

	Regards
		Oliver