On Wed, 13 May 2009, Daniel Mack wrote: > The FTDI USB serial driver dereferences its priv structure in > ftdi_close() without checking for NULL pointer conditions, see the Oops > attached. > > The flaw can be reproduced using the following steps: > > - connect an FTDI serial converter > - run cu -l ttyUSB0 > - disconnect the device > - hit enter in the cu session > > The root cause for this is that ftdi_sio_port_remove() is called upon > the devices disconnect which sets the private pointer to NULL, but the > device remains opened until 'cu' gets a write error and subsequently > closes the device. > > As we can't call cancel_delayed_work_sync() from ftdi_close() anymore, > do that from ftdi_sio_port_remove() now. > @@ -1601,11 +1602,14 @@ static void ftdi_close(struct tty_struct *tty, > mutex_unlock(&port->serial->disc_mutex); > > /* cancel any scheduled reading */ > - cancel_delayed_work_sync(&priv->rx_work); > + if (priv) > + cancel_delayed_work_sync(&priv->rx_work); > > /* shutdown our bulk read */ > usb_kill_urb(port->read_urb); > - kref_put(&priv->kref, ftdi_sio_priv_release); > + > + if (priv) > + kref_put(&priv->kref, ftdi_sio_priv_release); > } /* ftdi_close */ This patch is wrong because it leaks priv: If the pointer is NULL in ftdi_close() then the final kref_put() will never be issued. How about getting rid of the usb_set_serial_port_data(port, NULL); line in ftdi_sio_port_remove() instead? It's not the greatest solution, but it's the best we can do given the way the usb-serial framework is set up. It would also be a good idea to get rid of this comment: /* all open ports are closed at this point * (by usbserial.c:__serial_close, which calls ftdi_close) */ since it is completely wrong. And the similar comment preceding ftdi_shutdown(). Moving the cancel_delayed_work_sync() does make sense. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
