USB: cdc-acm: handle broken union descriptors
Subsystems (3)
v2 cover USB: cdc-acm: handle broken union descriptors
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(-)
Signature · 1 line
2.26.2
PATCH v2 1/4 Revert "cdc-acm: hardening against malicious devices"
This reverts commit 2ad9d544f2497a7bf239c34bd2b86fd19683dbb5.
Drop bogus sanity check; an interface in the active configuration will always have a current altsetting assigned by USB core.
--- drivers/usb/class/cdc-acm.c | 5 ----- 1 file changed, 5 deletions(-)
drivers/usb/class/cdc-acm.c +0 -5
@@ -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" );
Signature · 1 line
2.26.2
PATCH v2 2/4 USB: cdc-acm: handle broken union descriptors
In reply to Johan HovoldHandle 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.
--- drivers/usb/class/cdc-acm.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
drivers/usb/class/cdc-acm.c +12 -0
@@ -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) {
Signature · 1 line
2.26.2
PATCH v2 3/4 USB: cdc-acm: use common data-class define
In reply to Johan HovoldUse the data-class define provided by USB core and drop the driver-specific one.
--- drivers/usb/class/cdc-acm.c | 6 ++---- drivers/usb/class/cdc-acm.h | 2 -- 2 files changed, 2 insertions(+), 6 deletions(-)
drivers/usb/class/cdc-acm.c +2 -4
@@ -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);
drivers/usb/class/cdc-acm.h +0 -2
@@ -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)
Signature · 1 line
2.26.2
PATCH v2 4/4 USB: cdc-acm: clean up no-union-descriptor handling
In reply to Johan HovoldFor 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.
--- drivers/usb/class/cdc-acm.c | 32 ++++++++++---------------------- drivers/usb/class/cdc-acm.h | 11 +++++------ 2 files changed, 15 insertions(+), 28 deletions(-)
drivers/usb/class/cdc-acm.c +10 -22
@@ -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,
drivers/usb/class/cdc-acm.h +5 -6
@@ -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)
Signature · 1 line
2.26.2
Oliver Neukum
maintainerRe: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
Am Montag, den 21.09.2020, 15:59 +0200 schrieb Johan Hovold:
Quoted 3 lines
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
Re: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
Quoted 10 lines · Oliver Neukum
Am Montag, den 21.09.2020, 15:59 +0200 schrieb Johan Hovold:
Quoted 3 lines
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.
Quoted 3 lines
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
Oliver Neukum
maintainerRe: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
Am Montag, den 21.09.2020, 16:28 +0200 schrieb Johan Hovold:
Quoted 7 lines · Oliver Neukum
Quoted 6 lines
Am Montag, den 21.09.2020, 15:59 +0200 schrieb Johan Hovold:
Quoted 3 lines
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,
Quoted 6 lines
Quoted 4 lines
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_INTERFACEif it has a data
In hindsight that seems not the best name.
Quoted 3 lines
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.
Quoted 2 lines
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
Re: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
Quoted 20 lines · Oliver Neukum
Am Montag, den 21.09.2020, 16:28 +0200 schrieb Johan Hovold:
Quoted 7 lines · Oliver Neukum
Quoted 6 lines
Am Montag, den 21.09.2020, 15:59 +0200 schrieb Johan Hovold:
Quoted 3 lines
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,
Quoted 6 lines
Quoted 4 lines
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_INTERFACEif it has a dataIn hindsight that seems not the best name.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 4 lines
Quoted 2 lines
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
Oliver Neukum
maintainerRe: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
Am Montag, den 21.09.2020, 17:16 +0200 schrieb Johan Hovold:
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
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.
Quoted 5 lines
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
Re: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
Quoted 15 lines · Oliver Neukum
Am Montag, den 21.09.2020, 17:16 +0200 schrieb Johan Hovold:
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 6 lines
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
Oliver Neukum
maintainerRe: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
Am Dienstag, den 22.09.2020, 09:05 +0200 schrieb Johan Hovold:
Quoted 9 lines
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.
Quoted 2 lines
The second commit, adding support for a specific device, didn't fix that problem generally
Yes
Quoted 2 lines
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.
Quoted 2 lines
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
Re: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
Quoted 17 lines · Oliver Neukum
Am Dienstag, den 22.09.2020, 09:05 +0200 schrieb Johan Hovold:
Quoted 9 lines
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.
Quoted 2 lines
The second commit, adding support for a specific device, didn't fix that problem generally
Quoted 7 lines
Yes
Quoted 2 lines
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.
Quoted 4 lines
Quoted 2 lines
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
Oliver Neukum
maintainerRe: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
Am Dienstag, den 22.09.2020, 12:54 +0200 schrieb Johan Hovold:
Quoted 8 lines
Heh. Did you actually read the commit message?
"Add
NO_DATA_INTERFACEquirk 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
Re: [PATCH v2 4/4] USB: cdc-acm: clean up no-union-descriptor handling
Quoted 9 lines · Oliver Neukum
Am Dienstag, den 22.09.2020, 12:54 +0200 schrieb Johan Hovold:
Quoted 7 lines
Heh. Did you actually read the commit message?
"Add
NO_DATA_INTERFACEquirk 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."
Quoted 4 lines
Quoted 1 line
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
Hi Johan,
Yes, sure. Patching....
Cheers.
---
Daniel
Hi Johan, Oliver,
I just compiled and tested, everything still works.
Cheers.
---
Daniel
Quoted 3 lines · 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
Hi Johan,
Okay, I replied to the second patch, hope I didn't make any mistake.
Cheers.
---
Daniel
Quoted 3 lines · 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
Am Montag, den 21.09.2020, 15:59 +0200 schrieb Johan Hovold:
Quoted 10 lines
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.
Quoted 3 lines
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?