[PATCH 0/4] USB: cdc-acm: handle broken union descriptors

[PATCH 0/4] USB: cdc-acm: handle broken union descriptors

[Johan Hovold]

This series adds support for handle 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
final patch drops the driver specific data class define in favour of the
common one.

I'm not adding a CC stable tag since this is technically a new feature
even if it enables a class of radio-scanner devices. I guess we can
consider backporting once this gets some more testing though.

Note that I also included a fourth RFC patch implementing an alternative
approach which could replace the second patch entirely. Depending on the
feedback on that, there may be a v2 of the series.

Johan


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 handling of quirky devices

 drivers/usb/class/cdc-acm.c | 43 +++++++++++--------------------------
 drivers/usb/class/cdc-acm.h | 13 +++++------
 2 files changed, 18 insertions(+), 38 deletions(-)

-- 
2.26.2

[PATCH 1/4] Revert "cdc-acm: hardening against malicious devices"

[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

[PATCH 2/4] USB: cdc-acm: handle broken union descriptors

[Johan Hovold]

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 is 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>
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..5b1a1cd0168d 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_warn(&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

[PATCH 3/4] USB: cdc-acm: use common data-class define

[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 5b1a1cd0168d..2758e295871e 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

[RFC 4/4] USB: cdc-acm: clean up handling of quirky devices

[Johan Hovold]

Instead of falling back to "combined-interface" probing when detecting
broken union and call-management descriptors, assume all interfaces with
three endpoints are of "combined-interface" type.

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 | 44 ++++++++++---------------------------
 drivers/usb/class/cdc-acm.h | 11 +++++-----
 2 files changed, 16 insertions(+), 39 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 2758e295871e..eda883ce6d9d 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1217,44 +1217,27 @@ static int acm_probe(struct usb_interface *intf,
 	if (cmgmd)
 		call_intf_num = cmgmd->bDataInterface;
 
+	if (intf->cur_altsetting->desc.bNumEndpoints == 3) {
+		dev_dbg(&intf->dev, "assuming single interface\n");
+		combined_interfaces = 1;
+		control_interface = data_interface = intf;
+		goto look_for_collapsed_interface;
+	}
+
 	if (!union_header) {
 		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;
-
 		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_warn(&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) {
@@ -1881,11 +1864,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

Re: [RFC 4/4] USB: cdc-acm: clean up handling of quirky devices

[Oliver Neukum]

Am Montag, den 21.09.2020, 13:35 +0200 schrieb Johan Hovold:
> Instead of falling back to "combined-interface" probing when detecting
> broken union and call-management descriptors, assume all interfaces with
> three endpoints are of "combined-interface" type.

Hi,

this just ignores a union header. I am afraid that is not correct.
Could you move it into the !union_header clause?

	Regards
		Oliver

Re: [RFC 4/4] USB: cdc-acm: clean up handling of quirky devices

[Johan Hovold]

On Mon, Sep 21, 2020 at 01:53:52PM +0200, Oliver Neukum wrote:
> Am Montag, den 21.09.2020, 13:35 +0200 schrieb Johan Hovold:
> > Instead of falling back to "combined-interface" probing when detecting
> > broken union and call-management descriptors, assume all interfaces with
> > three endpoints are of "combined-interface" type.
> 
> Hi,
> 
> this just ignores a union header. I am afraid that is not correct.
> Could you move it into the !union_header clause?

And probe for a combined interface before falling back to the management
descriptor then? Along the lines of

	if (!union_desc) {
		if (bNumEndpoints == 3) {
			goto probe_combined_interface;
		} else if (call_intf_num > 0) {
			data_intf_num = call_intf_num;
			...
		} else {
			return -ENODEV;
		}
	} else {
		...
	}
			
Johan

Re: [RFC 4/4] USB: cdc-acm: clean up handling of quirky devices

[Oliver Neukum]

Am Montag, den 21.09.2020, 14:15 +0200 schrieb Johan Hovold:
> On Mon, Sep 21, 2020 at 01:53:52PM +0200, Oliver Neukum wrote:
> > Am Montag, den 21.09.2020, 13:35 +0200 schrieb Johan Hovold:
> > > Instead of falling back to "combined-interface" probing when detecting
> > > broken union and call-management descriptors, assume all interfaces with
> > > three endpoints are of "combined-interface" type.
> > 
> > Hi,
> > 
> > this just ignores a union header. I am afraid that is not correct.
> > Could you move it into the !union_header clause?
> 
> And probe for a combined interface before falling back to the management
> descriptor then? Along the lines of


Yes, exactly.

	Regards
		Oliver