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: <sta...@vger.kernel.org>
Signed-off-by: Johan Hovold <jhov...@gmail.com>
---
As the patch description says, this fixes a few hairy bugs in keyspan. Since
the patch is so intrusive, it really should get some testing by someone with
actual hardware.
But then again, in it's current state, the driver will oops at disconnect or
module reload.
Johan
drivers/usb/serial/keyspan.c | 181 +++++++++++++++++++++----------------------
drivers/usb/serial/keyspan.h | 8 ++
2 files changed, 96 insertions(+), 93 deletions(-)
diff --git a/drivers/usb/serial/keyspan.c b/drivers/usb/serial/keyspan.c
index 29c943d..7179b0c 100644
--- 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 0a8a40b..0273dda 100644
--- 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[] = {
--
1.7.12.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
