Re: [PATCH] digi_acceleport: do sanity checking for the number of ports
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
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
---
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
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(&serial_priv->ds_serial_lock);
> serial_priv->ds_oob_port_num = serial->type->num_ports;
> + if (!(serial_priv->ds_oob_port_num == 2 && serial->type ==
> &digi_acceleport_2_device)
> + && !(serial_priv->ds_oob_port_num == 4 && serial->type ==
> &digi_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
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
> 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
> ---
> 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
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(&serial_priv->ds_serial_lock);
serial_priv->ds_oob_port_num = serial->type->num_ports;
+ if (!(serial_priv->ds_oob_port_num == 2 && serial->type ==
&digi_acceleport_2_device)
+ && !(serial_priv->ds_oob_port_num == 4 && serial->type ==
&digi_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
