Re: [PATCH] digi_acceleport: do sanity checking for the number of ports

[Oliver Neukum]

On Wed, 2016-03-16 at 10:58 -0400, Johan Hovold wrote:
> Note that this needs to be done for not just the port structure
> (ds_oob_port) by making sure that the serial->num_ports is large
> enough,
> but also that the oob-port has indeed got a write urb allocated (i.e.
> that all expected bulk-out endpoints are available)

How about this version?
We really should have a way to specify that to the generic serial
driver.

Regards
Oliver

From 75c015b7281411abbfc4c148e648786258cffedb Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneu...@suse.com>
Date: Mon, 14 Mar 2016 15:33:15 +0100
Subject: [PATCH] digi_acceleport: do sanity checking for the number of ports

The driver can be crashed with devices that expose crafted
descriptors with too few endpoints.
See:
http://seclists.org/bugtraq/2016/Mar/61

Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
 drivers/usb/serial/digi_acceleport.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/serial/digi_acceleport.c b/drivers/usb/serial/digi_acceleport.c
index 12b0e67..40584d5 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -1302,6 +1302,9 @@ static void digi_release(struct usb_serial *serial)
 
 static int digi_port_probe(struct usb_serial_port *port)
 {
+	if (!port->read_urb || !port->write_urb)
+		return -ENODEV;
+
 	return digi_port_init(port, port->port_number);
 }
 
-- 
2.1.4

Re: [PATCH] digi_acceleport: do sanity checking for the number of ports

[Johan Hovold]

On Wed, Mar 16, 2016 at 02:43:02PM +0100, Oliver Neukum wrote:
> The driver can be crashed with devices that expose crafted
> descriptors with too few endpoints.
> See:
> http://seclists.org/bugtraq/2016/Mar/61
> 
> Signed-off-by: Oliver Neukum 
> ---
>  drivers/usb/serial/digi_acceleport.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/serial/digi_acceleport.c 
> b/drivers/usb/serial/digi_acceleport.c
> index 12b0e67..c4d4d45 100644
> --- a/drivers/usb/serial/digi_acceleport.c
> +++ b/drivers/usb/serial/digi_acceleport.c
> @@ -1260,6 +1260,11 @@ static int digi_startup(struct usb_serial *serial)
>  
>   spin_lock_init(_priv->ds_serial_lock);
>   serial_priv->ds_oob_port_num = serial->type->num_ports;
> + if (!(serial_priv->ds_oob_port_num == 2 && serial->type == 
> _acceleport_2_device)
> + && !(serial_priv->ds_oob_port_num == 4 && serial->type == 
> _acceleport_4_device)) {
> + kfree(serial_priv);
> + return -EINVAL;
> + }

This is not correct and will in fact always fail for two-port devices,
and always succeed for 4-port devices (ds_oob_port_num is simply
assigned the value from the device-type structure which is 3 or 4).

What you need to do is to make sure that the expected end-points are
indeed present. Again, this should ideally be done during interface
probe, but you can just verify that the required structures are non-NULL
here for now if you prefer.

Note that this needs to be done for not just the port structure
(ds_oob_port) by making sure that the serial->num_ports is large enough,
but also that the oob-port has indeed got a write urb allocated (i.e.
that all expected bulk-out endpoints are available).

>   serial_priv->ds_oob_port = serial->port[serial_priv->ds_oob_port_num];
>  
>   ret = digi_port_init(serial_priv->ds_oob_port,

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] digi_acceleport: do sanity checking for the number of ports

[Johan Hovold]

On Thu, Mar 17, 2016 at 12:17:34PM +0100, Oliver Neukum wrote:
> On Wed, 2016-03-16 at 10:58 -0400, Johan Hovold wrote:
> > Note that this needs to be done for not just the port structure
> > (ds_oob_port) by making sure that the serial->num_ports is large
> > enough,
> > but also that the oob-port has indeed got a write urb allocated (i.e.
> > that all expected bulk-out endpoints are available)
> 
> How about this version?
> We really should have a way to specify that to the generic serial
> driver.

Indeed. I have some patches lying around that adds some infrastructure
to core that would make this easier, but I can't seem to find the time
to finish them. I'll try to get to it in a couple of weeks.

> From 75c015b7281411abbfc4c148e648786258cffedb Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneu...@suse.com>
> Date: Mon, 14 Mar 2016 15:33:15 +0100
> Subject: [PATCH] digi_acceleport: do sanity checking for the number of ports
> 
> The driver can be crashed with devices that expose crafted
> descriptors with too few endpoints.
> See:
> http://seclists.org/bugtraq/2016/Mar/61
> 
> Signed-off-by: Oliver Neukum <oneu...@suse.com>
> ---
>  drivers/usb/serial/digi_acceleport.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/serial/digi_acceleport.c 
> b/drivers/usb/serial/digi_acceleport.c
> index 12b0e67..40584d5 100644
> --- a/drivers/usb/serial/digi_acceleport.c
> +++ b/drivers/usb/serial/digi_acceleport.c
> @@ -1302,6 +1302,9 @@ static void digi_release(struct usb_serial *serial)
>  
>  static int digi_port_probe(struct usb_serial_port *port)
>  {
> + if (!port->read_urb || !port->write_urb)
> + return -ENODEV;
> +
>   return digi_port_init(port, port->port_number);
>  }

This won't work as the OOB port is never registered with the driver
core. Yeah, this driver is a bit of a mess...

May be a good idea for the normal ports though, unless you just verify
the expected endpoints once and for all in a probe callback.

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] digi_acceleport: do sanity checking for the number of ports

[Oliver Neukum]

The driver can be crashed with devices that expose crafted
descriptors with too few endpoints.
See:
http://seclists.org/bugtraq/2016/Mar/61

Signed-off-by: Oliver Neukum 
---
 drivers/usb/serial/digi_acceleport.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/serial/digi_acceleport.c 
b/drivers/usb/serial/digi_acceleport.c
index 12b0e67..c4d4d45 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -1260,6 +1260,11 @@ static int digi_startup(struct usb_serial *serial)
 
spin_lock_init(_priv->ds_serial_lock);
serial_priv->ds_oob_port_num = serial->type->num_ports;
+   if (!(serial_priv->ds_oob_port_num == 2 && serial->type == 
_acceleport_2_device)
+   && !(serial_priv->ds_oob_port_num == 4 && serial->type == 
_acceleport_4_device)) {
+   kfree(serial_priv);
+   return -EINVAL;
+   }
serial_priv->ds_oob_port = serial->port[serial_priv->ds_oob_port_num];
 
ret = digi_port_init(serial_priv->ds_oob_port,
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html