USB: cdc-acm: probe fixes
Subsystems (3)
cover USB: cdc-acm: probe fixes
This series fixes a couple of bugs in the probe errors paths and does some clean up in preparation for adding the missing error handling when claiming the data interface.
The first two should probably go into 5.12-rc, while the rest could be held off for 5.13 if preferred.
Johan
Johan Hovold (7):
USB: cdc-acm: fix double free on probe failure
USB: cdc-acm: fix use-after-free after probe failure
USB: cdc-acm: drop redundant driver-data assignment
USB: cdc-acm: drop redundant driver-data reset
USB: cdc-acm: clean up probe error labels
USB: cdc-acm: use negation for NULL checks
USB: cdc-acm: always claim data interface
drivers/usb/class/cdc-acm.c | 54 +++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 23 deletions(-)
Signature · 1 line
2.26.2
PATCH 1/7 USB: cdc-acm: fix double free on probe failure
If tty-device registration fails the driver copy of any Country Selection functional descriptor would end up being freed twice; first explicitly in the error path and then again in the tty-port destructor.
Drop the first erroneous free that was left when fixing a tty-port resource leak.
--- drivers/usb/class/cdc-acm.c | 1 - 1 file changed, 1 deletion(-)
drivers/usb/class/cdc-acm.c +0 -1
@@ -1508,7 +1508,6 @@ static int acm_probe(struct usb_interface *intf, &dev_attr_wCountryCodes);device_remove_file (&acm->control->dev, &dev_attr_iCountryCodeRelDate); -kfree (acm->country_codes); }device_remove_file (&acm->control->dev, &dev_attr_bmCapabilities); alloc_fail5:
Signature · 1 line
2.26.2
Re: [PATCH 1/7] USB: cdc-acm: fix double free on probe failure
Am Donnerstag, den 18.03.2021, 16:51 +0100 schrieb Johan Hovold:
Quoted 11 lines
If tty-device registration fails the driver copy of any Country Selection functional descriptor would end up being freed twice; first explicitly in the error path and then again in the tty-port destructor.
Drop the first erroneous free that was left when fixing a tty-port resource leak.
Fixes cae2bc768d17 ("usb: cdc-acm: Decrement tty port's refcount if probe() fail")Cc [email protected] # 4.19Cc Jaejoong Kim <[email protected]>Signed-off-by Johan Hovold <[email protected]>
PATCH 2/7 USB: cdc-acm: fix use-after-free after probe failure
In reply to Johan HovoldIf tty-device registration fails the driver would fail to release the data interface. When the device is later disconnected, the disconnect callback would still be called for the data interface and would go about releasing already freed resources.
--- drivers/usb/class/cdc-acm.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/usb/class/cdc-acm.c +5 -0
@@ -1503,6 +1503,11 @@ static int acm_probe(struct usb_interface *intf,return 0 ; alloc_fail6: +if (!acm->combined_interfaces) { +/* Clear driver data so that disconnect() returns early. */ +usb_set_intfdata (data_interface,NULL ); +usb_driver_release_interface (&acm_driver, data_interface); + }if (acm->country_codes) {device_remove_file (&acm->control->dev, &dev_attr_wCountryCodes);
Signature · 1 line
2.26.2
Re: [PATCH 2/7] USB: cdc-acm: fix use-after-free after probe failure
Am Donnerstag, den 18.03.2021, 16:51 +0100 schrieb Johan Hovold:
Quoted 9 lines
If tty-device registration fails the driver would fail to release the data interface. When the device is later disconnected, the disconnect callback would still be called for the data interface and would go about releasing already freed resources.
Fixes c93d81955005 ("usb: cdc-acm: fix error handling in acm_probe()")Cc [email protected] # 3.9Cc Alexey Khoroshilov <[email protected]>Signed-off-by Johan Hovold <[email protected]>]
PATCH 3/7 USB: cdc-acm: drop redundant driver-data assignment
In reply to Johan HovoldThe interface driver data has already been set by usb_driver_claim_interface() so drop the redundant subsequent assignment.
--- drivers/usb/class/cdc-acm.c | 1 - 1 file changed, 1 deletion(-)
drivers/usb/class/cdc-acm.c +0 -1
@@ -1487,7 +1487,6 @@ static int acm_probe(struct usb_interface *intf,acm_set_line (acm, &acm->line);usb_driver_claim_interface (&acm_driver, data_interface, acm); -usb_set_intfdata (data_interface, acm); tty_dev =tty_port_register_device (&acm->port, acm_tty_driver, minor, &control_interface->dev);
Signature · 1 line
2.26.2
Re: [PATCH 3/7] USB: cdc-acm: drop redundant driver-data assignment
Am Donnerstag, den 18.03.2021, 16:51 +0100 schrieb Johan Hovold:
Quoted 5 lines
The interface driver data has already been set by
usb_driver_claim_interface()so drop the redundant subsequent assignment.Signed-off-by Johan Hovold <[email protected]>
PATCH 4/7 USB: cdc-acm: drop redundant driver-data reset
In reply to Johan HovoldThere's no need to clear the interface driver data on failed probe (and driver core will clear it anyway).
--- drivers/usb/class/cdc-acm.c | 1 - 1 file changed, 1 deletion(-)
drivers/usb/class/cdc-acm.c +0 -1
@@ -1515,7 +1515,6 @@ static int acm_probe(struct usb_interface *intf, }device_remove_file (&acm->control->dev, &dev_attr_bmCapabilities); alloc_fail5: -usb_set_intfdata (intf,NULL );for (i =0 ; i <ACM_NW ; i++)usb_free_urb (acm->wb[i].urb); alloc_fail4:
Signature · 1 line
2.26.2
Re: [PATCH 4/7] USB: cdc-acm: drop redundant driver-data reset
Am Donnerstag, den 18.03.2021, 16:51 +0100 schrieb Johan Hovold:
Quoted 4 lines
There's no need to clear the interface driver data on failed probe (and driver core will clear it anyway).
Signed-off-by Johan Hovold <[email protected]>
PATCH 5/7 USB: cdc-acm: clean up probe error labels
In reply to Johan HovoldName the probe error labels after what they do rather than using sequence numbers which is harder to review and maintain (e.g. may require renaming unrelated labels when a label is added or removed).
--- drivers/usb/class/cdc-acm.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-)
drivers/usb/class/cdc-acm.c +18 -17
@@ -1324,7 +1324,7 @@ static int acm_probe(struct usb_interface *intf, acm =kzalloc (sizeof (struct acm ),GFP_KERNEL );if (acm ==NULL ) - goto alloc_fail; +return -ENOMEM ;tty_port_init (&acm->port); acm->port.ops = &acm_port_ops;
@@ -1341,7 +1341,7 @@ static int acm_probe(struct usb_interface *intf, minor =acm_alloc_minor (acm);if (minor <0 ) - goto alloc_fail1; + goto err_put_port; acm->minor = minor; acm->dev = usb_dev;
@@ -1372,15 +1372,15 @@ static int acm_probe(struct usb_interface *intf, buf =usb_alloc_coherent (usb_dev, ctrlsize,GFP_KERNEL , &acm->ctrl_dma);if (!buf) - goto alloc_fail1; + goto err_put_port; acm->ctrl_buffer = buf;if (acm_write_buffers_alloc (acm) <0 ) - goto alloc_fail2; + goto err_free_ctrl_buffer; acm->ctrlurb =usb_alloc_urb (0 ,GFP_KERNEL );if (!acm->ctrlurb) - goto alloc_fail3; + goto err_free_write_buffers;for (i =0 ; i < num_rx_buf; i++) {struct acm_rb *rb = &(acm->read_buffers[i]);
@@ -1389,13 +1389,13 @@ static int acm_probe(struct usb_interface *intf, rb->base =usb_alloc_coherent (acm->dev, readsize,GFP_KERNEL , &rb->dma);if (!rb->base) - goto alloc_fail4; + goto err_free_read_urbs; rb->index = i; rb->instance = acm; urb =usb_alloc_urb (0 ,GFP_KERNEL );if (!urb) - goto alloc_fail4; + goto err_free_read_urbs; urb->transfer_flags |=URB_NO_TRANSFER_DMA_MAP ; urb->transfer_dma = rb->dma;
@@ -1417,7 +1417,7 @@ static int acm_probe(struct usb_interface *intf, snd->urb =usb_alloc_urb (0 ,GFP_KERNEL );if (snd->urb ==NULL ) - goto alloc_fail5; + goto err_free_write_urbs;if (usb_endpoint_xfer_int (epwrite)) usb_fill_int_urb(snd->urb, usb_dev, acm->out,
@@ -1435,7 +1435,7 @@ static int acm_probe(struct usb_interface *intf, i =device_create_file (&intf->dev, &dev_attr_bmCapabilities);if (i <0 ) - goto alloc_fail5; + goto err_free_write_urbs;if (h.usb_cdc_country_functional_desc) {/* export the country data */ struct usb_cdc_country_functional_desc * cfd =
@@ -1492,7 +1492,7 @@ static int acm_probe(struct usb_interface *intf, &control_interface->dev);if (IS_ERR (tty_dev)) { rv =PTR_ERR (tty_dev); - goto alloc_fail6; + goto err_release_data_interface; }if (quirks &CLEAR_HALT_CONDITIONS ) {
@@ -1501,7 +1501,8 @@ static int acm_probe(struct usb_interface *intf, }return 0 ; -alloc_fail6: + +err_release_data_interface:if (!acm->combined_interfaces) {/* Clear driver data so that disconnect() returns early. */ usb_set_intfdata (data_interface,NULL );
@@ -1514,21 +1515,21 @@ static int acm_probe(struct usb_interface *intf, &dev_attr_iCountryCodeRelDate); }device_remove_file (&acm->control->dev, &dev_attr_bmCapabilities); -alloc_fail5: +err_free_write_urbs:for (i =0 ; i <ACM_NW ; i++)usb_free_urb (acm->wb[i].urb); -alloc_fail4: +err_free_read_urbs:for (i =0 ; i < num_rx_buf; i++)usb_free_urb (acm->read_urbs[i]);acm_read_buffers_free (acm);usb_free_urb (acm->ctrlurb); -alloc_fail3: +err_free_write_buffers:acm_write_buffers_free (acm); -alloc_fail2: +err_free_ctrl_buffer:usb_free_coherent (usb_dev, ctrlsize, acm->ctrl_buffer, acm->ctrl_dma); -alloc_fail1: +err_put_port:tty_port_put (&acm->port); -alloc_fail: +return rv; }
Signature · 1 line
2.26.2
Re: [PATCH 5/7] USB: cdc-acm: clean up probe error labels
Am Donnerstag, den 18.03.2021, 16:52 +0100 schrieb Johan Hovold:
Quoted 5 lines
Name the probe error labels after what they do rather than using sequence numbers which is harder to review and maintain (e.g. may require renaming unrelated labels when a label is added or removed).
Signed-off-by Johan Hovold <[email protected]>
PATCH 6/7 USB: cdc-acm: use negation for NULL checks
In reply to Johan HovoldUse negation consistently throughout the driver for NULL checks.
--- drivers/usb/class/cdc-acm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/usb/class/cdc-acm.c +2 -2
@@ -1323,7 +1323,7 @@ static int acm_probe(struct usb_interface *intf,dev_dbg (&intf->dev,"interfaces are valid\n" ); acm =kzalloc (sizeof (struct acm ),GFP_KERNEL ); -if (acm ==NULL ) +if (!acm)return -ENOMEM ;tty_port_init (&acm->port);
@@ -1416,7 +1416,7 @@ static int acm_probe(struct usb_interface *intf,struct acm_wb *snd = &(acm->wb[i]); snd->urb =usb_alloc_urb (0 ,GFP_KERNEL ); -if (snd->urb ==NULL ) +if (!snd->urb) goto err_free_write_urbs;if (usb_endpoint_xfer_int (epwrite))
Signature · 1 line
2.26.2
Re: [PATCH 6/7] USB: cdc-acm: use negation for NULL checks
Am Donnerstag, den 18.03.2021, 16:52 +0100 schrieb Johan Hovold:
Quoted 3 lines
Use negation consistently throughout the driver for NULL checks.
Signed-off-by Johan Hovold <[email protected]>
PATCH 7/7 USB: cdc-acm: always claim data interface
In reply to Johan HovoldMake sure to always claim the data interface and bail out if it's already bound to another driver or isn't authorised.
--- drivers/usb/class/cdc-acm.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/usb/class/cdc-acm.c +6 -1
@@ -1486,7 +1486,11 @@ static int acm_probe(struct usb_interface *intf, acm->line.bDataBits =8 ;acm_set_line (acm, &acm->line); -usb_driver_claim_interface (&acm_driver, data_interface, acm); +if (!acm->combined_interfaces) { + rv =usb_driver_claim_interface (&acm_driver, data_interface, acm); +if (rv) + goto err_remove_files; + } tty_dev =tty_port_register_device (&acm->port, acm_tty_driver, minor, &control_interface->dev);
@@ -1508,6 +1512,7 @@ static int acm_probe(struct usb_interface *intf,usb_set_intfdata (data_interface,NULL );usb_driver_release_interface (&acm_driver, data_interface); } +err_remove_files:if (acm->country_codes) {device_remove_file (&acm->control->dev, &dev_attr_wCountryCodes);
Signature · 1 line
2.26.2
Oliver Neukum
maintainerRe: [PATCH 7/7] USB: cdc-acm: always claim data interface
Am Donnerstag, den 18.03.2021, 16:52 +0100 schrieb Johan Hovold:
Quoted 2 lines
Make sure to always claim the data interface and bail out if it's already bound to another driver or isn't authorised.
Hi,
Thanks for the fixes. All previous ones are good work. this one is problematic I am afraid.
acm_probe() has a test for the availability of the interface:
if (!combined_interfaces && usb_interface_claimed(data_interface)) {
/* valid in this context */
dev_dbg(&intf->dev, "The data interface isn't available\n");
return -EBUSY;
}That check is ancient. BKL still existed. If you want to remove it and do proper error handling, that would be good. But if you do error handling, the check has to go, too.
Regards
Oliver
Re: [PATCH 7/7] USB: cdc-acm: always claim data interface
Quoted 18 lines · Oliver Neukum
Am Donnerstag, den 18.03.2021, 16:52 +0100 schrieb Johan Hovold:
Quoted 2 lines
Make sure to always claim the data interface and bail out if it's already bound to another driver or isn't authorised.
Hi,
Thanks for the fixes. All previous ones are good work. this one is problematic I am afraid.
acm_probe()has a test for the availability of the interface:if (!combined_interfaces && usb_interface_claimed(data_interface)) { /* valid in this context */ dev_dbg(&intf->dev, "The data interface isn't available\n"); return -EBUSY; }
Quoted 3 lines
That check is ancient. BKL still existed. If you want to remove it and do proper error handling, that would be good. But if you do error handling, the check has to go, too.
Thanks, this bit can go indeed. But note that it's simply because it's now redundant.
I'll send a v2.
Johan