[alsa-devel] [PATCH 0/9] sound: Add sanity checks for invalid EPs
Hi,
this is a patchset to cover the codes that may submit URBs containing invalid EPs without validation, which result in the kernel warning from the USB core. The first patch adds a new helper for simplifying the EP check, and the rest applies it at needed places.
USB devs: could you check the first patch? I tried usb_find_common_endpoints() and friends at first, but it made the code more complex in the end, because we're dealing with the fixed endpoints and the validation is required for them.
The original issues were spotted by syzkaller, and I put a few others for covering more similar cases.
thanks,
Takashi
===
Takashi Iwai (9): usb: core: Add a helper function to check the validity of EP type in URB ALSA: bcd2000: Add a sanity check for invalid EPs ALSA: caiaq: Add a sanity check for invalid EPs ALSA: line6: Add a sanity check for invalid EPs ALSA: usb-audio: Add sanity checks for invalid EPs ALSA: usx2y: Add sanity checks for invalid EPs ALSA: hiface: Add sanity checks for invalid EPs ALSA: caiaq: Add yet more sanity checks for invalid EPs ALSA: line6: Add yet more sanity checks for invalid EPs
drivers/usb/core/urb.c | 28 +++++++++++++++++++++++++--- include/linux/usb.h | 2 ++ sound/usb/bcd2000/bcd2000.c | 7 +++++++ sound/usb/caiaq/device.c | 7 +++++++ sound/usb/caiaq/input.c | 9 +++++++++ sound/usb/hiface/pcm.c | 9 +++++++-- sound/usb/line6/driver.c | 30 ++++++++++++++++++++++-------- sound/usb/line6/midi.c | 17 +++++++++++------ sound/usb/midi.c | 38 ++++++++++++++++++++++++++++++-------- sound/usb/usx2y/usbusx2y.c | 8 ++++++++ sound/usb/usx2y/usbusx2yaudio.c | 3 +++ 11 files changed, 131 insertions(+), 27 deletions(-)
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 | 28 +++++++++++++++++++++++++--- include/linux/usb.h | 2 ++ 2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c index 47903d510955..580dfaec8af7 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_check_type - 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_check_type(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_check_type); + /** * 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; 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,
On Tue, Oct 10, 2017 at 03:38:11PM +0200, Takashi Iwai wrote:
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 | 28 +++++++++++++++++++++++++--- include/linux/usb.h | 2 ++ 2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c index 47903d510955..580dfaec8af7 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_check_type - 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_check_type(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_check_type);
/**
- 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;
Can you also call usb_urb_ep_check_type() in usb_submit_urb()?
thanks,
greg k-h
On Tue, 10 Oct 2017 15:53:49 +0200, Greg KH wrote:
On Tue, Oct 10, 2017 at 03:38:11PM +0200, Takashi Iwai wrote:
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 | 28 +++++++++++++++++++++++++--- include/linux/usb.h | 2 ++ 2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c index 47903d510955..580dfaec8af7 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_check_type - 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_check_type(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_check_type);
/**
- 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;
Can you also call usb_urb_ep_check_type() in usb_submit_urb()?
OK, will do that in v2 patch.
thanks,
Takashi
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 --- sound/usb/bcd2000/bcd2000.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/sound/usb/bcd2000/bcd2000.c b/sound/usb/bcd2000/bcd2000.c index 7371e5b06035..a6408209d7f1 100644 --- a/sound/usb/bcd2000/bcd2000.c +++ b/sound/usb/bcd2000/bcd2000.c @@ -342,6 +342,13 @@ static int bcd2000_init_midi(struct bcd2000 *bcd2k) bcd2k->midi_out_buf, BUFSIZE, bcd2000_output_complete, bcd2k, 1);
+ /* sanity checks of EPs before actually submitting */ + if (usb_urb_ep_type_check(bcd2k->midi_in_urb) || + usb_urb_ep_type_check(bcd2k->midi_out_urb)) { + dev_err(&bcd2k->dev->dev, "invalid MIDI EP\n"); + return -EINVAL; + } + bcd2000_init_device(bcd2k);
return 0;
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?
Thanks!
sound/usb/bcd2000/bcd2000.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/sound/usb/bcd2000/bcd2000.c b/sound/usb/bcd2000/bcd2000.c index 7371e5b06035..a6408209d7f1 100644 --- a/sound/usb/bcd2000/bcd2000.c +++ b/sound/usb/bcd2000/bcd2000.c @@ -342,6 +342,13 @@ static int bcd2000_init_midi(struct bcd2000 *bcd2k) bcd2k->midi_out_buf, BUFSIZE, bcd2000_output_complete, bcd2k, 1);
/* sanity checks of EPs before actually submitting */if (usb_urb_ep_type_check(bcd2k->midi_in_urb) ||usb_urb_ep_type_check(bcd2k->midi_out_urb)) {dev_err(&bcd2k->dev->dev, "invalid MIDI EP\n");return -EINVAL;}bcd2000_init_device(bcd2k); return 0;-- 2.14.2
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,
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
As syzkaller spotted, currently caiaq 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 3 != type 1 ------------[ cut here ]------------ WARNING: CPU: 1 PID: 1150 at drivers/usb/core/urb.c:449 usb_submit_urb+0xf8a/0x11d0 Modules linked in: CPU: 1 PID: 1150 Comm: kworker/1:1 Not tainted 4.14.0-rc2-42660-g24b7bd59eec0 #277 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 Workqueue: usb_hub_wq hub_event Call Trace: init_card sound/usb/caiaq/device.c:467 snd_probe+0x81c/0x1150 sound/usb/caiaq/device.c:525 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 --- sound/usb/caiaq/device.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/sound/usb/caiaq/device.c b/sound/usb/caiaq/device.c index 0fb6b1b79261..a29674bf96e5 100644 --- a/sound/usb/caiaq/device.c +++ b/sound/usb/caiaq/device.c @@ -461,6 +461,13 @@ static int init_card(struct snd_usb_caiaqdev *cdev) cdev->midi_out_buf, EP1_BUFSIZE, snd_usb_caiaq_midi_output_done, cdev);
+ /* sanity checks of EPs before actually submitting */ + if (usb_urb_ep_type_check(&cdev->ep1_in_urb) || + usb_urb_ep_type_check(&cdev->midi_out_urb)) { + dev_err(dev, "invalid EPs\n"); + return -EINVAL; + } + init_waitqueue_head(&cdev->ep1_wait_queue); init_waitqueue_head(&cdev->prepare_wait_queue);
As syzkaller spotted, currently line6 drivers submit 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 3 != type 1 ------------[ cut here ]------------ WARNING: CPU: 0 PID: 24 at drivers/usb/core/urb.c:449 usb_submit_urb+0xf8a/0x11d0 Modules linked in: CPU: 0 PID: 24 Comm: kworker/0:1 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: line6_start_listen+0x55f/0x9e0 sound/usb/line6/driver.c:82 line6_init_cap_control sound/usb/line6/driver.c:690 line6_probe+0x7c9/0x1310 sound/usb/line6/driver.c:764 podhd_probe+0x64/0x70 sound/usb/line6/podhd.c:474 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 Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/driver.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index c8f723c3a033..167aebf8276e 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -78,6 +78,13 @@ static int line6_start_listen(struct usb_line6 *line6) line6->buffer_listen, LINE6_BUFSIZE_LISTEN, line6_data_received, line6); } + + /* sanity checks of EP before actually submitting */ + if (usb_urb_ep_type_check(line6->urb_listen)) { + dev_err(line6->ifcdev, "invalid control EP\n"); + return -EINVAL; + } + line6->urb_listen->actual_length = 0; err = usb_submit_urb(line6->urb_listen, GFP_ATOMIC); return err;
USB-audio driver may set up a URB containing the fixed EP without validating its presence for some non-class-compliant devices. This may end up with an oops-like kernel warning when submitted.
For avoiding it, this patch adds the call of the new sanity-check helper for URBs. The checks are needed only for MIDI I/O as the other places have already some other checks.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/midi.c | 38 ++++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-)
diff --git a/sound/usb/midi.c b/sound/usb/midi.c index a92e2b2a91ec..7ab25de5ca0a 100644 --- a/sound/usb/midi.c +++ b/sound/usb/midi.c @@ -1282,6 +1282,7 @@ static int snd_usbmidi_in_endpoint_create(struct snd_usb_midi *umidi, unsigned int pipe; int length; unsigned int i; + int err;
rep->in = NULL; ep = kzalloc(sizeof(*ep), GFP_KERNEL); @@ -1292,8 +1293,8 @@ static int snd_usbmidi_in_endpoint_create(struct snd_usb_midi *umidi, for (i = 0; i < INPUT_URBS; ++i) { ep->urbs[i] = usb_alloc_urb(0, GFP_KERNEL); if (!ep->urbs[i]) { - snd_usbmidi_in_endpoint_delete(ep); - return -ENOMEM; + err = -ENOMEM; + goto error; } } if (ep_info->in_interval) @@ -1305,8 +1306,8 @@ static int snd_usbmidi_in_endpoint_create(struct snd_usb_midi *umidi, buffer = usb_alloc_coherent(umidi->dev, length, GFP_KERNEL, &ep->urbs[i]->transfer_dma); if (!buffer) { - snd_usbmidi_in_endpoint_delete(ep); - return -ENOMEM; + err = -ENOMEM; + goto error; } if (ep_info->in_interval) usb_fill_int_urb(ep->urbs[i], umidi->dev, @@ -1318,10 +1319,20 @@ static int snd_usbmidi_in_endpoint_create(struct snd_usb_midi *umidi, pipe, buffer, length, snd_usbmidi_in_urb_complete, ep); ep->urbs[i]->transfer_flags = URB_NO_TRANSFER_DMA_MAP; + err = usb_urb_ep_type_check(ep->urbs[i]); + if (err < 0) { + dev_err(&umidi->dev->dev, "invalid MIDI in EP %x\n", + ep_info->in_ep); + goto error; + } }
rep->in = ep; return 0; + + error: + snd_usbmidi_in_endpoint_delete(ep); + return -ENOMEM; }
/* @@ -1357,6 +1368,7 @@ static int snd_usbmidi_out_endpoint_create(struct snd_usb_midi *umidi, unsigned int i; unsigned int pipe; void *buffer; + int err;
rep->out = NULL; ep = kzalloc(sizeof(*ep), GFP_KERNEL); @@ -1367,8 +1379,8 @@ static int snd_usbmidi_out_endpoint_create(struct snd_usb_midi *umidi, for (i = 0; i < OUTPUT_URBS; ++i) { ep->urbs[i].urb = usb_alloc_urb(0, GFP_KERNEL); if (!ep->urbs[i].urb) { - snd_usbmidi_out_endpoint_delete(ep); - return -ENOMEM; + err = -ENOMEM; + goto error; } ep->urbs[i].ep = ep; } @@ -1406,8 +1418,8 @@ static int snd_usbmidi_out_endpoint_create(struct snd_usb_midi *umidi, ep->max_transfer, GFP_KERNEL, &ep->urbs[i].urb->transfer_dma); if (!buffer) { - snd_usbmidi_out_endpoint_delete(ep); - return -ENOMEM; + err = -ENOMEM; + goto error; } if (ep_info->out_interval) usb_fill_int_urb(ep->urbs[i].urb, umidi->dev, @@ -1419,6 +1431,12 @@ static int snd_usbmidi_out_endpoint_create(struct snd_usb_midi *umidi, pipe, buffer, ep->max_transfer, snd_usbmidi_out_urb_complete, &ep->urbs[i]); + err = usb_urb_ep_type_check(ep->urbs[i].urb); + if (err < 0) { + dev_err(&umidi->dev->dev, "invalid MIDI out EP %x\n", + ep_info->out_ep); + goto error; + } ep->urbs[i].urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP; }
@@ -1437,6 +1455,10 @@ static int snd_usbmidi_out_endpoint_create(struct snd_usb_midi *umidi,
rep->out = ep; return 0; + + error: + snd_usbmidi_out_endpoint_delete(ep); + return err; }
/*
usx2y driver sets up URBs containing the fixed endpoints without validation. This may end up with an oops-like kernel warning when submitted.
For avoiding it, this patch adds the calls of the new sanity-check helper for URBs.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/usx2y/usbusx2y.c | 8 ++++++++ sound/usb/usx2y/usbusx2yaudio.c | 3 +++ 2 files changed, 11 insertions(+)
diff --git a/sound/usb/usx2y/usbusx2y.c b/sound/usb/usx2y/usbusx2y.c index 4569c0efac0a..55a631ccfa25 100644 --- a/sound/usb/usx2y/usbusx2y.c +++ b/sound/usb/usx2y/usbusx2y.c @@ -244,6 +244,9 @@ static void i_usX2Y_In04Int(struct urb *urb) usb_sndbulkpipe(usX2Y->dev, 0x04), &p4out->val.vol, p4out->type == eLT_Light ? sizeof(struct us428_lights) : 5, i_usX2Y_Out04Int, usX2Y); + err = usb_urb_ep_type_check(usX2Y->AS04.urb[j]); + if (err < 0) + break; err = usb_submit_urb(usX2Y->AS04.urb[j], GFP_ATOMIC); us428ctls->p4outSent = send; break; @@ -279,6 +282,9 @@ int usX2Y_AsyncSeq04_init(struct usX2Ydev *usX2Y) usX2Y->AS04.buffer + URB_DataLen_AsyncSeq*i, 0, i_usX2Y_Out04Int, usX2Y ); + err = usb_urb_ep_type_check(usX2Y->AS04.urb[i]); + if (err < 0) + break; } return err; } @@ -298,6 +304,8 @@ int usX2Y_In04_init(struct usX2Ydev *usX2Y) usX2Y->In04Buf, 21, i_usX2Y_In04Int, usX2Y, 10); + if (usb_urb_ep_type_check(usX2Y->In04urb)) + return -EINVAL; return usb_submit_urb(usX2Y->In04urb, GFP_KERNEL); }
diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c index f93b355756e6..345e439aa95b 100644 --- a/sound/usb/usx2y/usbusx2yaudio.c +++ b/sound/usb/usx2y/usbusx2yaudio.c @@ -677,6 +677,9 @@ static int usX2Y_rate_set(struct usX2Ydev *usX2Y, int rate) usb_fill_bulk_urb(us->urb[i], usX2Y->dev, usb_sndbulkpipe(usX2Y->dev, 4), usbdata + i, 2, i_usX2Y_04Int, usX2Y); } + err = usb_urb_ep_type_check(us->urb[0]); + if (err < 0) + goto cleanup; us->submitted = 0; us->len = NOOF_SETRATE_URBS; usX2Y->US04 = us;
hiface usb-audio driver sets up URBs containing the fixed endpoints without validation. This may end up with an oops-like kernel warning when submitted.
For avoiding it, this patch adds the calls of the new sanity-check helper for URBs.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/hiface/pcm.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/sound/usb/hiface/pcm.c b/sound/usb/hiface/pcm.c index 175d8d6b7f59..396c317115b1 100644 --- a/sound/usb/hiface/pcm.c +++ b/sound/usb/hiface/pcm.c @@ -541,6 +541,8 @@ static int hiface_pcm_init_urb(struct pcm_urb *urb, usb_fill_bulk_urb(&urb->instance, chip->dev, usb_sndbulkpipe(chip->dev, ep), (void *)urb->buffer, PCM_PACKET_SIZE, handler, urb); + if (usb_urb_ep_type_check(&urb->instance)) + return -EINVAL; init_usb_anchor(&urb->submitted);
return 0; @@ -599,9 +601,12 @@ int hiface_pcm_init(struct hiface_chip *chip, u8 extra_freq) mutex_init(&rt->stream_mutex); spin_lock_init(&rt->playback.lock);
- for (i = 0; i < PCM_N_URBS; i++) - hiface_pcm_init_urb(&rt->out_urbs[i], chip, OUT_EP, + for (i = 0; i < PCM_N_URBS; i++) { + ret = hiface_pcm_init_urb(&rt->out_urbs[i], chip, OUT_EP, hiface_pcm_out_urb_handler); + if (ret < 0) + return ret; + }
ret = snd_pcm_new(chip->card, "USB-SPDIF Audio", 0, 1, 0, &pcm); if (ret < 0) {
A few other places in caiaq driver have the URB handling with the fixed endpoints without checking the validity, too. Add the sanity check with the new helper function at each appropriate place for avoiding the spurious kernel warnings due to invalid EPs.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/caiaq/input.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/sound/usb/caiaq/input.c b/sound/usb/caiaq/input.c index 4b3fb91deecd..e883659ea6e7 100644 --- a/sound/usb/caiaq/input.c +++ b/sound/usb/caiaq/input.c @@ -718,6 +718,9 @@ int snd_usb_caiaq_input_init(struct snd_usb_caiaqdev *cdev) usb_rcvbulkpipe(usb_dev, 0x4), cdev->ep4_in_buf, EP4_BUFSIZE, snd_usb_caiaq_ep4_reply_dispatch, cdev); + ret = usb_urb_ep_type_check(cdev->ep4_in_urb); + if (ret < 0) + goto exit_free_idev;
snd_usb_caiaq_set_auto_msg(cdev, 1, 10, 5);
@@ -757,6 +760,9 @@ int snd_usb_caiaq_input_init(struct snd_usb_caiaqdev *cdev) usb_rcvbulkpipe(usb_dev, 0x4), cdev->ep4_in_buf, EP4_BUFSIZE, snd_usb_caiaq_ep4_reply_dispatch, cdev); + ret = usb_urb_ep_type_check(cdev->ep4_in_urb); + if (ret < 0) + goto exit_free_idev;
snd_usb_caiaq_set_auto_msg(cdev, 1, 10, 5);
@@ -802,6 +808,9 @@ int snd_usb_caiaq_input_init(struct snd_usb_caiaqdev *cdev) usb_rcvbulkpipe(usb_dev, 0x4), cdev->ep4_in_buf, EP4_BUFSIZE, snd_usb_caiaq_ep4_reply_dispatch, cdev); + ret = usb_urb_ep_type_check(cdev->ep4_in_urb); + if (ret < 0) + goto exit_free_idev;
snd_usb_caiaq_set_auto_msg(cdev, 1, 10, 5); break;
There are a few other places calling usb_submit_urb() with the URB composed from the fixed endpoint without validation. For avoiding the spurious kernel warnings, add the sanity checks to appropriate places.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/driver.c | 23 +++++++++++++++-------- sound/usb/line6/midi.c | 17 +++++++++++------ 2 files changed, 26 insertions(+), 14 deletions(-)
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index 167aebf8276e..8d5a454842f4 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -175,17 +175,24 @@ static int line6_send_raw_message_async_part(struct message *msg, }
msg->done += bytes; - retval = usb_submit_urb(urb, GFP_ATOMIC);
- if (retval < 0) { - dev_err(line6->ifcdev, "%s: usb_submit_urb failed (%d)\n", - __func__, retval); - usb_free_urb(urb); - kfree(msg); - return retval; - } + /* sanity checks of EP before actually submitting */ + retval = usb_urb_ep_type_check(urb); + if (retval < 0) + goto error; + + retval = usb_submit_urb(urb, GFP_ATOMIC); + if (retval < 0) + goto error;
return 0; + + error: + dev_err(line6->ifcdev, "%s: usb_submit_urb failed (%d)\n", + __func__, retval); + usb_free_urb(urb); + kfree(msg); + return retval; }
/* diff --git a/sound/usb/line6/midi.c b/sound/usb/line6/midi.c index 1d3a23b02d68..6d7cde56a355 100644 --- a/sound/usb/line6/midi.c +++ b/sound/usb/line6/midi.c @@ -130,16 +130,21 @@ static int send_midi_async(struct usb_line6 *line6, unsigned char *data, transfer_buffer, length, midi_sent, line6, line6->interval); urb->actual_length = 0; - retval = usb_submit_urb(urb, GFP_ATOMIC); + retval = usb_urb_ep_type_check(urb); + if (retval < 0) + goto error;
- if (retval < 0) { - dev_err(line6->ifcdev, "usb_submit_urb failed\n"); - usb_free_urb(urb); - return retval; - } + retval = usb_submit_urb(urb, GFP_ATOMIC); + if (retval < 0) + goto error;
++line6->line6midi->num_active_send_urbs; return 0; + + error: + dev_err(line6->ifcdev, "usb_submit_urb failed\n"); + usb_free_urb(urb); + return retval; }
static int line6_midi_output_open(struct snd_rawmidi_substream *substream)
participants (3)
-
Andrey Konovalov -
Greg KH -
Takashi Iwai