[PATCH v2 0/9] sound: Add sanity checks for invalid EPs

[PATCH v2 0/9] sound: Add sanity checks for invalid EPs

[Takashi Iwai]

Hi,

this is a revised patch set 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.

The original issues were spotted by syzkaller, and I put a few others
for covering more similar cases.

The only updated code in v2 is about the first patch to add
usb_urb_ep_type_check() helper (in addition to tested-by tag from
Andrey).  Typos were fixed and it's called also from usb_submit_urb()
as Greg suggested, too.

USB devs: does this look OK now?

If yes, and if I get acks, I can take the patches to sound tree.  Or I
don't mind if you take all to usb tree, too.  Or maybe better, I'll
prepare an immutable branch based on 4.4.14-rc (rc4 for now) in case
you want to pull into both trees.  Let me know your wish.


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          | 30 ++++++++++++++++++++++++++----
 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, 132 insertions(+), 28 deletions(-)

-- 
2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/9] usb: core: Add a helper function to check the validity of EP type in URB

[Takashi Iwai]

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.

Tested-by: Andrey Konovalov <andreyknvl-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
---
v1->v2:
* Fix function name typos
* Call usb_urb_ep_type_check() in usb_submit_urb(), too

 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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Takashi Iwai]

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-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Tested-by: Andrey Konovalov <andreyknvl-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/9] ALSA: caiaq: Add a sanity check for invalid EPs

[Takashi Iwai]

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-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Tested-by: Andrey Konovalov <andreyknvl-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
---
 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);
 
-- 
2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/9] ALSA: line6: Add a sanity check for invalid EPs

[Takashi Iwai]

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-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Tested-by: Andrey Konovalov <andreyknvl-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
---
 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 0ff5a7d2e19f..0da6f68761e3 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;
-- 
2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 5/9] ALSA: usb-audio: Add sanity checks for invalid EPs

[Takashi Iwai]

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-l3A5Bk7waGM@public.gmane.org>
---
 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;
 }
 
 /*
-- 
2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 6/9] ALSA: usx2y: Add sanity checks for invalid EPs

[Takashi Iwai]

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;
-- 
2.14.2

[PATCH v2 7/9] ALSA: hiface: Add sanity checks for invalid EPs

[Takashi Iwai]

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-l3A5Bk7waGM@public.gmane.org>
---
 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) {
-- 
2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 8/9] ALSA: caiaq: Add yet more sanity checks for invalid EPs

[Takashi Iwai]

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-l3A5Bk7waGM@public.gmane.org>
---
 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;
-- 
2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 9/9] ALSA: line6: Add yet more sanity checks for invalid EPs

[Takashi Iwai]

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-l3A5Bk7waGM@public.gmane.org>
---
 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 0da6f68761e3..7c682b219584 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)
-- 
2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/9] sound: Add sanity checks for invalid EPs

[Greg KH]

On Wed, Oct 11, 2017 at 12:36:37PM +0200, Takashi Iwai wrote:
> Hi,
> 
> this is a revised patch set 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.
> 
> The original issues were spotted by syzkaller, and I put a few others
> for covering more similar cases.
> 
> The only updated code in v2 is about the first patch to add
> usb_urb_ep_type_check() helper (in addition to tested-by tag from
> Andrey).  Typos were fixed and it's called also from usb_submit_urb()
> as Greg suggested, too.
> 
> USB devs: does this look OK now?
> 
> If yes, and if I get acks, I can take the patches to sound tree.  Or I
> don't mind if you take all to usb tree, too.  Or maybe better, I'll
> prepare an immutable branch based on 4.4.14-rc (rc4 for now) in case
> you want to pull into both trees.  Let me know your wish.

Whole series looks good to me, feel free to take them in your tree if
that's the easiest:

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

No need for a branch, merges in this area should be rare...

thanks,

greg k-h

Re: [PATCH v2 0/9] sound: Add sanity checks for invalid EPs

[Takashi Iwai]

On Wed, 11 Oct 2017 15:03:29 +0200,
Greg KH wrote:
> 
> On Wed, Oct 11, 2017 at 12:36:37PM +0200, Takashi Iwai wrote:
> > Hi,
> > 
> > this is a revised patch set 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.
> > 
> > The original issues were spotted by syzkaller, and I put a few others
> > for covering more similar cases.
> > 
> > The only updated code in v2 is about the first patch to add
> > usb_urb_ep_type_check() helper (in addition to tested-by tag from
> > Andrey).  Typos were fixed and it's called also from usb_submit_urb()
> > as Greg suggested, too.
> > 
> > USB devs: does this look OK now?
> > 
> > If yes, and if I get acks, I can take the patches to sound tree.  Or I
> > don't mind if you take all to usb tree, too.  Or maybe better, I'll
> > prepare an immutable branch based on 4.4.14-rc (rc4 for now) in case
> > you want to pull into both trees.  Let me know your wish.
> 
> Whole series looks good to me, feel free to take them in your tree if
> that's the easiest:
> 
> Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> 
> No need for a branch, merges in this area should be rare...

Great, thanks for a quick review!
I'll merge through sound git tree.


Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/9] usb: core: Add a helper function to check the validity of EP type in URB

[Johan Hovold]

On Wed, Oct 11, 2017 at 12:36:38PM +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.
> 
> Tested-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> v1->v2:
> * Fix function name typos
> * Call usb_urb_ep_type_check() in usb_submit_urb(), too
> 
>  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
> +};

No this one is no longer used outside of usb_urb_ep_type_check().

> +
> +/**
> + * 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;

And I know Greg asked you to call this function from usb_submit_urb()
which have already done the above ep-lookup once. Maybe not that many
extra cycles, but still seems unnecessary to me.

> +	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]);

Johan

Re: [PATCH v2 3/9] ALSA: caiaq: Add a sanity check for invalid EPs

[Johan Hovold]

On Wed, Oct 11, 2017 at 12:36:40PM +0200, Takashi Iwai wrote:
> 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>
> Tested-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;
> +	}
> +

Unrelated to this patch, but this driver fails to kill the ep1_in_urb
(which is submitted in this function) in case of later probe errors.
This can lead to use-after-free or crashes in the completion callback.

>  	init_waitqueue_head(&cdev->ep1_wait_queue);
>  	init_waitqueue_head(&cdev->prepare_wait_queue);

Johan

Re: [PATCH v2 4/9] ALSA: line6: Add a sanity check for invalid EPs

[Johan Hovold]

On Wed, Oct 11, 2017 at 12:36:41PM +0200, Takashi Iwai wrote:
> 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>
> Tested-by: Andrey Konovalov <andreyknvl@google.com>
> 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 0ff5a7d2e19f..0da6f68761e3 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;
> +	}

You're now doing this check twice (here and in usb_submit_urb) every
time this URB is submitted rather just checking it once in probe.

Seems like a quick band-aid to me.

> +
>  	line6->urb_listen->actual_length = 0;
>  	err = usb_submit_urb(line6->urb_listen, GFP_ATOMIC);
>  	return err;

Johan

Re: [PATCH v2 1/9] usb: core: Add a helper function to check the validity of EP type in URB

[Takashi Iwai]

On Wed, 11 Oct 2017 16:14:55 +0200,
Johan Hovold wrote:
> 
> On Wed, Oct 11, 2017 at 12:36:38PM +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.
> > 
> > Tested-by: Andrey Konovalov <andreyknvl@google.com>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > v1->v2:
> > * Fix function name typos
> > * Call usb_urb_ep_type_check() in usb_submit_urb(), too
> > 
> >  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
> > +};
> 
> No this one is no longer used outside of usb_urb_ep_type_check().

It is used in the error message, so I kept it here.


> > +
> > +/**
> > + * 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;
> 
> And I know Greg asked you to call this function from usb_submit_urb()
> which have already done the above ep-lookup once. Maybe not that many
> extra cycles, but still seems unnecessary to me.

Yeah, we can do micro-optimization later if needed, e.g. splitting the
function.  It's a question of balance between code optimization and
readability...


thanks,

Takashi

> > +	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]);
> 
> Johan
> 

Re: [PATCH v2 6/9] ALSA: usx2y: Add sanity checks for invalid EPs

[Johan Hovold]

On Wed, Oct 11, 2017 at 12:36:43PM +0200, Takashi Iwai wrote:
> 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-l3A5Bk7waGM@public.gmane.org>
> ---
>  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;

It doesn't make much sense to add this check to the completion handler,
where, if you ever get here, you already know that endpoint check at
first submission succeeded.

>  						err = usb_submit_urb(usX2Y->AS04.urb[j], GFP_ATOMIC);
>  						us428ctls->p4outSent = send;
>  						break;

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 9/9] ALSA: line6: Add yet more sanity checks for invalid EPs

[Johan Hovold]

On Wed, Oct 11, 2017 at 12:36:46PM +0200, Takashi Iwai wrote:
> 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-l3A5Bk7waGM@public.gmane.org>
> ---
>  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 0da6f68761e3..7c682b219584 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;

This also seems like something which should have been done once at urb
allocation time, rather than on every submission (just like the URB
initialisation should have been).

> +
> +	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;

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/9] ALSA: caiaq: Add a sanity check for invalid EPs

[Takashi Iwai]

On Wed, 11 Oct 2017 16:20:31 +0200,
Johan Hovold wrote:
> 
> On Wed, Oct 11, 2017 at 12:36:40PM +0200, Takashi Iwai wrote:
> > 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>
> > Tested-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;
> > +	}
> > +
> 
> Unrelated to this patch, but this driver fails to kill the ep1_in_urb
> (which is submitted in this function) in case of later probe errors.
> This can lead to use-after-free or crashes in the completion callback.

Yes, a good catch.  Below is the fix patch.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: caiaq: Fix stray URB at probe error path

caiaq driver doesn't kill the URB properly at its error path during
the probe, which may lead to a use-after-free error later.  This patch
addresses it.

Reported-by: Johan Hovold <johan@kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/caiaq/device.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/sound/usb/caiaq/device.c b/sound/usb/caiaq/device.c
index a29674bf96e5..d55ca48de3ea 100644
--- a/sound/usb/caiaq/device.c
+++ b/sound/usb/caiaq/device.c
@@ -476,10 +476,12 @@ static int init_card(struct snd_usb_caiaqdev *cdev)
 
 	err = snd_usb_caiaq_send_command(cdev, EP1_CMD_GET_DEVICE_INFO, NULL, 0);
 	if (err)
-		return err;
+		goto err_kill_urb;
 
-	if (!wait_event_timeout(cdev->ep1_wait_queue, cdev->spec_received, HZ))
-		return -ENODEV;
+	if (!wait_event_timeout(cdev->ep1_wait_queue, cdev->spec_received, HZ)) {
+		err = -ENODEV;
+		goto err_kill_urb;
+	}
 
 	usb_string(usb_dev, usb_dev->descriptor.iManufacturer,
 		   cdev->vendor_name, CAIAQ_USB_STR_LEN);
@@ -514,6 +516,10 @@ static int init_card(struct snd_usb_caiaqdev *cdev)
 
 	setup_card(cdev);
 	return 0;
+
+ err_kill_urb:
+	usb_kill_urb(&cdev->ep1_in_urb);
+	return err;
 }
 
 static int snd_probe(struct usb_interface *intf,
-- 
2.14.2

Re: [PATCH v2 6/9] ALSA: usx2y: Add sanity checks for invalid EPs

[Takashi Iwai]

On Wed, 11 Oct 2017 16:33:37 +0200,
Johan Hovold wrote:
> 
> On Wed, Oct 11, 2017 at 12:36:43PM +0200, Takashi Iwai wrote:
> > 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-l3A5Bk7waGM@public.gmane.org>
> > ---
> >  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;
> 
> It doesn't make much sense to add this check to the completion handler,
> where, if you ever get here, you already know that endpoint check at
> first submission succeeded.

OK, this one can be dropped while the latter one is kept.


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/9] ALSA: line6: Add a sanity check for invalid EPs

[Takashi Iwai]

On Wed, 11 Oct 2017 16:28:37 +0200,
Johan Hovold wrote:
> 
> On Wed, Oct 11, 2017 at 12:36:41PM +0200, Takashi Iwai wrote:
> > 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-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > Tested-by: Andrey Konovalov <andreyknvl-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
> > ---
> >  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 0ff5a7d2e19f..0da6f68761e3 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;
> > +	}
> 
> You're now doing this check twice (here and in usb_submit_urb) every
> time this URB is submitted rather just checking it once in probe.
> 
> Seems like a quick band-aid to me.

Yes, it is :)

We may split line6_start_listen() to two functions, one for
initializing URBs and another for submission, but it'll be more codes
and the check is fairly cheap.  So again it's a question whether
easier to read or to get minor optimization.


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 9/9] ALSA: line6: Add yet more sanity checks for invalid EPs

[Takashi Iwai]

On Wed, 11 Oct 2017 16:39:52 +0200,
Johan Hovold wrote:
> 
> On Wed, Oct 11, 2017 at 12:36:46PM +0200, Takashi Iwai wrote:
> > 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-l3A5Bk7waGM@public.gmane.org>
> > ---
> >  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 0da6f68761e3..7c682b219584 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;
> 
> This also seems like something which should have been done once at urb
> allocation time, rather than on every submission (just like the URB
> initialisation should have been).

Yes, ideally this would be done in the init time.  But the code
allocates a URB temporarily and fills it depending on the board
configuration, so it's not too trivial to do that without a big code
refactoring.  So the current patch is merely hardening a bit.
For obtaining the ideal code flow, we'd need the whole code
restructuring in anyway for this driver; currently it's too messy.


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/9] ALSA: caiaq: Add a sanity check for invalid EPs

[Johan Hovold]

On Wed, Oct 11, 2017 at 04:40:36PM +0200, Takashi Iwai wrote:
> On Wed, 11 Oct 2017 16:20:31 +0200,
> Johan Hovold wrote:

> > Unrelated to this patch, but this driver fails to kill the ep1_in_urb
> > (which is submitted in this function) in case of later probe errors.
> > This can lead to use-after-free or crashes in the completion callback.
> 
> Yes, a good catch.  Below is the fix patch.

> -- 8< --
> From: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
> Subject: [PATCH] ALSA: caiaq: Fix stray URB at probe error path
> 
> caiaq driver doesn't kill the URB properly at its error path during
> the probe, which may lead to a use-after-free error later.  This patch
> addresses it.
> 
> Reported-by: Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>

Looks good to me:

Reviewed-by: Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

> ---
>  sound/usb/caiaq/device.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/usb/caiaq/device.c b/sound/usb/caiaq/device.c
> index a29674bf96e5..d55ca48de3ea 100644
> --- a/sound/usb/caiaq/device.c
> +++ b/sound/usb/caiaq/device.c
> @@ -476,10 +476,12 @@ static int init_card(struct snd_usb_caiaqdev *cdev)
>  
>  	err = snd_usb_caiaq_send_command(cdev, EP1_CMD_GET_DEVICE_INFO, NULL, 0);
>  	if (err)
> -		return err;
> +		goto err_kill_urb;
>  
> -	if (!wait_event_timeout(cdev->ep1_wait_queue, cdev->spec_received, HZ))
> -		return -ENODEV;
> +	if (!wait_event_timeout(cdev->ep1_wait_queue, cdev->spec_received, HZ)) {
> +		err = -ENODEV;
> +		goto err_kill_urb;
> +	}
>  
>  	usb_string(usb_dev, usb_dev->descriptor.iManufacturer,
>  		   cdev->vendor_name, CAIAQ_USB_STR_LEN);
> @@ -514,6 +516,10 @@ static int init_card(struct snd_usb_caiaqdev *cdev)
>  
>  	setup_card(cdev);
>  	return 0;
> +
> + err_kill_urb:
> +	usb_kill_urb(&cdev->ep1_in_urb);
> +	return err;
>  }
>  
>  static int snd_probe(struct usb_interface *intf,
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/9] ALSA: line6: Add a sanity check for invalid EPs

[Johan Hovold]

On Wed, Oct 11, 2017 at 04:45:16PM +0200, Takashi Iwai wrote:
> On Wed, 11 Oct 2017 16:28:37 +0200,
> Johan Hovold wrote:
> > 
> > On Wed, Oct 11, 2017 at 12:36:41PM +0200, Takashi Iwai wrote:
> > > 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-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > > Tested-by: Andrey Konovalov <andreyknvl-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > > Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
> > > ---
> > >  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 0ff5a7d2e19f..0da6f68761e3 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;
> > > +	}
> > 
> > You're now doing this check twice (here and in usb_submit_urb) every
> > time this URB is submitted rather just checking it once in probe.
> > 
> > Seems like a quick band-aid to me.
> 
> Yes, it is :)
> 
> We may split line6_start_listen() to two functions, one for
> initializing URBs and another for submission, but it'll be more codes
> and the check is fairly cheap.  So again it's a question whether
> easier to read or to get minor optimization.

I'd say it's also a question about long-term maintainability. Those
quick band-aids tend to keep piling up over time.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/9] usb: core: Add a helper function to check the validity of EP type in URB

[Johan Hovold]

On Wed, Oct 11, 2017 at 04:31:07PM +0200, Takashi Iwai wrote:
> On Wed, 11 Oct 2017 16:14:55 +0200,
> Johan Hovold wrote:
> > 
> > On Wed, Oct 11, 2017 at 12:36:38PM +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.
> > > 
> > > Tested-by: Andrey Konovalov <andreyknvl-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > > Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
> > > ---
> > > v1->v2:
> > > * Fix function name typos
> > > * Call usb_urb_ep_type_check() in usb_submit_urb(), too
> > > 
> > >  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
> > > +};
> > 
> > No this one is no longer used outside of usb_urb_ep_type_check().
> 
> It is used in the error message, so I kept it here.

Ah, my bad, sorry.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/9] ALSA: line6: Add a sanity check for invalid EPs

[Takashi Iwai]

On Wed, 11 Oct 2017 16:52:09 +0200,
Johan Hovold wrote:
> 
> On Wed, Oct 11, 2017 at 04:45:16PM +0200, Takashi Iwai wrote:
> > On Wed, 11 Oct 2017 16:28:37 +0200,
> > Johan Hovold wrote:
> > > 
> > > On Wed, Oct 11, 2017 at 12:36:41PM +0200, Takashi Iwai wrote:
> > > > 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-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > > > Tested-by: Andrey Konovalov <andreyknvl-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > > > Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
> > > > ---
> > > >  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 0ff5a7d2e19f..0da6f68761e3 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;
> > > > +	}
> > > 
> > > You're now doing this check twice (here and in usb_submit_urb) every
> > > time this URB is submitted rather just checking it once in probe.
> > > 
> > > Seems like a quick band-aid to me.
> > 
> > Yes, it is :)
> > 
> > We may split line6_start_listen() to two functions, one for
> > initializing URBs and another for submission, but it'll be more codes
> > and the check is fairly cheap.  So again it's a question whether
> > easier to read or to get minor optimization.
> 
> I'd say it's also a question about long-term maintainability. Those
> quick band-aids tend to keep piling up over time.

The codes we've touched are mostly for device-specific drivers or
quirk codes, and they are basically finished ones, so I guess it's a
kind of once-off fix.  Let's see.


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/9] ALSA: caiaq: Add a sanity check for invalid EPs

[Takashi Iwai]

On Wed, 11 Oct 2017 16:49:10 +0200,
Johan Hovold wrote:
> 
> On Wed, Oct 11, 2017 at 04:40:36PM +0200, Takashi Iwai wrote:
> > On Wed, 11 Oct 2017 16:20:31 +0200,
> > Johan Hovold wrote:
> 
> > > Unrelated to this patch, but this driver fails to kill the ep1_in_urb
> > > (which is submitted in this function) in case of later probe errors.
> > > This can lead to use-after-free or crashes in the completion callback.
> > 
> > Yes, a good catch.  Below is the fix patch.
> 
> > -- 8< --
> > From: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
> > Subject: [PATCH] ALSA: caiaq: Fix stray URB at probe error path
> > 
> > caiaq driver doesn't kill the URB properly at its error path during
> > the probe, which may lead to a use-after-free error later.  This patch
> > addresses it.
> > 
> > Reported-by: Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
> 
> Looks good to me:
> 
> Reviewed-by: Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Thanks.  It's rather a material for for-linus with Cc to stable, so
marked it and queued.


Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html