[PATCH 0/3] USB: tweak the new control-message helpers

[PATCH 0/3] USB: tweak the new control-message helpers

[Johan Hovold]

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 first patch drops the sanity check from the helpers; the second
removes a redundant check for short transfers in usb_control_msg_send()
which is always treated as an error; the final patch switches to using
-EREMOTEIO for short reads which is the error code already used by the
host-controller drivers for this.

Johan


Johan Hovold (3):
  USB: core: drop pipe-type check from new control-message helpers
  USB: core: drop short-transfer check from usb_control_msg_send()
  USB: core: return -EREMOTEIO on short usb_control_msg_recv()

 drivers/usb/core/message.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

-- 
2.26.2

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

[Johan Hovold]

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.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/core/message.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index c4e876050074..a04b01247117 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -204,9 +204,6 @@ int usb_control_msg_send(struct usb_device *dev, __u8 endpoint, __u8 request,
 	int ret;
 	u8 *data = NULL;
 
-	if (usb_pipe_type_check(dev, pipe))
-		return -EINVAL;
-
 	if (size) {
 		data = kmemdup(driver_data, size, memflags);
 		if (!data)
@@ -273,7 +270,7 @@ int usb_control_msg_recv(struct usb_device *dev, __u8 endpoint, __u8 request,
 	int ret;
 	u8 *data;
 
-	if (!size || !driver_data || usb_pipe_type_check(dev, pipe))
+	if (!size || !driver_data)
 		return -EINVAL;
 
 	data = kmalloc(size, memflags);
-- 
2.26.2

[PATCH 2/3] USB: core: drop short-transfer check from usb_control_msg_send()

[Johan Hovold]

A failure to send a complete control message is always an error so
there's no need to check for short transfers in usb_control_msg_send().

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/core/message.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index a04b01247117..b08de9571f7a 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -216,9 +216,8 @@ int usb_control_msg_send(struct usb_device *dev, __u8 endpoint, __u8 request,
 
 	if (ret < 0)
 		return ret;
-	if (ret == size)
-		return 0;
-	return -EINVAL;
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(usb_control_msg_send);
 
-- 
2.26.2

[PATCH 3/3] USB: core: return -EREMOTEIO on short usb_control_msg_recv()

[Johan Hovold]

Return -EREMOTEIO instead of -EINVAL on short control transfers when
using the new usb_control_msg_recv() helper.

EINVAL is used to report invalid arguments (e.g. to the helper) and
should not be used for unrelated errors.

Many driver currently return -EIO on short control transfers but since
host-controller drivers already use -EREMOTEIO for short transfers
whenever the URB_SHORT_NOT_OK flag is set, let's use that here as well.

This also allows usb_control_msg_recv() to eventually use
URB_SHORT_NOT_OK without changing the return value again.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/core/message.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index b08de9571f7a..30e9e680c74c 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -286,7 +286,7 @@ int usb_control_msg_recv(struct usb_device *dev, __u8 endpoint, __u8 request,
 		memcpy(driver_data, data, size);
 		ret = 0;
 	} else {
-		ret = -EINVAL;
+		ret = -EREMOTEIO;
 	}
 
 exit:
-- 
2.26.2

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

[Greg Kroah-Hartman]

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.

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?

thanks,

greg k-h

Re: [PATCH 2/3] USB: core: drop short-transfer check from usb_control_msg_send()

[Greg Kroah-Hartman]

On Fri, Dec 04, 2020 at 09:51:09AM +0100, Johan Hovold wrote:
> A failure to send a complete control message is always an error so
> there's no need to check for short transfers in usb_control_msg_send().
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/usb/core/message.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index a04b01247117..b08de9571f7a 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -216,9 +216,8 @@ int usb_control_msg_send(struct usb_device *dev, __u8 endpoint, __u8 request,
>  
>  	if (ret < 0)
>  		return ret;
> -	if (ret == size)
> -		return 0;
> -	return -EINVAL;
> +
> +	return 0;

Ah, this came from the read call where a short read is not an error, but
we wanted it to be.  Nice catch.

greg k-h

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

[Johan Hovold]

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.

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().

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).

> 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.

Johan

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

[Greg Kroah-Hartman]

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 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?

> 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.

> > 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.

thanks,

greg k-h

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

[Alan Stern]

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?

There _were_ some core changes made in response to syzbot reports.  For 
example, ac854131d984 ("USB: core: Fix misleading driver bug report") -- 
although I don't remember whether this bug report involved the sound 
driver -- and was also your own commit fcc2cc1f3561 ("USB: move 
snd_usb_pipe_sanity_check into the USB core").

> > 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?

Johan's point is that all the various bug reports and earlier fixes to 
the sound driver (and others) involved URBs that were for endpoints 
_other_ than ep0.  The things he wants to change are all messages sent 
to ep0.  The checks for other endpoints will remain intact.

> > 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.
> 
> > > 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.

The existing checks in usbcore will still be there.  If an error crops 
up, we will catch it.  But it's always okay to call usb_control_msg() 
for an ep0 pipe rather than usb_control_msg_{send|recv} -- we _know_ 
that the extra checks will never fail when an URB is addressed to ep0.

Alan Stern

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

[Johan Hovold]

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

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

[Greg Kroah-Hartman]

On Mon, Dec 07, 2020 at 10:46:38AM +0100, Johan Hovold wrote:
> 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.

Ok, thanks for the longer-than-should-have-been-needed explanations,
this makes sense and is now queued up.

greg k-h