On Wed, 04 Oct 2017 11:24:42 +0200, Johan Hovold wrote:
On Wed, Oct 04, 2017 at 08:10:59AM +0200, Takashi Iwai wrote:
On Tue, 03 Oct 2017 19:42:21 +0200, Greg Kroah-Hartman wrote:
On Tue, Oct 03, 2017 at 12:50:08PM -0400, Alan Stern wrote:
On Tue, 3 Oct 2017, Takashi Iwai wrote:
It's a dev_WARN because it indicates a potentially serious error in the driver: The driver has submitted an interrupt URB to a bulk endpoint. That may not sound bad, but the same check gets triggered if a driver submits a bulk URB to an isochronous endpoint, or any other invalid combination.
Most likely the explanation here is that the driver doesn't bother to check the endpoint type because it expects the endpoint will always be interrupt. But that is not a safe strategy. USB devices and their firmware should not be trusted unnecessarily.
The best fix is, like you said, to add a sanity check in the caller.
OK, but then do we have some handy helper for the check? As other bug reports by syzkaller suggest, there are a few other drivers that do the same, submitting a urb with naive assumption of the fixed EP for specific devices. In the end we'll need to put the very same checks there in multiple places.
Perhaps we could add a helper routine that would take a list of expected endpoint types and check that the actual endpoints match the types. But of course, all the drivers you're talking about would have to add a call to this helper routine.
We have almost this type of function, usb_find_common_endpoints(), what's wrong with using that? Johan has already swept the tree and added a lot of these checks, odds are no one looked at the sound/ subdir...
Yeah, I only swept the tree for instances were a missing endpoint could lead to a NULL-deref. This is not the case here were the endpoint addresses are hardcoded in the driver.
I also never got around to applying the new helper outside of drivers/usb.
Well, what I had in my mind is just a snippet from usb_submit_urb(), something like:
bool usb_sanity_check_urb_pipe(struct urb *urb) { struct usb_host_endpoint *ep; int xfertype; static const int pipetypes[4] = { PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT };
ep = usb_pipe_endpoint(urb->dev, urb->pipe); xfertype = usb_endpoint_type(&ep->desc); return usb_pipetype(urb->pipe) != pipetypes[xfertype]; }
And calling this before usb_submit_urb() in each place that assigns the fixed EP as device-specific quirks. Does it make sense?
Not really. Your driver should not even bind to an interface which lacks the expected endpoints (rather than check this at a potentially later point in time when URBs are submitted).
The endpoint may exist but it may be invalid, as the problem is triggered by a VM. It doesn't parse but tries a fixed EP as it's no compliant device.
The new helper which Greg mentioned would allow this to implemented with just a few lines of code. Just add it to bcd2000_init_midi() or similar.
Could you give an example? Then I can ask Andrey whether such a call really addresses the issue.
thanks,
Takashi