USB: cdc-acm: probe fixes
Subsystems (3)
v2 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
Changes in v2
- clarify that the driver already had a check for a bound data interface and remove the now redundant check (Oliver) (7/8)
- add Oliver's ack to patches 1 through 6
- add new to avoid logging a successful probe message on late errors (8/8)
Johan Hovold (8):
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
USB: cdc-acm: do not log successful probe on later errors
drivers/usb/class/cdc-acm.c | 65 +++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 32 deletions(-)
Signature · 1 line
2.26.3
PATCH v2 1/8 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.3
PATCH v2 2/8 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.3
PATCH v2 3/8 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.3
PATCH v2 4/8 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.3
PATCH v2 5/8 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.3
PATCH v2 6/8 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.3
PATCH v2 7/8 USB: cdc-acm: always claim data interface
In reply to Johan HovoldMake sure to always claim the data interface and bail out if binding fails.
Note that the driver had a check to verify that the data interface was not already bound to a driver but would not detect other failures (e.g. if the interface was not authorised).
--- drivers/usb/class/cdc-acm.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
drivers/usb/class/cdc-acm.c +6 -8
@@ -1296,13 +1296,6 @@ static int acm_probe(struct usb_interface *intf,if (!combined_interfaces && intf != control_interface)return -ENODEV ; -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 ; - } - -if (data_interface->cur_altsetting->desc.bNumEndpoints <2 || control_interface->cur_altsetting->desc.bNumEndpoints ==0 )return -EINVAL ;
@@ -1486,7 +1479,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 +1505,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.3
PATCH v2 8/8 USB: cdc-acm: do not log successful probe on later errors
In reply to Johan HovoldDo not log the successful-probe message until the tty device has been registered.
--- drivers/usb/class/cdc-acm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/usb/class/cdc-acm.c +2 -2
@@ -1473,8 +1473,6 @@ static int acm_probe(struct usb_interface *intf, acm->nb_index =0 ; acm->nb_size =0 ; -dev_info (&intf->dev,"ttyACM%d: USB ACM device\n" , minor); - acm->line.dwDTERate =cpu_to_le32 (9600 ); acm->line.bDataBits =8 ;acm_set_line (acm, &acm->line);
@@ -1497,6 +1495,8 @@ static int acm_probe(struct usb_interface *intf,usb_clear_halt (usb_dev, acm->out); } +dev_info (&intf->dev,"ttyACM%d: USB ACM device\n" , minor); +return 0 ; err_release_data_interface:
Signature · 1 line
2.26.3
Greg Kroah-Hartman
maintainerQuoted 6 lines · Johan Hovold
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.
I'll just take them all for 5.12-rc, thanks!
greg k-h