On Tue, Oct 10, 2017 at 4:33 PM, Takashi Iwai tiwai@suse.de wrote:
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.
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 andreyknvl@google.com
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