USB: serial: xr: fix NULL-deref on disconnect
diff mbox series

Message ID 20210226100826.18987-1-johan@kernel.org
State Accepted
Commit f164f5d8a70245217bee89b246dc4fcfececa526
Headers show
Series
  • USB: serial: xr: fix NULL-deref on disconnect
Related show

Commit Message

Johan Hovold Feb. 26, 2021, 10:08 a.m. UTC
Claiming the sibling control interface is a bit more involved and
specifically requires adding support to USB-serial core for managing
either interface being unbound first, something which could otherwise
lead to a NULL-pointer dereference.

Similarly, additional infrastructure is also needed to handle suspend
properly.

Since the driver currently isn't actually using the control interface,
we can defer this for now by simply not claiming the control interface.

Fixes: c2d405aa86b4 ("USB: serial: add MaxLinear/Exar USB to Serial driver")
Reported-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Cc: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/xr_serial.c | 25 -------------------------
 1 file changed, 25 deletions(-)

Comments

Greg KH Feb. 26, 2021, 10:20 a.m. UTC | #1
On Fri, Feb 26, 2021 at 11:08:26AM +0100, Johan Hovold wrote:
> Claiming the sibling control interface is a bit more involved and
> specifically requires adding support to USB-serial core for managing
> either interface being unbound first, something which could otherwise
> lead to a NULL-pointer dereference.
> 
> Similarly, additional infrastructure is also needed to handle suspend
> properly.
> 
> Since the driver currently isn't actually using the control interface,
> we can defer this for now by simply not claiming the control interface.
> 
> Fixes: c2d405aa86b4 ("USB: serial: add MaxLinear/Exar USB to Serial driver")
> Reported-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Cc: Manivannan Sadhasivam <mani@kernel.org>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Mauro Carvalho Chehab Feb. 26, 2021, 10:54 a.m. UTC | #2
Em Fri, 26 Feb 2021 11:08:26 +0100
Johan Hovold <johan@kernel.org> escreveu:

> Claiming the sibling control interface is a bit more involved and
> specifically requires adding support to USB-serial core for managing
> either interface being unbound first, something which could otherwise
> lead to a NULL-pointer dereference.
> 
> Similarly, additional infrastructure is also needed to handle suspend
> properly.
> 
> Since the driver currently isn't actually using the control interface,
> we can defer this for now by simply not claiming the control interface.
> 
> Fixes: c2d405aa86b4 ("USB: serial: add MaxLinear/Exar USB to Serial driver")
> Reported-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Cc: Manivannan Sadhasivam <mani@kernel.org>
> Signed-off-by: Johan Hovold <johan@kernel.org>

That solved the issue with XR21V1410:

	[ 8176.265862] usbcore: registered new interface driver xr_serial
	[ 8176.265885] usbserial: USB Serial support registered for xr_serial
	[ 8176.265921] xr_serial 2-1:1.1: xr_serial converter detected
	[ 8176.266041] usb 2-1: xr_serial converter now attached to ttyUSB0
	[ 8176.268023] printk: console [ttyUSB0] enabled
	[ 8186.512841] usb 2-1: USB disconnect, device number 5
	[ 8186.513131] printk: console [ttyUSB0] disabled
	[ 8186.513340] xr_serial ttyUSB0: xr_serial converter now disconnected from ttyUSB0
	[ 8186.513376] xr_serial 2-1:1.1: device disconnected

Tested-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Regards,
Mauro

> ---
>  drivers/usb/serial/xr_serial.c | 25 -------------------------
>  1 file changed, 25 deletions(-)
> 
> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
> index 483d07dee19d..0ca04906da4b 100644
> --- a/drivers/usb/serial/xr_serial.c
> +++ b/drivers/usb/serial/xr_serial.c
> @@ -545,37 +545,13 @@ static void xr_close(struct usb_serial_port *port)
>  
>  static int xr_probe(struct usb_serial *serial, const struct usb_device_id *id)
>  {
> -	struct usb_driver *driver = serial->type->usb_driver;
> -	struct usb_interface *control_interface;
> -	int ret;
> -
>  	/* Don't bind to control interface */
>  	if (serial->interface->cur_altsetting->desc.bInterfaceNumber == 0)
>  		return -ENODEV;
>  
> -	/* But claim the control interface during data interface probe */
> -	control_interface = usb_ifnum_to_if(serial->dev, 0);
> -	if (!control_interface)
> -		return -ENODEV;
> -
> -	ret = usb_driver_claim_interface(driver, control_interface, NULL);
> -	if (ret) {
> -		dev_err(&serial->interface->dev, "Failed to claim control interface\n");
> -		return ret;
> -	}
> -
>  	return 0;
>  }
>  
> -static void xr_disconnect(struct usb_serial *serial)
> -{
> -	struct usb_driver *driver = serial->type->usb_driver;
> -	struct usb_interface *control_interface;
> -
> -	control_interface = usb_ifnum_to_if(serial->dev, 0);
> -	usb_driver_release_interface(driver, control_interface);
> -}
> -
>  static const struct usb_device_id id_table[] = {
>  	{ USB_DEVICE(0x04e2, 0x1410) }, /* XR21V141X */
>  	{ }
> @@ -590,7 +566,6 @@ static struct usb_serial_driver xr_device = {
>  	.id_table		= id_table,
>  	.num_ports		= 1,
>  	.probe			= xr_probe,
> -	.disconnect		= xr_disconnect,
>  	.open			= xr_open,
>  	.close			= xr_close,
>  	.break_ctl		= xr_break_ctl,



Thanks,
Mauro
Manivannan Sadhasivam Feb. 26, 2021, 10:57 a.m. UTC | #3
On Fri, Feb 26, 2021 at 11:08:26AM +0100, Johan Hovold wrote:
> Claiming the sibling control interface is a bit more involved and
> specifically requires adding support to USB-serial core for managing
> either interface being unbound first, something which could otherwise
> lead to a NULL-pointer dereference.
> 
> Similarly, additional infrastructure is also needed to handle suspend
> properly.
> 
> Since the driver currently isn't actually using the control interface,
> we can defer this for now by simply not claiming the control interface.
> 
> Fixes: c2d405aa86b4 ("USB: serial: add MaxLinear/Exar USB to Serial driver")
> Reported-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Cc: Manivannan Sadhasivam <mani@kernel.org>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Thanks,
Mani

> ---
>  drivers/usb/serial/xr_serial.c | 25 -------------------------
>  1 file changed, 25 deletions(-)
> 
> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
> index 483d07dee19d..0ca04906da4b 100644
> --- a/drivers/usb/serial/xr_serial.c
> +++ b/drivers/usb/serial/xr_serial.c
> @@ -545,37 +545,13 @@ static void xr_close(struct usb_serial_port *port)
>  
>  static int xr_probe(struct usb_serial *serial, const struct usb_device_id *id)
>  {
> -	struct usb_driver *driver = serial->type->usb_driver;
> -	struct usb_interface *control_interface;
> -	int ret;
> -
>  	/* Don't bind to control interface */
>  	if (serial->interface->cur_altsetting->desc.bInterfaceNumber == 0)
>  		return -ENODEV;
>  
> -	/* But claim the control interface during data interface probe */
> -	control_interface = usb_ifnum_to_if(serial->dev, 0);
> -	if (!control_interface)
> -		return -ENODEV;
> -
> -	ret = usb_driver_claim_interface(driver, control_interface, NULL);
> -	if (ret) {
> -		dev_err(&serial->interface->dev, "Failed to claim control interface\n");
> -		return ret;
> -	}
> -
>  	return 0;
>  }
>  
> -static void xr_disconnect(struct usb_serial *serial)
> -{
> -	struct usb_driver *driver = serial->type->usb_driver;
> -	struct usb_interface *control_interface;
> -
> -	control_interface = usb_ifnum_to_if(serial->dev, 0);
> -	usb_driver_release_interface(driver, control_interface);
> -}
> -
>  static const struct usb_device_id id_table[] = {
>  	{ USB_DEVICE(0x04e2, 0x1410) }, /* XR21V141X */
>  	{ }
> @@ -590,7 +566,6 @@ static struct usb_serial_driver xr_device = {
>  	.id_table		= id_table,
>  	.num_ports		= 1,
>  	.probe			= xr_probe,
> -	.disconnect		= xr_disconnect,
>  	.open			= xr_open,
>  	.close			= xr_close,
>  	.break_ctl		= xr_break_ctl,
> -- 
> 2.26.2
>
Johan Hovold March 1, 2021, 9:18 a.m. UTC | #4
On Fri, Feb 26, 2021 at 11:54:47AM +0100, Mauro Carvalho Chehab wrote:
> Em Fri, 26 Feb 2021 11:08:26 +0100
> Johan Hovold <johan@kernel.org> escreveu:
> 
> > Claiming the sibling control interface is a bit more involved and
> > specifically requires adding support to USB-serial core for managing
> > either interface being unbound first, something which could otherwise
> > lead to a NULL-pointer dereference.
> > 
> > Similarly, additional infrastructure is also needed to handle suspend
> > properly.
> > 
> > Since the driver currently isn't actually using the control interface,
> > we can defer this for now by simply not claiming the control interface.
> > 
> > Fixes: c2d405aa86b4 ("USB: serial: add MaxLinear/Exar USB to Serial driver")
> > Reported-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > Cc: Manivannan Sadhasivam <mani@kernel.org>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> 
> That solved the issue with XR21V1410:
> 
> 	[ 8176.265862] usbcore: registered new interface driver xr_serial
> 	[ 8176.265885] usbserial: USB Serial support registered for xr_serial
> 	[ 8176.265921] xr_serial 2-1:1.1: xr_serial converter detected
> 	[ 8176.266041] usb 2-1: xr_serial converter now attached to ttyUSB0
> 	[ 8176.268023] printk: console [ttyUSB0] enabled
> 	[ 8186.512841] usb 2-1: USB disconnect, device number 5
> 	[ 8186.513131] printk: console [ttyUSB0] disabled
> 	[ 8186.513340] xr_serial ttyUSB0: xr_serial converter now disconnected from ttyUSB0
> 	[ 8186.513376] xr_serial 2-1:1.1: device disconnected
> 
> Tested-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Thanks for testing, and thanks to Greg and Manivannan for the review.

Now applied.

Johan

Patch
diff mbox series

diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
index 483d07dee19d..0ca04906da4b 100644
--- a/drivers/usb/serial/xr_serial.c
+++ b/drivers/usb/serial/xr_serial.c
@@ -545,37 +545,13 @@  static void xr_close(struct usb_serial_port *port)
 
 static int xr_probe(struct usb_serial *serial, const struct usb_device_id *id)
 {
-	struct usb_driver *driver = serial->type->usb_driver;
-	struct usb_interface *control_interface;
-	int ret;
-
 	/* Don't bind to control interface */
 	if (serial->interface->cur_altsetting->desc.bInterfaceNumber == 0)
 		return -ENODEV;
 
-	/* But claim the control interface during data interface probe */
-	control_interface = usb_ifnum_to_if(serial->dev, 0);
-	if (!control_interface)
-		return -ENODEV;
-
-	ret = usb_driver_claim_interface(driver, control_interface, NULL);
-	if (ret) {
-		dev_err(&serial->interface->dev, "Failed to claim control interface\n");
-		return ret;
-	}
-
 	return 0;
 }
 
-static void xr_disconnect(struct usb_serial *serial)
-{
-	struct usb_driver *driver = serial->type->usb_driver;
-	struct usb_interface *control_interface;
-
-	control_interface = usb_ifnum_to_if(serial->dev, 0);
-	usb_driver_release_interface(driver, control_interface);
-}
-
 static const struct usb_device_id id_table[] = {
 	{ USB_DEVICE(0x04e2, 0x1410) }, /* XR21V141X */
 	{ }
@@ -590,7 +566,6 @@  static struct usb_serial_driver xr_device = {
 	.id_table		= id_table,
 	.num_ports		= 1,
 	.probe			= xr_probe,
-	.disconnect		= xr_disconnect,
 	.open			= xr_open,
 	.close			= xr_close,
 	.break_ctl		= xr_break_ctl,