From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751421AbdJDKXP (ORCPT ); Wed, 4 Oct 2017 06:23:15 -0400 Received: from mail-lf0-f68.google.com ([209.85.215.68]:49977 "EHLO mail-lf0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751223AbdJDKXN (ORCPT ); Wed, 4 Oct 2017 06:23:13 -0400 X-Google-Smtp-Source: AOwi7QDycDbRYL8Lrk9OYsP3jniucZv7j63wiAIKtVKCCDEXf0sCc2FVpSn+clue5r+HPgty7h+PBw== Date: Wed, 4 Oct 2017 12:23:11 +0200 From: Johan Hovold To: Takashi Iwai Cc: Johan Hovold , Greg Kroah-Hartman , Alan Stern , Andrey Konovalov , alsa-devel@alsa-project.org, Arvind Yadav , Jaroslav Kysela , Takashi Sakamoto , LKML , Dmitry Vyukov , Kostya Serebryany , syzkaller , linux-usb@vger.kernel.org Subject: Re: usb/sound/bcd2000: warning in bcd2000_init_device Message-ID: <20171004102311.GG3404@localhost> References: <20171003174221.GA13006@kroah.com> <20171004092442.GF3404@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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