Re: [PATCH 1/3] USB: core: drop pipe-type check from new control-message helpers

[Johan Hovold <johan@kernel.org>]

On Sun, Dec 06, 2020 at 12:20:24PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Dec 04, 2020 at 04:50:17PM +0100, Johan Hovold wrote:
> > On Fri, Dec 04, 2020 at 04:14:18PM +0100, Greg Kroah-Hartman wrote:
> > > On Fri, Dec 04, 2020 at 09:51:08AM +0100, Johan Hovold wrote:
> > > > The new control-message helpers include a pipe-type check which is
> > > > almost completely redundant.
> > > > 
> > > > Control messages are generally sent to the default pipe which always
> > > > exists and is of the correct type since its endpoint representation is
> > > > created by USB core as part of enumeration for all devices.
> > > > 
> > > > There is currently only one instance of a driver in the tree which use
> > > > a control endpoint other than endpoint 0 (and it does not use the new
> > > > helpers).
> > > > 
> > > > Drivers should be testing for the existence of their resources at probe
> > > > rather than at runtime, but to catch drivers failing to do so USB core
> > > > already does a sanity check on URB submission and triggers a WARN().
> > > > Having the same sanity check done in the helper only suppresses the
> > > > warning without allowing us to find and fix the drivers.
> > > 
> > > The issue is "bad" devices.  syzbot fuzzed the USB sound drivers with
> > > stuff like this and found a bunch of problems, which is where this check
> > > originally came from.  While it is nice to "warn" people, that keeps
> > > moving forward and then the driver tries to submit an urb for this
> > > endpoint and things blow up.  Or throw more warnings, I can't remember.
> > 
> > Nothing blows up, it's just a reminder to fix the driver which I don't
> > think we should suppress.
> > 
> > I looked at the sound driver changes for this a while back it has the
> > same "problem" in that it uses a too big hammer for something that's not
> > an issue.
> 
> Then what about the syzbot issues found?  They didn't seem to be
> "caught" by any usb core changes, which is why they were added to the
> sound driver.
> 
> Or am I mis-remembering this?

The big hammer I was referring to is the commit adding the
snd_usb_pipe_sanity_check() helper to sound:

	801ebf1043ae ("ALSA: usb-audio: Sanity checks for each pipe and
	EP types")

It adds a sanity check like the one you included in the new
control-message helper to the corresponding wrappers in sounds, but also
to some individual drivers using usb_control_msg() or
usb_interrupt_msg() directly.

Those checks that were added for ep0 are completely unnecessary since
the WARN_ON in usb_submit_urb() will *never* trigger on such requests.

The checks added for endpoints other than ep0 were the ones that syzbot
could potentially hit and typically involved usb_interrupt_msg(). By
silently bailing out before submitting the URB, well you suppress that
warning, but you don't really fix the driver.
 
> > The sanity check in sound was only "needed" in cases where drivers where
> > issuing synchronous requests for endpoints other than ep0 and the
> > drivers never verified the type of the endpoint before submitting
> > thereby hitting the WARN() in usb_submit_urb().
> 
> Ok, but we still have to check for that somewhere, right?

Not for ep0, no.

For other endpoints there should be a check in probe() so that we don't
pretend to support a driver only to silently fail in some subroutine at
some later point when trying to use the device.

> > That has never been an issue for ep0 since it is created by USB core and
> > by definition is of control type (i.e. regardless of the device
> > descriptors).
> > 
> > By silently refusing to submit, we even risk breaking drivers which can
> > use either an interrupt or bulk endpoint depending on the firmware (we
> > have a few drivers supporting such devices already).
> 
> I don't understand this, sorry.

I was referring to the kind of checks added to for example the sound
drivers for endpoints other than ep0 where snd_usb_pipe_sanity_check()
was called before usb_interrupt_msg() *only* to suppress the WARN_ON()
in usb_submit_urb().

That could potentially silently break a working driver and such checks
would be better to do once at probe, rather than at every submission.

> > > So I'd like to keep this check here if at all possible, to ensure we
> > > don't have to fix those "bugs" again, it's not hurting anything here, is
> > > it?
> > 
> > But for this function which creates a control pipe it will by definition
> > never be an issue unless it is used with a control endpoint other than
> > ep0. And there are basically no such devices/drivers around; there is
> > only a single such usb_control_msg() in the entire kernel tree. (I can
> > add sanity check to its probe function.)
> > 
> > So specifically there's nothing for syzbot to trigger here, and having
> > the check in place for control transfers and ep0 is more confusing than
> > helpful.
> 
> My worry is that we will trigger the issues found by syzbot again, if
> this is removed.  If that check is also somewhere else, that's fine to
> remove these, but I'm confused as to if that is the case here or not.

I guarantee you that syzbot cannot trigger anything again from removing
the pipe-type checks from the new helpers.

Such a check is only useful for endpoints other than ep0, but then they
should preferably be done once at probe time.

Johan