linux-kernel - Re: usb/sound/bcd2000: warning in bcd2000_init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <s5h4lrf19fw.wl-tiwai@suse.de>
Date:   Wed, 04 Oct 2017 12:41:55 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Johan Hovold <johan@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Alan Stern <stern@...land.harvard.edu>,
        Andrey Konovalov <andreyknvl@...gle.com>,
        alsa-devel@...a-project.org,
        Arvind Yadav <arvind.yadav.cs@...il.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Sakamoto <o-takashi@...amocchi.jp>,
        LKML <linux-kernel@...r.kernel.org>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Kostya Serebryany <kcc@...gle.com>,
        syzkaller <syzkaller@...glegroups.com>, linux-usb@...r.kernel.org
Subject: Re: usb/sound/bcd2000: warning in bcd2000_init_device

On Wed, 04 Oct 2017 12:23:11 +0200,
Johan Hovold wrote:
> 
> 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.

OK, but in our cases, it's not about using the returned one but
checking whether it's the expected address, right?  The device is
non-compliant and that's the reason the driver takes the fixed EP.

In anyway, the check will be shortly before the URB submission because
the EP is often determined a late stage of probe, as most of errors
happened for the MIDI interface that are device-specific.


thanks,

Takashi