Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption
@ 2019-08-14 15:59 Jonathan Bell
  2019-08-15 10:55 ` Oliver Neukum
  2019-08-15 12:51 ` Lars Melin
  0 siblings, 2 replies; 9+ messages in thread
From: Jonathan Bell @ 2019-08-14 15:59 UTC (permalink / raw)
  To: linux-usb; +Cc: Minas Harutyunyan

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption
  2019-08-14 15:59 dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption Jonathan Bell
@ 2019-08-15 10:55 ` Oliver Neukum
  2019-08-15 11:41   ` Jonathan Bell
  2019-08-15 12:51 ` Lars Melin
  1 sibling, 1 reply; 9+ messages in thread
From: Oliver Neukum @ 2019-08-15 10:55 UTC (permalink / raw)
  To: Jonathan Bell, linux-usb; +Cc: Minas Harutyunyan

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption
  2019-08-15 10:55 ` Oliver Neukum
@ 2019-08-15 11:41   ` Jonathan Bell
  2019-08-15 14:37     ` Alan Stern
  2019-08-15 14:52     ` Oliver Neukum
  0 siblings, 2 replies; 9+ messages in thread
From: Jonathan Bell @ 2019-08-15 11:41 UTC (permalink / raw)
  To: Oliver Neukum; +Cc: linux-usb, Minas Harutyunyan

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption
  2019-08-14 15:59 dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption Jonathan Bell
  2019-08-15 10:55 ` Oliver Neukum
@ 2019-08-15 12:51 ` Lars Melin
  2019-08-15 12:54   ` Jonathan Bell
  1 sibling, 1 reply; 9+ messages in thread
From: Lars Melin @ 2019-08-15 12:51 UTC (permalink / raw)
  To: Jonathan Bell, linux-usb; +Cc: Minas Harutyunyan

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption
  2019-08-15 12:51 ` Lars Melin
@ 2019-08-15 12:54   ` Jonathan Bell
  0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Bell @ 2019-08-15 12:54 UTC (permalink / raw)
  To: Lars Melin; +Cc: linux-usb, Minas Harutyunyan

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption
  2019-08-15 11:41   ` Jonathan Bell
@ 2019-08-15 14:37     ` Alan Stern
  2019-08-15 14:52     ` Oliver Neukum
  1 sibling, 0 replies; 9+ messages in thread
From: Alan Stern @ 2019-08-15 14:37 UTC (permalink / raw)
  To: Jonathan Bell; +Cc: Oliver Neukum, linux-usb, Minas Harutyunyan

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption
  2019-08-15 11:41   ` Jonathan Bell
  2019-08-15 14:37     ` Alan Stern
@ 2019-08-15 14:52     ` Oliver Neukum
  2019-08-16 22:18       ` Jonathan Bell
  1 sibling, 1 reply; 9+ messages in thread
From: Oliver Neukum @ 2019-08-15 14:52 UTC (permalink / raw)
  To: Jonathan Bell; +Cc: Minas Harutyunyan, linux-usb

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption
  2019-08-15 14:52     ` Oliver Neukum
@ 2019-08-16 22:18       ` Jonathan Bell
  2019-08-19 11:01         ` Oliver Neukum
  0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Bell @ 2019-08-16 22:18 UTC (permalink / raw)
  To: Oliver Neukum; +Cc: Minas Harutyunyan, linux-usb

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?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption
  2019-08-16 22:18       ` Jonathan Bell
@ 2019-08-19 11:01         ` Oliver Neukum
  0 siblings, 0 replies; 9+ messages in thread
From: Oliver Neukum @ 2019-08-19 11:01 UTC (permalink / raw)
  To: Jonathan Bell; +Cc: Minas Harutyunyan, linux-usb

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-14 15:59 dwc2 / Raspberry Pi - hardware bug for small transfers results in memory corruption Jonathan Bell
2019-08-15 10:55 ` Oliver Neukum
2019-08-15 11:41   ` Jonathan Bell
2019-08-15 14:37     ` Alan Stern
2019-08-15 14:52     ` Oliver Neukum
2019-08-16 22:18       ` Jonathan Bell
2019-08-19 11:01         ` Oliver Neukum
2019-08-15 12:51 ` Lars Melin
2019-08-15 12:54   ` Jonathan Bell

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git