On Wed, Oct 04, 2017 at 12:04:06PM +0200, Takashi Iwai wrote:
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:
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.
Yes, that's why a driver should verify that the endpoints it expects are indeed present (and of the right type) already at probe.
In Andrey's fuzzing it's triggered by in a VM using the dummy_hcd driver, but this could just as well be a (malicious) physical device with unexpected descriptors.
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.
If you grep for usb_find_common_endpoints you'll find a few examples of how that function may be used (e.g. in drivers/usb/misc/usblcd.c).
The helper iterates of the endpoint descriptors of an interface alt-setting and returns a descriptor for each requested type if found. After a vetting of our current drivers I concluded that this would cover the needs of the vast majority of drivers.
So for the driver in question you'd only need to add something like:
struct usb_endpoint_descriptor *int_in, *int_out; int ret;
ret = usb_find_common_endpoints(interface->cur_altsetting, NULL, NULL, &int_in, &int_out); if (ret) { dev_err(&interface->dev, "required endpoints not found\n"); return -ENODEV; }
Then you can use int_in->bEndpointAddress etc. when initialising your URBs.
Johan