| Message ID | 20201204085110.20055-2-johan@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7fe53dcbbfbd91ad953022281adcc6cbc9dbc052 |
| Headers | show |
| Series | USB: tweak the new control-message helpers | expand |
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
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
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
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
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
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
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);
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(-)