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
Use negation consistently throughout the driver for NULL checks.
Signed-off-by: Johan Hovold <[email protected]>
---
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
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.19
Cc: Jaejoong Kim <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
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
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 <[email protected]>
---
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
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.9
Cc: Alexey Khoroshilov <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
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
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: [email protected] # 4.19
> Cc: Jaejoong Kim <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
Acked-by: Oliver Neukum <[email protected]>
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: [email protected] # 3.9
> Cc: Alexey Khoroshilov <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>]
Acked-by: Oliver Neukum <[email protected]>
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 <[email protected]>
Acked-by: Oliver Neukum <[email protected]>
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
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