Re: [PATCH 2/9] ALSA: bcd2000: Add a sanity check for invalid EPs

[Takashi Iwai <tiwai@suse.de>]

On Tue, 10 Oct 2017 16:00:25 +0200,
Andrey Konovalov wrote:
> 
> On Tue, Oct 10, 2017 at 3:38 PM, Takashi Iwai <tiwai@suse.de> 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 <andreyknvl@google.com>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> 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.

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 <tiwai@suse.de>
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 <tiwai@suse.de>
---
 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