lkml.org 
[lkml]   [2017]   [Oct]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: usb/sound/bcd2000: warning in bcd2000_init_device
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

\
 
 \ /
  Last update: 2017-10-04 12:23    [W:0.088 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site