All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/4] USB: cdc-acm: handle broken union descriptors
@ 2020-09-21 13:59 Johan Hovold
  2020-09-21 13:59 ` [PATCH v2 1/4] Revert "cdc-acm: hardening against malicious devices" Johan Hovold
                   ` (6 more replies)
  0 siblings, 7 replies; 22+ messages in thread
From: Johan Hovold @ 2020-09-21 13:59 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Greg Kroah-Hartman, linux-usb, Daniel Caujolle-Bert, Johan Hovold

This series adds support for handling broken union descriptors by
falling back to "combined-interface" probing.

The first patch drops some bogus altsetting sanity checks which would
otherwise have had to be needlessly reproduced for consistency. The
third patch drops the driver specific data class define in favour of the
common one. The last one, cleans up the no-union-descriptor handling by
probing for a "combined-interface" before falling back to the
call-management descriptor.

Note that I changed my mind on the stable tag; we can't be overly
paranoid about a theoretical risk of breaking some quirky devices. And
if we do, we still want to know about it, right?

Daniel, would you mind giving these a spin as well?

Johan

v2
 - add stable tag to 2/2 as it enables a new class of devices
 - demote a broken-union warning to dev_dbg
 - replace the fourth RFC patch with a clean up of the
   no-union-descriptor case only


Johan Hovold (4):
  Revert "cdc-acm: hardening against malicious devices"
  USB: cdc-acm: handle broken union descriptors
  USB: cdc-acm: use common data-class define
  USB: cdc-acm: clean up no-union-descriptor handling

 drivers/usb/class/cdc-acm.c | 55 ++++++++++++++++---------------------
 drivers/usb/class/cdc-acm.h | 13 ++++-----
 2 files changed, 29 insertions(+), 39 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v2 1/4] Revert "cdc-acm: hardening against malicious devices"
  2020-09-21 13:59 [PATCH v2 0/4] USB: cdc-acm: handle broken union descriptors Johan Hovold
@ 2020-09-21 13:59 ` Johan Hovold
  2020-09-21 13:59 ` [PATCH v2 2/4] USB: cdc-acm: handle broken union descriptors Johan Hovold
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 22+ messages in thread
From: Johan Hovold @ 2020-09-21 13:59 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Greg Kroah-Hartman, linux-usb, Daniel Caujolle-Bert, Johan Hovold

This reverts commit 2ad9d544f2497a7bf239c34bd2b86fd19683dbb5.

Drop bogus sanity check; an interface in the active configuration will
always have a current altsetting assigned by USB core.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/class/cdc-acm.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 7f6f3ab5b8a6..e28ac640ff9c 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1196,9 +1196,6 @@ static int acm_probe(struct usb_interface *intf,
 		return -EINVAL;
 	}
 
-	if (!intf->cur_altsetting)
-		return -EINVAL;
-
 	if (!buflen) {
 		if (intf->cur_altsetting->endpoint &&
 				intf->cur_altsetting->endpoint->extralen &&
@@ -1252,8 +1249,6 @@ static int acm_probe(struct usb_interface *intf,
 		dev_dbg(&intf->dev, "no interfaces\n");
 		return -ENODEV;
 	}
-	if (!data_interface->cur_altsetting || !control_interface->cur_altsetting)
-		return -ENODEV;
 
 	if (data_intf_num != call_intf_num)
 		dev_dbg(&intf->dev, "Separate call control interface. That is not fully supported.\n");
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 2/4] USB: cdc-acm: handle broken union descriptors
  2020-09-21 13:59 [PATCH v2 0/4] USB: cdc-acm: handle broken union descriptors Johan Hovold
  2020-09-21 13:59 ` [PATCH v2 1/4] Revert "cdc-acm: hardening against malicious devices" Johan Hovold
@ 2020-09-21 13:59 ` Johan Hovold
  2020-09-22  9:53   ` <Daniel Caujolle-Bert>
  2020-09-21 13:59 ` [PATCH v2 3/4] USB: cdc-acm: use common data-class define Johan Hovold
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 22+ messages in thread
From: Johan Hovold @ 2020-09-21 13:59 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Greg Kroah-Hartman, linux-usb, Daniel Caujolle-Bert,
	Johan Hovold, stable

Handle broken union functional descriptors where the master-interface
doesn't exist or where its class is of neither Communication or Data
type (as required by the specification) by falling back to
"combined-interface" probing.

Note that this still allows for handling union descriptors with switched
interfaces.

This specifically makes the Whistler radio scanners TRX series devices
work with the driver without adding further quirks to the device-id
table.

Link: https://lore.kernel.org/r/5f4ca4f8.1c69fb81.a4487.0f5f@mx.google.com
Reported-by: Daniel Caujolle-Bert <f1rmb.daniel@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/class/cdc-acm.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index e28ac640ff9c..f42ade505569 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1240,9 +1240,21 @@ static int acm_probe(struct usb_interface *intf,
 			}
 		}
 	} else {
+		int class = -1;
+
 		data_intf_num = union_header->bSlaveInterface0;
 		control_interface = usb_ifnum_to_if(usb_dev, union_header->bMasterInterface0);
 		data_interface = usb_ifnum_to_if(usb_dev, data_intf_num);
+
+		if (control_interface)
+			class = control_interface->cur_altsetting->desc.bInterfaceClass;
+
+		if (class != USB_CLASS_COMM && class != USB_CLASS_CDC_DATA) {
+			dev_dbg(&intf->dev, "Broken union descriptor, assuming single interface\n");
+			combined_interfaces = 1;
+			control_interface = data_interface = intf;
+			goto look_for_collapsed_interface;
+		}
 	}
 
 	if (!control_interface || !data_interface) {
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 3/4] USB: cdc-acm: use common data-class define
  2020-09-21 13:59 [PATCH v2 0/4] USB: cdc-acm: handle broken union descriptors Johan Hovold
  2020-09-21 13:59 ` [PATCH v2 1/4] Revert "cdc-acm: hardening against malicious devices" Johan Hovold
  2020-09-21 13:59 ` [PATCH v2 2/4] USB: cdc-acm: handle broken union descriptors Johan Hovold
@ 2020-09-21 13:59 ` Johan Hovold
  2020-09-21 13:59 ` [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling Johan Hovold
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 22+ messages in thread
From: Johan Hovold @ 2020-09-21 13:59 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Greg Kroah-Hartman, linux-usb, Daniel Caujolle-Bert, Johan Hovold

Use the data-class define provided by USB core and drop the
driver-specific one.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/class/cdc-acm.c | 6 ++----
 drivers/usb/class/cdc-acm.h | 2 --
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index f42ade505569..a361b937684a 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1287,10 +1287,8 @@ static int acm_probe(struct usb_interface *intf,
 skip_normal_probe:
 
 	/*workaround for switched interfaces */
-	if (data_interface->cur_altsetting->desc.bInterfaceClass
-						!= CDC_DATA_INTERFACE_TYPE) {
-		if (control_interface->cur_altsetting->desc.bInterfaceClass
-						== CDC_DATA_INTERFACE_TYPE) {
+	if (data_interface->cur_altsetting->desc.bInterfaceClass != USB_CLASS_CDC_DATA) {
+		if (control_interface->cur_altsetting->desc.bInterfaceClass == USB_CLASS_CDC_DATA) {
 			dev_dbg(&intf->dev,
 				"Your device has switched interfaces.\n");
 			swap(control_interface, data_interface);
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index cd5e9d8ab237..b7174a0098a5 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -131,8 +131,6 @@ struct acm {
 	unsigned long quirks;
 };
 
-#define CDC_DATA_INTERFACE_TYPE	0x0a
-
 /* constants describing various quirks and errors */
 #define NO_UNION_NORMAL			BIT(0)
 #define SINGLE_RX_URB			BIT(1)
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
  2020-09-21 13:59 [PATCH v2 0/4] USB: cdc-acm: handle broken union descriptors Johan Hovold
                   ` (2 preceding siblings ...)
  2020-09-21 13:59 ` [PATCH v2 3/4] USB: cdc-acm: use common data-class define Johan Hovold
@ 2020-09-21 13:59 ` Johan Hovold
  2020-09-21 14:16   ` Oliver Neukum
  2020-09-21 14:21 ` [PATCH v2 0/4] USB: cdc-acm: handle broken union descriptors <Daniel Caujolle-Bert>
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 22+ messages in thread
From: Johan Hovold @ 2020-09-21 13:59 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Greg Kroah-Hartman, linux-usb, Daniel Caujolle-Bert, Johan Hovold

For interfaces that lack a union descriptor, probe for a
"combined-interface" before falling back to the call-management
descriptor instead of the other way round.

This allows for the removal of the NO_DATA_INTERFACE quirk and makes the
probe algorithm somewhat easier to follow.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/class/cdc-acm.c | 32 ++++++++++----------------------
 drivers/usb/class/cdc-acm.h | 11 +++++------
 2 files changed, 15 insertions(+), 28 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index a361b937684a..357e896a4fc0 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1218,26 +1218,19 @@ static int acm_probe(struct usb_interface *intf,
 		call_intf_num = cmgmd->bDataInterface;
 
 	if (!union_header) {
-		if (call_intf_num > 0) {
+		if (intf->cur_altsetting->desc.bNumEndpoints == 3) {
+			dev_dbg(&intf->dev, "No union descriptor, assuming single interface\n");
+			combined_interfaces = 1;
+			control_interface = data_interface = intf;
+			goto look_for_collapsed_interface;
+		} else if (call_intf_num > 0) {
 			dev_dbg(&intf->dev, "No union descriptor, using call management descriptor\n");
-			/* quirks for Droids MuIn LCD */
-			if (quirks & NO_DATA_INTERFACE) {
-				data_interface = usb_ifnum_to_if(usb_dev, 0);
-			} else {
-				data_intf_num = call_intf_num;
-				data_interface = usb_ifnum_to_if(usb_dev, data_intf_num);
-			}
+			data_intf_num = call_intf_num;
+			data_interface = usb_ifnum_to_if(usb_dev, data_intf_num);
 			control_interface = intf;
 		} else {
-			if (intf->cur_altsetting->desc.bNumEndpoints != 3) {
-				dev_dbg(&intf->dev,"No union descriptor, giving up\n");
-				return -ENODEV;
-			} else {
-				dev_warn(&intf->dev,"No union descriptor, testing for castrated device\n");
-				combined_interfaces = 1;
-				control_interface = data_interface = intf;
-				goto look_for_collapsed_interface;
-			}
+			dev_dbg(&intf->dev, "No union descriptor, giving up\n");
+			return -ENODEV;
 		}
 	} else {
 		int class = -1;
@@ -1881,11 +1874,6 @@ static const struct usb_device_id acm_ids[] = {
 
 	/* NOTE: non-Nokia COMM/ACM/0xff is likely MSFT RNDIS... NOT a modem! */
 
-	/* Support for Droids MuIn LCD */
-	{ USB_DEVICE(0x04d8, 0x000b),
-	.driver_info = NO_DATA_INTERFACE,
-	},
-
 #if IS_ENABLED(CONFIG_INPUT_IMS_PCU)
 	{ USB_DEVICE(0x04d8, 0x0082),	/* Application mode */
 	.driver_info = IGNORE_DEVICE,
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index b7174a0098a5..b2135095898f 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -135,9 +135,8 @@ struct acm {
 #define NO_UNION_NORMAL			BIT(0)
 #define SINGLE_RX_URB			BIT(1)
 #define NO_CAP_LINE			BIT(2)
-#define NO_DATA_INTERFACE		BIT(4)
-#define IGNORE_DEVICE			BIT(5)
-#define QUIRK_CONTROL_LINE_STATE	BIT(6)
-#define CLEAR_HALT_CONDITIONS		BIT(7)
-#define SEND_ZERO_PACKET		BIT(8)
-#define DISABLE_ECHO			BIT(9)
+#define IGNORE_DEVICE			BIT(3)
+#define QUIRK_CONTROL_LINE_STATE	BIT(4)
+#define CLEAR_HALT_CONDITIONS		BIT(5)
+#define SEND_ZERO_PACKET		BIT(6)
+#define DISABLE_ECHO			BIT(7)
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
  2020-09-21 13:59 ` [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling Johan Hovold
@ 2020-09-21 14:16   ` Oliver Neukum
  2020-09-21 14:28     ` Johan Hovold
  0 siblings, 1 reply; 22+ messages in thread
From: Oliver Neukum @ 2020-09-21 14:16 UTC (permalink / raw)
  To: Johan Hovold, Erik Slagter
  Cc: Greg Kroah-Hartman, linux-usb, Daniel Caujolle-Bert

Am Montag, den 21.09.2020, 15:59 +0200 schrieb Johan Hovold:
> For interfaces that lack a union descriptor, probe for a
> "combined-interface" before falling back to the call-management
> descriptor instead of the other way round.

Hi,

the more I look at this the more it seems to me like the
device that has the quirk does NOT have a collapsed interface
but two interfaces and just a lack of a union descriptor.
I am taking the original author into CC (hoping it still workd)

Johan, would just taking the first three patches of the series work for
now?

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 0/4] USB: cdc-acm: handle broken union descriptors
  2020-09-21 13:59 [PATCH v2 0/4] USB: cdc-acm: handle broken union descriptors Johan Hovold
                   ` (3 preceding siblings ...)
  2020-09-21 13:59 ` [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling Johan Hovold
@ 2020-09-21 14:21 ` <Daniel Caujolle-Bert>
  2020-09-21 16:19 ` <Daniel Caujolle-Bert>
  2020-09-22 12:10 ` Oliver Neukum
  6 siblings, 0 replies; 22+ messages in thread
From: <Daniel Caujolle-Bert> @ 2020-09-21 14:21 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Oliver Neukum, Greg Kroah-Hartman, linux-usb, Daniel Caujolle-Bert

Hi Johan,


   Yes, sure. Patching....


Cheers.
---
Daniel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
  2020-09-21 14:16   ` Oliver Neukum
@ 2020-09-21 14:28     ` Johan Hovold
  2020-09-21 15:04       ` Oliver Neukum
  0 siblings, 1 reply; 22+ messages in thread
From: Johan Hovold @ 2020-09-21 14:28 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Johan Hovold, Erik Slagter, Greg Kroah-Hartman, linux-usb,
	Daniel Caujolle-Bert

On Mon, Sep 21, 2020 at 04:16:56PM +0200, Oliver Neukum wrote:
> Am Montag, den 21.09.2020, 15:59 +0200 schrieb Johan Hovold:
> > For interfaces that lack a union descriptor, probe for a
> > "combined-interface" before falling back to the call-management
> > descriptor instead of the other way round.
> 
> Hi,
> 
> the more I look at this the more it seems to me like the
> device that has the quirk does NOT have a collapsed interface
> but two interfaces and just a lack of a union descriptor.

But then why name the quirk NO_DATA_INTERFACE if it has a data
interface? By hardcoding the data-interface number to be the one and
only interface, you'd end up probing for a "combined" interface also
with a broken call-management descriptor.

Side note: I really think we should start mandating lsusb output to go
along with any patch for quirky devices.

> I am taking the original author into CC (hoping it still workd)
> Johan, would just taking the first three patches of the series work for
> now?

Of course, no problem holding back the last one for a bit.

Johan

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
  2020-09-21 14:28     ` Johan Hovold
@ 2020-09-21 15:04       ` Oliver Neukum
  2020-09-21 15:16         ` Johan Hovold
  0 siblings, 1 reply; 22+ messages in thread
From: Oliver Neukum @ 2020-09-21 15:04 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Erik Slagter, Greg Kroah-Hartman, linux-usb, Daniel Caujolle-Bert

Am Montag, den 21.09.2020, 16:28 +0200 schrieb Johan Hovold:
> On Mon, Sep 21, 2020 at 04:16:56PM +0200, Oliver Neukum wrote:
> > Am Montag, den 21.09.2020, 15:59 +0200 schrieb Johan Hovold:
> > > For interfaces that lack a union descriptor, probe for a
> > > "combined-interface" before falling back to the call-management
> > > descriptor instead of the other way round.
> > 
> > Hi,

Hi,

> > 
> > the more I look at this the more it seems to me like the
> > device that has the quirk does NOT have a collapsed interface
> > but two interfaces and just a lack of a union descriptor.
> 
> But then why name the quirk NO_DATA_INTERFACE if it has a data

In hindsight that seems not the best name.

> interface? By hardcoding the data-interface number to be the one and
> only interface, you'd end up probing for a "combined" interface also
> with a broken call-management descriptor.

Well, by the changelog assuming a combined interface caused an oops.
Thence I am forced to conclude that the davices _has_ a separate
data interface, but no union descriptor.

> Side note: I really think we should start mandating lsusb output to go
> along with any patch for quirky devices.

Good idea. Convince Greg.

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
  2020-09-21 15:04       ` Oliver Neukum
@ 2020-09-21 15:16         ` Johan Hovold
  2020-09-21 17:17           ` Oliver Neukum
  0 siblings, 1 reply; 22+ messages in thread
From: Johan Hovold @ 2020-09-21 15:16 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Johan Hovold, Erik Slagter, Greg Kroah-Hartman, linux-usb,
	Daniel Caujolle-Bert

On Mon, Sep 21, 2020 at 05:04:34PM +0200, Oliver Neukum wrote:
> Am Montag, den 21.09.2020, 16:28 +0200 schrieb Johan Hovold:
> > On Mon, Sep 21, 2020 at 04:16:56PM +0200, Oliver Neukum wrote:
> > > Am Montag, den 21.09.2020, 15:59 +0200 schrieb Johan Hovold:
> > > > For interfaces that lack a union descriptor, probe for a
> > > > "combined-interface" before falling back to the call-management
> > > > descriptor instead of the other way round.
> > > 
> > > Hi,
> 
> Hi,
> 
> > > 
> > > the more I look at this the more it seems to me like the
> > > device that has the quirk does NOT have a collapsed interface
> > > but two interfaces and just a lack of a union descriptor.
> > 
> > But then why name the quirk NO_DATA_INTERFACE if it has a data
> 
> In hindsight that seems not the best name.
> 
> > interface? By hardcoding the data-interface number to be the one and
> > only interface, you'd end up probing for a "combined" interface also
> > with a broken call-management descriptor.
> 
> Well, by the changelog assuming a combined interface caused an oops.
> Thence I am forced to conclude that the davices _has_ a separate
> data interface, but no union descriptor.

No, the oops was probably due to the missing sanity check later added by
403dff4e2c94 ("USB: cdc-acm: check for valid interfaces").

With a broken call-management descriptor pointing to a non-existent
interface we'd oops before that commit.

> > Side note: I really think we should start mandating lsusb output to go
> > along with any patch for quirky devices.
> 
> Good idea. Convince Greg.

Heh. I'm sure he can be convinced. :)

Johan

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 0/4] USB: cdc-acm: handle broken union descriptors
  2020-09-21 13:59 [PATCH v2 0/4] USB: cdc-acm: handle broken union descriptors Johan Hovold
                   ` (4 preceding siblings ...)
  2020-09-21 14:21 ` [PATCH v2 0/4] USB: cdc-acm: handle broken union descriptors <Daniel Caujolle-Bert>
@ 2020-09-21 16:19 ` <Daniel Caujolle-Bert>
  2020-09-22  7:08   ` Johan Hovold
  2020-09-22 12:10 ` Oliver Neukum
  6 siblings, 1 reply; 22+ messages in thread
From: <Daniel Caujolle-Bert> @ 2020-09-21 16:19 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Oliver Neukum, Greg Kroah-Hartman, linux-usb, Daniel Caujolle-Bert

Hi Johan, Oliver,

   I just compiled and tested, everything still works.


Cheers.
---
Daniel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
  2020-09-21 15:16         ` Johan Hovold
@ 2020-09-21 17:17           ` Oliver Neukum
  2020-09-22  7:05             ` Johan Hovold
  0 siblings, 1 reply; 22+ messages in thread
From: Oliver Neukum @ 2020-09-21 17:17 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Erik Slagter, Greg Kroah-Hartman, linux-usb, Daniel Caujolle-Bert

Am Montag, den 21.09.2020, 17:16 +0200 schrieb Johan Hovold:
> > > interface? By hardcoding the data-interface number to be the one and
> > > only interface, you'd end up probing for a "combined" interface also
> > > with a broken call-management descriptor.
> > 
> > Well, by the changelog assuming a combined interface caused an oops.
> > Thence I am forced to conclude that the davices _has_ a separate
> > data interface, but no union descriptor.
> 
> No, the oops was probably due to the missing sanity check later added by
> 403dff4e2c94 ("USB: cdc-acm: check for valid interfaces").
> 
> With a broken call-management descriptor pointing to a non-existent
> interface we'd oops before that commit.

Hi,

maybe I am dense, but a patch that comes after a patch that is said to
fix something? Furthermore that patch would not come it work,
it would merely make probe() fail cleanly. If I read the changelog
correctly, the change makes the device work.

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
  2020-09-21 17:17           ` Oliver Neukum
@ 2020-09-22  7:05             ` Johan Hovold
  2020-09-22 10:40               ` Oliver Neukum
  0 siblings, 1 reply; 22+ messages in thread
From: Johan Hovold @ 2020-09-22  7:05 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Johan Hovold, Erik Slagter, Greg Kroah-Hartman, linux-usb,
	Daniel Caujolle-Bert

On Mon, Sep 21, 2020 at 07:17:37PM +0200, Oliver Neukum wrote:
> Am Montag, den 21.09.2020, 17:16 +0200 schrieb Johan Hovold:
> > > > interface? By hardcoding the data-interface number to be the one and
> > > > only interface, you'd end up probing for a "combined" interface also
> > > > with a broken call-management descriptor.
> > > 
> > > Well, by the changelog assuming a combined interface caused an oops.
> > > Thence I am forced to conclude that the davices _has_ a separate
> > > data interface, but no union descriptor.
> > 
> > No, the oops was probably due to the missing sanity check later added by
> > 403dff4e2c94 ("USB: cdc-acm: check for valid interfaces").
> > 
> > With a broken call-management descriptor pointing to a non-existent
> > interface we'd oops before that commit.
> 
> Hi,
> 
> maybe I am dense, but a patch that comes after a patch that is said to
> fix something? Furthermore that patch would not come it work,
> it would merely make probe() fail cleanly. If I read the changelog
> correctly, the change makes the device work.

The relevant commits are

  a2bfb4a346d2 ("USB: support for cdc-acm of single interface devices")      (2009)
  fd5054c169d2 ("USB: cdc_acm: Fix oops when Droids MuIn LCD is connected")  (2011)
  403dff4e2c94 ("USB: cdc-acm: check for valid interfaces")                  (2014) 

Before Greg added the sanity checks in that last commit, a broken
call-management descriptor referring to a non-existent interface would
cause a NULL-pointer dereference.

The second commit, adding support for a specific device, didn't fix that
problem generally and only worked around it for one device by hardcoding
the data interface to match the control interface, thereby falling back
to the "combined-interface" probing you added in that first commit.

See? The second commit fixed the oops *and* made the device probe
successfully. If we'd had the sanity check in place by then it would
only have make it probe successfully.

Johan

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 0/4] USB: cdc-acm: handle broken union descriptors
  2020-09-21 16:19 ` <Daniel Caujolle-Bert>
@ 2020-09-22  7:08   ` Johan Hovold
  2020-09-22  9:56     ` <Daniel Caujolle-Bert>
  0 siblings, 1 reply; 22+ messages in thread
From: Johan Hovold @ 2020-09-22  7:08 UTC (permalink / raw)
  To: <Daniel Caujolle-Bert>
  Cc: Johan Hovold, Oliver Neukum, Greg Kroah-Hartman, linux-usb

On Mon, Sep 21, 2020 at 06:19:12PM +0200, <Daniel Caujolle-Bert> wrote:
> Hi Johan, Oliver,
> 
>    I just compiled and tested, everything still works.

Thanks for testing, Daniel.

If you want to you can reply to the second patch with a Tested-by tag so
that it gets added by Greg's tooling (or reply to the cover letter if
you want to have that tag added to all the patches in the series).

Johan

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 2/4] USB: cdc-acm: handle broken union descriptors
  2020-09-21 13:59 ` [PATCH v2 2/4] USB: cdc-acm: handle broken union descriptors Johan Hovold
@ 2020-09-22  9:53   ` <Daniel Caujolle-Bert>
  0 siblings, 0 replies; 22+ messages in thread
From: <Daniel Caujolle-Bert> @ 2020-09-22  9:53 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Oliver Neukum, Greg Kroah-Hartman, linux-usb,
	Daniel Caujolle-Bert, stable

Tested-by: Daniel Caujolle-Bert <f1rmb.daniel@gmail.com>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 0/4] USB: cdc-acm: handle broken union descriptors
  2020-09-22  7:08   ` Johan Hovold
@ 2020-09-22  9:56     ` <Daniel Caujolle-Bert>
  2020-09-22 10:07       ` Johan Hovold
  0 siblings, 1 reply; 22+ messages in thread
From: <Daniel Caujolle-Bert> @ 2020-09-22  9:56 UTC (permalink / raw)
  To: Johan Hovold
  Cc: <Daniel Caujolle-Bert>,
	Oliver Neukum, Greg Kroah-Hartman, linux-usb

Hi Johan,

   Okay, I replied to the second patch, hope I didn't make any mistake.


Cheers.
---
Daniel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 0/4] USB: cdc-acm: handle broken union descriptors
  2020-09-22  9:56     ` <Daniel Caujolle-Bert>
@ 2020-09-22 10:07       ` Johan Hovold
  0 siblings, 0 replies; 22+ messages in thread
From: Johan Hovold @ 2020-09-22 10:07 UTC (permalink / raw)
  To: <Daniel Caujolle-Bert>
  Cc: Johan Hovold, Oliver Neukum, Greg Kroah-Hartman, linux-usb

On Tue, Sep 22, 2020 at 11:56:02AM +0200, <Daniel Caujolle-Bert> wrote:
> Hi Johan,
> 
>    Okay, I replied to the second patch, hope I didn't make any mistake.

Looks good. Thanks again!

Johan

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
  2020-09-22  7:05             ` Johan Hovold
@ 2020-09-22 10:40               ` Oliver Neukum
  2020-09-22 10:54                 ` Johan Hovold
  0 siblings, 1 reply; 22+ messages in thread
From: Oliver Neukum @ 2020-09-22 10:40 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Erik Slagter, Greg Kroah-Hartman, linux-usb, Daniel Caujolle-Bert

Am Dienstag, den 22.09.2020, 09:05 +0200 schrieb Johan Hovold:

> The relevant commits are
> 
>   a2bfb4a346d2 ("USB: support for cdc-acm of single interface devices")      (2009)
>   fd5054c169d2 ("USB: cdc_acm: Fix oops when Droids MuIn LCD is connected")  (2011)
>   403dff4e2c94 ("USB: cdc-acm: check for valid interfaces")                  (2014) 
> 
> Before Greg added the sanity checks in that last commit, a broken
> call-management descriptor referring to a non-existent interface would
> cause a NULL-pointer dereference.

Yes.

> The second commit, adding support for a specific device, didn't fix that
> problem generally

Yes

>  and only worked around it for one device by hardcoding
> the data interface to match the control interface,

How do you know. It hardcoded the data interface. That it matches
the control interface is a guess.

>  thereby falling back
> to the "combined-interface" probing you added in that first commit.

How do you know? They may or may not match. 

	Regards
		Oliver



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
  2020-09-22 10:40               ` Oliver Neukum
@ 2020-09-22 10:54                 ` Johan Hovold
  2020-09-22 11:41                   ` Oliver Neukum
  0 siblings, 1 reply; 22+ messages in thread
From: Johan Hovold @ 2020-09-22 10:54 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Johan Hovold, Erik Slagter, Greg Kroah-Hartman, linux-usb,
	Daniel Caujolle-Bert

On Tue, Sep 22, 2020 at 12:40:42PM +0200, Oliver Neukum wrote:
> Am Dienstag, den 22.09.2020, 09:05 +0200 schrieb Johan Hovold:
> 
> > The relevant commits are
> > 
> >   a2bfb4a346d2 ("USB: support for cdc-acm of single interface devices")      (2009)
> >   fd5054c169d2 ("USB: cdc_acm: Fix oops when Droids MuIn LCD is connected")  (2011)
> >   403dff4e2c94 ("USB: cdc-acm: check for valid interfaces")                  (2014) 
> > 
> > Before Greg added the sanity checks in that last commit, a broken
> > call-management descriptor referring to a non-existent interface would
> > cause a NULL-pointer dereference.
> 
> Yes.
> 
> > The second commit, adding support for a specific device, didn't fix that
> > problem generally
> 
> Yes
> 
> >  and only worked around it for one device by hardcoding
> > the data interface to match the control interface,
> 
> How do you know. It hardcoded the data interface. That it matches
> the control interface is a guess.

No, see below.

> >  thereby falling back
> > to the "combined-interface" probing you added in that first commit.
> 
> How do you know? They may or may not match. 

Heh. Did you actually read the commit message?

	"Add NO_DATA_INTERFACE quirk to tell the driver that "control"
	 and "data" interfaces are not separated for this device, which
	 prevents dereferencing a null pointer in the device probe
	 code."

Convinced yet?

Johan

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
  2020-09-22 10:54                 ` Johan Hovold
@ 2020-09-22 11:41                   ` Oliver Neukum
  2020-09-22 11:47                     ` Johan Hovold
  0 siblings, 1 reply; 22+ messages in thread
From: Oliver Neukum @ 2020-09-22 11:41 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Erik Slagter, Greg Kroah-Hartman, linux-usb, Daniel Caujolle-Bert

Am Dienstag, den 22.09.2020, 12:54 +0200 schrieb Johan Hovold:

> Heh. Did you actually read the commit message?
> 
> 	"Add NO_DATA_INTERFACE quirk to tell the driver that "control"
> 	 and "data" interfaces are not separated for this device, which
> 	 prevents dereferencing a null pointer in the device probe
> 	 code."
> 
> Convinced yet?

This patch does not fully match its description. Very well. Proceed.
Could you resubmit and I'll ack?

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
  2020-09-22 11:41                   ` Oliver Neukum
@ 2020-09-22 11:47                     ` Johan Hovold
  0 siblings, 0 replies; 22+ messages in thread
From: Johan Hovold @ 2020-09-22 11:47 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Johan Hovold, Erik Slagter, Greg Kroah-Hartman, linux-usb,
	Daniel Caujolle-Bert

On Tue, Sep 22, 2020 at 01:41:06PM +0200, Oliver Neukum wrote:
> Am Dienstag, den 22.09.2020, 12:54 +0200 schrieb Johan Hovold:
> 
> > Heh. Did you actually read the commit message?
> > 
> > 	"Add NO_DATA_INTERFACE quirk to tell the driver that "control"
> > 	 and "data" interfaces are not separated for this device, which
> > 	 prevents dereferencing a null pointer in the device probe
> > 	 code."
> > 
> > Convinced yet?
> 
> This patch does not fully match its description. Very well. Proceed.
> Could you resubmit and I'll ack?

I think you can just ack whole series by replying to 0/4. If Greg wants
it resubmitted I'll include that ack when resending. Works for you?

Johan

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 0/4] USB: cdc-acm: handle broken union descriptors
  2020-09-21 13:59 [PATCH v2 0/4] USB: cdc-acm: handle broken union descriptors Johan Hovold
                   ` (5 preceding siblings ...)
  2020-09-21 16:19 ` <Daniel Caujolle-Bert>
@ 2020-09-22 12:10 ` Oliver Neukum
  6 siblings, 0 replies; 22+ messages in thread
From: Oliver Neukum @ 2020-09-22 12:10 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, Daniel Caujolle-Bert

Am Montag, den 21.09.2020, 15:59 +0200 schrieb Johan Hovold:
> This series adds support for handling broken union descriptors by
> falling back to "combined-interface" probing.
> 
> The first patch drops some bogus altsetting sanity checks which would
> otherwise have had to be needlessly reproduced for consistency. The
> third patch drops the driver specific data class define in favour of the
> common one. The last one, cleans up the no-union-descriptor handling by
> probing for a "combined-interface" before falling back to the
> call-management descriptor.
> 
> Note that I changed my mind on the stable tag; we can't be overly
> paranoid about a theoretical risk of breaking some quirky devices. And
> if we do, we still want to know about it, right?

Acked-by: Oliver Neukum <oneukum@suse.com>

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2020-09-22 12:11 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-21 13:59 [PATCH v2 0/4] USB: cdc-acm: handle broken union descriptors Johan Hovold
2020-09-21 13:59 ` [PATCH v2 1/4] Revert "cdc-acm: hardening against malicious devices" Johan Hovold
2020-09-21 13:59 ` [PATCH v2 2/4] USB: cdc-acm: handle broken union descriptors Johan Hovold
2020-09-22  9:53   ` <Daniel Caujolle-Bert>
2020-09-21 13:59 ` [PATCH v2 3/4] USB: cdc-acm: use common data-class define Johan Hovold
2020-09-21 13:59 ` [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling Johan Hovold
2020-09-21 14:16   ` Oliver Neukum
2020-09-21 14:28     ` Johan Hovold
2020-09-21 15:04       ` Oliver Neukum
2020-09-21 15:16         ` Johan Hovold
2020-09-21 17:17           ` Oliver Neukum
2020-09-22  7:05             ` Johan Hovold
2020-09-22 10:40               ` Oliver Neukum
2020-09-22 10:54                 ` Johan Hovold
2020-09-22 11:41                   ` Oliver Neukum
2020-09-22 11:47                     ` Johan Hovold
2020-09-21 14:21 ` [PATCH v2 0/4] USB: cdc-acm: handle broken union descriptors <Daniel Caujolle-Bert>
2020-09-21 16:19 ` <Daniel Caujolle-Bert>
2020-09-22  7:08   ` Johan Hovold
2020-09-22  9:56     ` <Daniel Caujolle-Bert>
2020-09-22 10:07       ` Johan Hovold
2020-09-22 12:10 ` Oliver Neukum

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.