From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrey Konovalov Subject: Re: [PATCH 2/9] ALSA: bcd2000: Add a sanity check for invalid EPs Date: Tue, 10 Oct 2017 20:20:19 +0200 Message-ID: References: <20171010133819.10567-1-tiwai@suse.de> <20171010133819.10567-3-tiwai@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-oi0-f47.google.com (mail-oi0-f47.google.com [209.85.218.47]) by alsa0.perex.cz (Postfix) with ESMTP id 9082A2673F2 for ; Tue, 10 Oct 2017 20:20:21 +0200 (CEST) Received: by mail-oi0-f47.google.com with SMTP id q4so29111028oic.7 for ; Tue, 10 Oct 2017 11:20:21 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Takashi Iwai Cc: alsa-devel@alsa-project.org, USB list List-Id: alsa-devel@alsa-project.org On Tue, Oct 10, 2017 at 4:33 PM, Takashi Iwai wrote: > On Tue, 10 Oct 2017 16:00:25 +0200, > Andrey Konovalov wrote: >> >> On Tue, Oct 10, 2017 at 3:38 PM, Takashi Iwai wrote: >> > As syzkaller spotted, currently bcd2000 driver submits a URB with the >> > fixed EP without checking whether it's actually available, which may >> > result in a kernel warning like: >> > usb 1-1: BOGUS urb xfer, pipe 1 != type 3 >> > ------------[ cut here ]------------ >> > WARNING: CPU: 0 PID: 1846 at drivers/usb/core/urb.c:449 >> > usb_submit_urb+0xf8a/0x11d0 >> > Modules linked in: >> > CPU: 0 PID: 1846 Comm: kworker/0:2 Not tainted >> > 4.14.0-rc2-42613-g1488251d1a98 #238 >> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 >> > Workqueue: usb_hub_wq hub_event >> > Call Trace: >> > bcd2000_init_device sound/usb/bcd2000/bcd2000.c:289 >> > bcd2000_init_midi sound/usb/bcd2000/bcd2000.c:345 >> > bcd2000_probe+0xe64/0x19e0 sound/usb/bcd2000/bcd2000.c:406 >> > usb_probe_interface+0x35d/0x8e0 drivers/usb/core/driver.c:361 >> > .... >> > >> > This patch adds a sanity check of validity of EPs at the device >> > initialization phase for avoiding the call with an invalid EP. >> > >> > Reported-by: Andrey Konovalov >> > Signed-off-by: Takashi Iwai >> >> Hi Takashi, >> >> I've applied patches #1 and #2 and for some reason get this when I try >> to build the kernel: >> >> LD vmlinux.o >> MODPOST vmlinux.o >> sound/usb/bcd2000/bcd2000.o: In function `bcd2000_init_midi': >> .../sound/usb/bcd2000/bcd2000.c:346: undefined reference to >> `usb_urb_ep_type_check' >> .../sound/usb/bcd2000/bcd2000.c:347: undefined reference to >> `usb_urb_ep_type_check' >> make: *** [vmlinux] Error 1 >> >> What could be wrong? > > Mea culpa, I generated patches from the wrong branch. > Luckily only the first patch was wrong, the function name was > misspelled. Ah, I thought so and even intentionally checked for a typo in the function name, but somehow still missed that :) I've run my reproducers with your patches applied, all the warnings are gone. Thanks! Tested-by: Andrey Konovalov > > Below is the right patch for patch 1, which already includes Greg's > suggestions. I'm going to send a v2 series in anyway later, so just > putting this one below. > > Sorry for the inconvenience! > > > Takashi > > -- 8< -- > From: Takashi Iwai > Subject: [PATCH 1/9] usb: core: Add a helper function to check the validity of EP type in URB > > This patch adds a new helper function to perform a sanity check of the > given URB to see whether it contains a valid endpoint. It's a light- > weight version of what usb_submit_urb() does, but without the kernel > warning followed by the stack trace, just returns an error code. > > Especially for a driver that doesn't parse the descriptor but fills > the URB with the fixed endpoint (e.g. some quirks for non-compliant > devices), this kind of check is preferable at the probe phase before > actually submitting the urb. > > Signed-off-by: Takashi Iwai > --- > drivers/usb/core/urb.c | 30 ++++++++++++++++++++++++++---- > include/linux/usb.h | 2 ++ > 2 files changed, 28 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c > index 47903d510955..8b800e34407b 100644 > --- a/drivers/usb/core/urb.c > +++ b/drivers/usb/core/urb.c > @@ -187,6 +187,31 @@ EXPORT_SYMBOL_GPL(usb_unanchor_urb); > > /*-------------------------------------------------------------------*/ > > +static const int pipetypes[4] = { > + PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT > +}; > + > +/** > + * usb_urb_ep_type_check - sanity check of endpoint in the given urb > + * @urb: urb to be checked > + * > + * This performs a light-weight sanity check for the endpoint in the > + * given urb. It returns 0 if the urb contains a valid endpoint, otherwise > + * a negative error code. > + */ > +int usb_urb_ep_type_check(const struct urb *urb) > +{ > + const struct usb_host_endpoint *ep; > + > + ep = usb_pipe_endpoint(urb->dev, urb->pipe); > + if (!ep) > + return -EINVAL; > + if (usb_pipetype(urb->pipe) != pipetypes[usb_endpoint_type(&ep->desc)]) > + return -EINVAL; > + return 0; > +} > +EXPORT_SYMBOL_GPL(usb_urb_ep_type_check); > + > /** > * usb_submit_urb - issue an asynchronous transfer request for an endpoint > * @urb: pointer to the urb describing the request > @@ -326,9 +351,6 @@ EXPORT_SYMBOL_GPL(usb_unanchor_urb); > */ > int usb_submit_urb(struct urb *urb, gfp_t mem_flags) > { > - static int pipetypes[4] = { > - PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT > - }; > int xfertype, max; > struct usb_device *dev; > struct usb_host_endpoint *ep; > @@ -444,7 +466,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags) > */ > > /* Check that the pipe's type matches the endpoint's type */ > - if (usb_pipetype(urb->pipe) != pipetypes[xfertype]) > + if (usb_urb_ep_type_check(urb)) > dev_WARN(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n", > usb_pipetype(urb->pipe), pipetypes[xfertype]); > > diff --git a/include/linux/usb.h b/include/linux/usb.h > index cb9fbd54386e..2b861804fffa 100644 > --- a/include/linux/usb.h > +++ b/include/linux/usb.h > @@ -1728,6 +1728,8 @@ static inline int usb_urb_dir_out(struct urb *urb) > return (urb->transfer_flags & URB_DIR_MASK) == URB_DIR_OUT; > } > > +int usb_urb_ep_type_check(const struct urb *urb); > + > void *usb_alloc_coherent(struct usb_device *dev, size_t size, > gfp_t mem_flags, dma_addr_t *dma); > void usb_free_coherent(struct usb_device *dev, size_t size, > -- > 2.14.2 >