USB: keyspan: fix NULL-pointer dereferences and memory leaks

Fix NULL-pointer dereference at release by moving port data allocation
and deallocation to port_probe and port_remove.

Fix NULL-pointer dereference at disconnect by stopping port urbs at
port_remove.

Since commit 0998d0631001288 (device-core: Ensure drvdata = NULL when no
driver is bound) the port private data is no longer accessible at
disconnect or release.

Note that this patch also fixes port and interface-data memory leaks in
the error path of attach should port initialisation fail for any port.

Compile-only tested.

Cc: <stable@vger.kernel.org>
Signed-off-by: Johan Hovold <jhovold@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/usb/serial/keyspan.c b/drivers/usb/serial/keyspan.c
index 29c943d737d0db508b70cda61ce6770d3a335f43..7179b0c5f8148e183219b2a71c34d6d7bf7494a6 100644 (file)
--- a/drivers/usb/serial/keyspan.c
+++ b/drivers/usb/serial/keyspan.c
@@ -1374,13 +1374,9 @@ static struct callbacks {
           data in device_details */
 static void keyspan_setup_urbs(struct usb_serial *serial)
 {
-       int                             i, j;
        struct keyspan_serial_private   *s_priv;
        const struct keyspan_device_details     *d_details;
-       struct usb_serial_port          *port;
-       struct keyspan_port_private     *p_priv;
        struct callbacks                *cback;
-       int                             endp;
 
        s_priv = usb_get_serial_data(serial);
        d_details = s_priv->device_details;
@@ -1404,45 +1400,6 @@ static void keyspan_setup_urbs(struct usb_serial *serial)
                (serial, d_details->glocont_endpoint, USB_DIR_OUT,
                 serial, s_priv->glocont_buf, GLOCONT_BUFLEN,
                 cback->glocont_callback);
-
-       /* Setup endpoints for each port specific thing */
-       for (i = 0; i < d_details->num_ports; i++) {
-               port = serial->port[i];
-               p_priv = usb_get_serial_port_data(port);
-
-               /* Do indat endpoints first, once for each flip */
-               endp = d_details->indat_endpoints[i];
-               for (j = 0; j <= d_details->indat_endp_flip; ++j, ++endp) {
-                       p_priv->in_urbs[j] = keyspan_setup_urb
-                               (serial, endp, USB_DIR_IN, port,
-                                p_priv->in_buffer[j], 64,
-                                cback->indat_callback);
-               }
-               for (; j < 2; ++j)
-                       p_priv->in_urbs[j] = NULL;
-
-               /* outdat endpoints also have flip */
-               endp = d_details->outdat_endpoints[i];
-               for (j = 0; j <= d_details->outdat_endp_flip; ++j, ++endp) {
-                       p_priv->out_urbs[j] = keyspan_setup_urb
-                               (serial, endp, USB_DIR_OUT, port,
-                                p_priv->out_buffer[j], 64,
-                                cback->outdat_callback);
-               }
-               for (; j < 2; ++j)
-                       p_priv->out_urbs[j] = NULL;
-
-               /* inack endpoint */
-               p_priv->inack_urb = keyspan_setup_urb
-                       (serial, d_details->inack_endpoints[i], USB_DIR_IN,
-                        port, p_priv->inack_buffer, 1, cback->inack_callback);
-
-               /* outcont endpoint */
-               p_priv->outcont_urb = keyspan_setup_urb
-                       (serial, d_details->outcont_endpoints[i], USB_DIR_OUT,
-                        port, p_priv->outcont_buffer, 64,
-                        cback->outcont_callback);
-       }
 }
 
 /* usa19 function doesn't require prescaler */
@@ -2407,9 +2364,7 @@ static void keyspan_send_setup(struct usb_serial_port *port, int reset_port)
 static int keyspan_startup(struct usb_serial *serial)
 {
        int                             i, err;
-       struct usb_serial_port          *port;
        struct keyspan_serial_private   *s_priv;
-       struct keyspan_port_private     *p_priv;
        const struct keyspan_device_details     *d_details;
 
        for (i = 0; (d_details = keyspan_devices[i]) != NULL; ++i)
@@ -2432,19 +2387,6 @@ static int keyspan_startup(struct usb_serial *serial)
        s_priv->device_details = d_details;
        usb_set_serial_data(serial, s_priv);
 
-       /* Now setup per port private data */
-       for (i = 0; i < serial->num_ports; i++) {
-               port = serial->port[i];
-               p_priv = kzalloc(sizeof(struct keyspan_port_private),
-                                                               GFP_KERNEL);
-               if (!p_priv) {
-                       dev_dbg(&port->dev, "%s - kmalloc for keyspan_port_private (%d) failed!.\n", __func__, i);
-                       return 1;
-               }
-               p_priv->device_details = d_details;
-               usb_set_serial_port_data(port, p_priv);
-       }
-
        keyspan_setup_urbs(serial);
 
        if (s_priv->instat_urb != NULL) {
@@ -2463,59 +2405,112 @@ static int keyspan_startup(struct usb_serial *serial)
 
 static void keyspan_disconnect(struct usb_serial *serial)
 {
-       int                             i, j;
-       struct usb_serial_port          *port;
-       struct keyspan_serial_private   *s_priv;
-       struct keyspan_port_private     *p_priv;
+       struct keyspan_serial_private *s_priv;
 
        s_priv = usb_get_serial_data(serial);
 
-       /* Stop reading/writing urbs */
        stop_urb(s_priv->instat_urb);
        stop_urb(s_priv->glocont_urb);
        stop_urb(s_priv->indat_urb);
-       for (i = 0; i < serial->num_ports; ++i) {
-               port = serial->port[i];
-               p_priv = usb_get_serial_port_data(port);
-               stop_urb(p_priv->inack_urb);
-               stop_urb(p_priv->outcont_urb);
-               for (j = 0; j < 2; j++) {
-                       stop_urb(p_priv->in_urbs[j]);
-                       stop_urb(p_priv->out_urbs[j]);
-               }
-       }
+}
+
+static void keyspan_release(struct usb_serial *serial)
+{
+       struct keyspan_serial_private *s_priv;
+
+       s_priv = usb_get_serial_data(serial);
 
-       /* Now free them */
        usb_free_urb(s_priv->instat_urb);
        usb_free_urb(s_priv->indat_urb);
        usb_free_urb(s_priv->glocont_urb);
-       for (i = 0; i < serial->num_ports; ++i) {
-               port = serial->port[i];
-               p_priv = usb_get_serial_port_data(port);
-               usb_free_urb(p_priv->inack_urb);
-               usb_free_urb(p_priv->outcont_urb);
-               for (j = 0; j < 2; j++) {
-                       usb_free_urb(p_priv->in_urbs[j]);
-                       usb_free_urb(p_priv->out_urbs[j]);
-               }
-       }
+
+       kfree(s_priv);
 }
 
-static void keyspan_release(struct usb_serial *serial)
+static int keyspan_port_probe(struct usb_serial_port *port)
 {
-       int                             i;
-       struct usb_serial_port          *port;
-       struct keyspan_serial_private   *s_priv;
+       struct usb_serial *serial = port->serial;
+       struct keyspan_port_private *s_priv;
+       struct keyspan_port_private *p_priv;
+       const struct keyspan_device_details *d_details;
+       struct callbacks *cback;
+       int endp;
+       int port_num;
+       int i;
 
        s_priv = usb_get_serial_data(serial);
+       d_details = s_priv->device_details;
 
-       kfree(s_priv);
+       p_priv = kzalloc(sizeof(*p_priv), GFP_KERNEL);
+       if (!p_priv)
+               return -ENOMEM;
 
-       /* Now free per port private data */
-       for (i = 0; i < serial->num_ports; i++) {
-               port = serial->port[i];
-               kfree(usb_get_serial_port_data(port));
+       s_priv = usb_get_serial_data(port->serial);
+       p_priv->device_details = d_details;
+
+       /* Setup values for the various callback routines */
+       cback = &keyspan_callbacks[d_details->msg_format];
+
+       port_num = port->number - port->serial->minor;
+
+       /* Do indat endpoints first, once for each flip */
+       endp = d_details->indat_endpoints[port_num];
+       for (i = 0; i <= d_details->indat_endp_flip; ++i, ++endp) {
+               p_priv->in_urbs[i] = keyspan_setup_urb(serial, endp,
+                                               USB_DIR_IN, port,
+                                               p_priv->in_buffer[i], 64,
+                                               cback->indat_callback);
+       }
+       /* outdat endpoints also have flip */
+       endp = d_details->outdat_endpoints[port_num];
+       for (i = 0; i <= d_details->outdat_endp_flip; ++i, ++endp) {
+               p_priv->out_urbs[i] = keyspan_setup_urb(serial, endp,
+                                               USB_DIR_OUT, port,
+                                               p_priv->out_buffer[i], 64,
+                                               cback->outdat_callback);
+       }
+       /* inack endpoint */
+       p_priv->inack_urb = keyspan_setup_urb(serial,
+                                       d_details->inack_endpoints[port_num],
+                                       USB_DIR_IN, port,
+                                       p_priv->inack_buffer, 1,
+                                       cback->inack_callback);
+       /* outcont endpoint */
+       p_priv->outcont_urb = keyspan_setup_urb(serial,
+                                       d_details->outcont_endpoints[port_num],
+                                       USB_DIR_OUT, port,
+                                       p_priv->outcont_buffer, 64,
+                                        cback->outcont_callback);
+
+       usb_set_serial_port_data(port, p_priv);
+
+       return 0;
+}
+
+static int keyspan_port_remove(struct usb_serial_port *port)
+{
+       struct keyspan_port_private *p_priv;
+       int i;
+
+       p_priv = usb_get_serial_port_data(port);
+
+       stop_urb(p_priv->inack_urb);
+       stop_urb(p_priv->outcont_urb);
+       for (i = 0; i < 2; i++) {
+               stop_urb(p_priv->in_urbs[i]);
+               stop_urb(p_priv->out_urbs[i]);
+       }
+
+       usb_free_urb(p_priv->inack_urb);
+       usb_free_urb(p_priv->outcont_urb);
+       for (i = 0; i < 2; i++) {
+               usb_free_urb(p_priv->in_urbs[i]);
+               usb_free_urb(p_priv->out_urbs[i]);
        }
+
+       kfree(p_priv);
+
+       return 0;
 }
 
 MODULE_AUTHOR(DRIVER_AUTHOR);

diff --git a/drivers/usb/serial/keyspan.h b/drivers/usb/serial/keyspan.h
index 0a8a40b5711e8d288a222aef5c86573c1a674cc1..0273dda303a4672f348175b6787b03c356a62b72 100644 (file)
--- a/drivers/usb/serial/keyspan.h
+++ b/drivers/usb/serial/keyspan.h
@@ -42,6 +42,8 @@ static void keyspan_dtr_rts           (struct usb_serial_port *port, int on);
 static int  keyspan_startup            (struct usb_serial *serial);
 static void keyspan_disconnect         (struct usb_serial *serial);
 static void keyspan_release            (struct usb_serial *serial);
+static int keyspan_port_probe(struct usb_serial_port *port);
+static int keyspan_port_remove(struct usb_serial_port *port);
 static int  keyspan_write_room         (struct tty_struct *tty);
 
 static int  keyspan_write              (struct tty_struct *tty,
@@ -567,6 +569,8 @@ static struct usb_serial_driver keyspan_1port_device = {
        .attach                 = keyspan_startup,
        .disconnect             = keyspan_disconnect,
        .release                = keyspan_release,
+       .port_probe             = keyspan_port_probe,
+       .port_remove            = keyspan_port_remove,
 };
 
 static struct usb_serial_driver keyspan_2port_device = {
@@ -589,6 +593,8 @@ static struct usb_serial_driver keyspan_2port_device = {
        .attach                 = keyspan_startup,
        .disconnect             = keyspan_disconnect,
        .release                = keyspan_release,
+       .port_probe             = keyspan_port_probe,
+       .port_remove            = keyspan_port_remove,
 };
 
 static struct usb_serial_driver keyspan_4port_device = {
@@ -611,6 +617,8 @@ static struct usb_serial_driver keyspan_4port_device = {
        .attach                 = keyspan_startup,
        .disconnect             = keyspan_disconnect,
        .release                = keyspan_release,
+       .port_probe             = keyspan_port_probe,
+       .port_remove            = keyspan_port_remove,
 };
 
 static struct usb_serial_driver * const serial_drivers[] = {