[PATCH 0/7] USB: cdc-acm: probe fixes

[PATCH 0/7] USB: cdc-acm: probe fixes

[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.

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(-)

-- 
2.26.2

[PATCH 1/7] USB: cdc-acm: fix double free on probe failure

[Johan Hovold]

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: stable@vger.kernel.org      # 4.19
Cc: Jaejoong Kim <climbbb.kim@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/class/cdc-acm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 39ddb5585ded..d75a78ad464d 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -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:
-- 
2.26.2

[PATCH 2/7] USB: cdc-acm: fix use-after-free after probe failure

[Johan Hovold]

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: stable@vger.kernel.org      # 3.9
Cc: Alexey Khoroshilov <khoroshilov@ispras.ru>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/class/cdc-acm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index d75a78ad464d..dfc2480add91 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -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);
-- 
2.26.2

[PATCH 3/7] USB: cdc-acm: drop redundant driver-data assignment

[Johan Hovold]

The interface driver data has already been set by
usb_driver_claim_interface() so drop the redundant subsequent
assignment.

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

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index dfc2480add91..36dd1e05e455 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -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);
-- 
2.26.2

[PATCH 4/7] USB: cdc-acm: drop redundant driver-data reset

[Johan Hovold]

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 <johan@kernel.org>
---
 drivers/usb/class/cdc-acm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 36dd1e05e455..682772b8a4f7 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -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:
-- 
2.26.2

[PATCH 5/7] USB: cdc-acm: clean up probe error labels

[Johan Hovold]

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 <johan@kernel.org>
---
 drivers/usb/class/cdc-acm.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 682772b8a4f7..e3c45f5880fc 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -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;
 }
 
-- 
2.26.2

[PATCH 6/7] USB: cdc-acm: use negation for NULL checks

[Johan Hovold]

Use negation consistently throughout the driver for NULL checks.

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

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index e3c45f5880fc..6991ffd66c5d 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -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))
-- 
2.26.2

[PATCH 7/7] USB: cdc-acm: always claim data interface

[Johan Hovold]

Make sure to always claim the data interface and bail out if it's
already bound to another driver or isn't authorised.

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

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 6991ffd66c5d..58c444f9db5e 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -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);
-- 
2.26.2

Re: [PATCH 1/7] USB: cdc-acm: fix double free on probe failure

[Oliver Neukum]

Am Donnerstag, den 18.03.2021, 16:51 +0100 schrieb Johan Hovold:
> 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: stable@vger.kernel.org      # 4.19
> Cc: Jaejoong Kim <climbbb.kim@gmail.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
Acked-by: Oliver Neukum <oneukum@suse.com>

Re: [PATCH 2/7] USB: cdc-acm: fix use-after-free after probe failure

[Oliver Neukum]

Am Donnerstag, den 18.03.2021, 16:51 +0100 schrieb Johan Hovold:
> 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: stable@vger.kernel.org      # 3.9
> Cc: Alexey Khoroshilov <khoroshilov@ispras.ru>
> Signed-off-by: Johan Hovold <johan@kernel.org>]
Acked-by: Oliver Neukum <oneukum@suse.com>

Re: [PATCH 3/7] USB: cdc-acm: drop redundant driver-data assignment

[Oliver Neukum]

Am Donnerstag, den 18.03.2021, 16:51 +0100 schrieb Johan Hovold:
> The interface driver data has already been set by
> usb_driver_claim_interface() so drop the redundant subsequent
> assignment.
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>
Acked-by: Oliver Neukum <oneukum@suse.com>

Re: [PATCH 4/7] USB: cdc-acm: drop redundant driver-data reset

[Oliver Neukum]

Am Donnerstag, den 18.03.2021, 16:51 +0100 schrieb Johan Hovold:
> 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 <johan@kernel.org>
Acked-by: Oliver Neukum <oneukum@suse.com>

Re: [PATCH 5/7] USB: cdc-acm: clean up probe error labels

[Oliver Neukum]

Am Donnerstag, den 18.03.2021, 16:52 +0100 schrieb Johan Hovold:
> 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 <johan@kernel.org>
Acked-by: Oliver Neukum <oneukum@suse.com>

Re: [PATCH 6/7] USB: cdc-acm: use negation for NULL checks

[Oliver Neukum]

Am Donnerstag, den 18.03.2021, 16:52 +0100 schrieb Johan Hovold:
> Use negation consistently throughout the driver for NULL checks.
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>
Acked-by: Oliver Neukum <oneukum@suse.com>

Re: [PATCH 7/7] USB: cdc-acm: always claim data interface

[Oliver Neukum]

Am Donnerstag, den 18.03.2021, 16:52 +0100 schrieb Johan Hovold:
> 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

[Johan Hovold]

On Mon, Mar 22, 2021 at 11:46:47AM +0100, Oliver Neukum wrote:
> Am Donnerstag, den 18.03.2021, 16:52 +0100 schrieb Johan Hovold:
> > 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.

Thanks, this bit can go indeed. But note that it's simply because it's
now redundant.

I'll send a v2.

Johan