| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 6 Jun 2012 22:28:57 +0800
From: Ming Lei <tom.leiming@...il.com>
To: stefani@...bold.net
Cc: linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org,
oneukum@...e.de, alan@...rguk.ukuu.org.uk,
linux-usb@...r.kernel.org
Subject: Re: [PATCH] fix usb skeleton driver
On Wed, Jun 6, 2012 at 9:23 PM, <stefani@...bold.net> wrote:
> From: Stefani Seibold <stefani@...bold.net>
>
> This is a fix for the USB skeleton driver to bring it in shape.
>
> - The usb_interface structure pointer will be no longer stored
> - Every access to the USB will be handled trought the usb_interface pointer
> - Add a new bool 'connected' for signaling a disconnect (== false)
> - Handle a non blocking read without blocking
> - Code cleanup
> - Synchronize disconnect() handler with open() and release(), to fix races
> - Introduced fsync
> - Single user mode
> - More code cleanup :-)
> - Save some bytes in the dev structure
So many purposes, you need to split your patches for review easily, :-)
>
> Some word about the open()/release() races with disconnect() of an USB device
> (which can happen any time):
> - The return interface pointer from usb_find_interface() could be already owned
> by an other driver and no more longer handle by the skeleton driver.
> - Or the dev pointer return by usb_get_intfdata() could point to an already
> release memory.
>
> This races can not handled by a per device mutex. Only a driver global mutex
> could do this job, since the kref_put() in the skel_disconnect() must be
> protected, otherwise skel_open() could access an already released memory.
>
> I know that this races are very small, but on heavy load or misdesigned or buggy
> hardware this could lead in a OOPS or unpredictable behavior.
>
> The patch is against linux 3.5.0 commit eea5b5510fc5545d15b69da8e485a7424ae388cf
>
> Grek ask me to do this in more pieces, but i will post it for a first RFC.
>
> Hope this helps
>
> Signed-off-by: Stefani Seibold <stefani@...bold.net>
> ---
> drivers/usb/usb-skeleton.c | 218 ++++++++++++++++++++++----------------------
> 1 files changed, 108 insertions(+), 110 deletions(-)
>
> diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c
> index 0616f23..fce5a54 100644
> --- a/drivers/usb/usb-skeleton.c
> +++ b/drivers/usb/usb-skeleton.c
> @@ -1,7 +1,8 @@
> /*
> - * USB Skeleton driver - 2.2
> + * USB Skeleton driver - 2.3
> *
> * Copyright (C) 2001-2004 Greg Kroah-Hartman (greg@...ah.com)
> + * fixes by Stefani Seibold (stefani@...bold.net)
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License as
> @@ -48,8 +49,7 @@ MODULE_DEVICE_TABLE(usb, skel_table);
>
> /* Structure to hold all of our device specific stuff */
> struct usb_skel {
> - struct usb_device *udev; /* the usb device for this device */
> - struct usb_interface *interface; /* the interface for this device */
> + struct usb_device *udev; /* the usb device */
> struct semaphore limit_sem; /* limiting the number of writes in progress */
> struct usb_anchor submitted; /* in case we need to retract our submissions */
> struct urb *bulk_in_urb; /* the urb to read data with */
> @@ -62,15 +62,19 @@ struct usb_skel {
> int errors; /* the last request tanked */
> bool ongoing_read; /* a read is going on */
> bool processed_urb; /* indicates we haven't processed the urb */
> + bool connected; /* connected flag */
> + bool in_use; /* in use flag */
> spinlock_t err_lock; /* lock for errors */
> struct kref kref;
> - struct mutex io_mutex; /* synchronize I/O with disconnect */
> + struct mutex io_mutex; /* synchronize I/O */
> struct completion bulk_in_completion; /* to wait for an ongoing read */
> };
> #define to_skel_dev(d) container_of(d, struct usb_skel, kref)
>
> static struct usb_driver skel_driver;
> -static void skel_draw_down(struct usb_skel *dev);
> +
> +/* synchronize open/release with disconnect */
> +static DEFINE_MUTEX(sync_mutex);
>
> static void skel_delete(struct kref *kref)
> {
> @@ -86,15 +90,13 @@ static int skel_open(struct inode *inode, struct file *file)
> {
> struct usb_skel *dev;
> struct usb_interface *interface;
> - int subminor;
> - int retval = 0;
> + int retval;
>
> - subminor = iminor(inode);
> + /* lock against skel_disconnect() */
> + mutex_lock(&sync_mutex);
This one is not needed since we have minor_rwsem(drivers/usb/core/file.c)
to avoid the race.
>
> - interface = usb_find_interface(&skel_driver, subminor);
> + interface = usb_find_interface(&skel_driver, iminor(inode));
> if (!interface) {
> - pr_err("%s - error, can't find device for minor %d\n",
> - __func__, subminor);
> retval = -ENODEV;
> goto exit;
> }
> @@ -105,52 +107,61 @@ static int skel_open(struct inode *inode, struct file *file)
> goto exit;
> }
>
> - /* increment our usage count for the device */
> - kref_get(&dev->kref);
> -
> - /* lock the device to allow correctly handling errors
> - * in resumption */
> - mutex_lock(&dev->io_mutex);
> + if (dev->in_use) {
> + retval = -EBUSY;
> + goto exit;
> + }
>
> retval = usb_autopm_get_interface(interface);
> if (retval)
> - goto out_err;
> + goto exit;
> +
> + /* increment our usage count for the device */
> + kref_get(&dev->kref);
> +
> + dev->in_use = true;
> + mutex_unlock(&sync_mutex);
>
> /* save our object in the file's private structure */
> file->private_data = dev;
> - mutex_unlock(&dev->io_mutex);
> -
> + return 0;
> exit:
> + mutex_unlock(&sync_mutex);
> return retval;
> }
>
> static int skel_release(struct inode *inode, struct file *file)
> {
> - struct usb_skel *dev;
> -
> - dev = file->private_data;
> - if (dev == NULL)
> - return -ENODEV;
> + struct usb_skel *dev = file->private_data;
>
> + /* lock against skel_disconnect() */
> + mutex_lock(&sync_mutex);
Since the reference count is held now, so is there any race between
release and disconnect?
> /* allow the device to be autosuspended */
> - mutex_lock(&dev->io_mutex);
> - if (dev->interface)
> - usb_autopm_put_interface(dev->interface);
> - mutex_unlock(&dev->io_mutex);
> + if (dev->connected)
> + usb_autopm_put_interface(
> + usb_find_interface(&skel_driver, iminor(inode)));
> + dev->in_use = false;
>
> /* decrement the count on our device */
> kref_put(&dev->kref, skel_delete);
> + mutex_unlock(&sync_mutex);
> return 0;
> }
>
> -static int skel_flush(struct file *file, fl_owner_t id)
> +static void skel_draw_down(struct usb_skel *dev)
> {
> - struct usb_skel *dev;
> - int res;
> + int time;
> +
> + time = usb_wait_anchor_empty_timeout(&dev->submitted, 1000);
> + if (!time)
> + usb_kill_anchored_urbs(&dev->submitted);
> + usb_kill_urb(dev->bulk_in_urb);
> +}
>
> - dev = file->private_data;
> - if (dev == NULL)
> - return -ENODEV;
> +static int skel_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> +{
> + struct usb_skel *dev = file->private_data;
> + int res;
>
> /* wait for io to stop */
> mutex_lock(&dev->io_mutex);
> @@ -167,6 +178,11 @@ static int skel_flush(struct file *file, fl_owner_t id)
> return res;
> }
>
> +static int skel_flush(struct file *file, fl_owner_t id)
> +{
> + return skel_fsync(file, 0, LLONG_MAX, 0);
> +}
> +
> static void skel_read_bulk_callback(struct urb *urb)
> {
> struct usb_skel *dev;
> @@ -179,7 +195,7 @@ static void skel_read_bulk_callback(struct urb *urb)
> if (!(urb->status == -ENOENT ||
> urb->status == -ECONNRESET ||
> urb->status == -ESHUTDOWN))
> - dev_err(&dev->interface->dev,
> + dev_err(&urb->dev->dev,
> "%s - nonzero write bulk status received: %d\n",
> __func__, urb->status);
>
> @@ -187,7 +203,7 @@ static void skel_read_bulk_callback(struct urb *urb)
> } else {
> dev->bulk_in_filled = urb->actual_length;
> }
> - dev->ongoing_read = 0;
> + dev->ongoing_read = false;
> spin_unlock(&dev->err_lock);
>
> complete(&dev->bulk_in_completion);
> @@ -195,7 +211,7 @@ static void skel_read_bulk_callback(struct urb *urb)
>
> static int skel_do_read_io(struct usb_skel *dev, size_t count)
> {
> - int rv;
> + int retval;
>
> /* prepare a read */
> usb_fill_bulk_urb(dev->bulk_in_urb,
> @@ -207,67 +223,61 @@ static int skel_do_read_io(struct usb_skel *dev, size_t count)
> skel_read_bulk_callback,
> dev);
> /* tell everybody to leave the URB alone */
> - spin_lock_irq(&dev->err_lock);
> - dev->ongoing_read = 1;
> - spin_unlock_irq(&dev->err_lock);
> + dev->ongoing_read = true;
>
> /* do it */
> - rv = usb_submit_urb(dev->bulk_in_urb, GFP_KERNEL);
> - if (rv < 0) {
> - dev_err(&dev->interface->dev,
> + retval = usb_submit_urb(dev->bulk_in_urb, GFP_KERNEL);
> + if (retval < 0) {
> + dev_err(&dev->udev->dev,
> "%s - failed submitting read urb, error %d\n",
> - __func__, rv);
> + __func__, retval);
> dev->bulk_in_filled = 0;
> - rv = (rv == -ENOMEM) ? rv : -EIO;
> - spin_lock_irq(&dev->err_lock);
> - dev->ongoing_read = 0;
> - spin_unlock_irq(&dev->err_lock);
> + retval = (retval == -ENOMEM) ? retval : -EIO;
> + dev->ongoing_read = false;
> }
>
> - return rv;
> + return retval;
> }
>
> static ssize_t skel_read(struct file *file, char *buffer, size_t count,
> loff_t *ppos)
> {
> - struct usb_skel *dev;
> - int rv;
> - bool ongoing_io;
> -
> - dev = file->private_data;
> + struct usb_skel *dev = file->private_data;
> + int retval;
>
> /* if we cannot read at all, return EOF */
> if (!dev->bulk_in_urb || !count)
> return 0;
>
> /* no concurrent readers */
> - rv = mutex_lock_interruptible(&dev->io_mutex);
> - if (rv < 0)
> - return rv;
> + if (file->f_flags & O_NONBLOCK) {
> + if (!mutex_trylock(&dev->io_mutex))
> + return -EAGAIN;
> + } else {
> + retval = mutex_lock_interruptible(&dev->io_mutex);
> + if (retval < 0)
> + return retval;
> + }
>
> - if (!dev->interface) { /* disconnect() was called */
> - rv = -ENODEV;
> + if (!dev->connected) { /* disconnect() was called */
> + retval = -ENODEV;
> goto exit;
> }
>
> /* if IO is under way, we must not touch things */
> retry:
> - spin_lock_irq(&dev->err_lock);
> - ongoing_io = dev->ongoing_read;
> - spin_unlock_irq(&dev->err_lock);
> -
> - if (ongoing_io) {
> + if (dev->ongoing_read) {
> /* nonblocking IO shall not wait */
> if (file->f_flags & O_NONBLOCK) {
> - rv = -EAGAIN;
> + retval = -EAGAIN;
> goto exit;
> }
> /*
> * IO may take forever
> * hence wait in an interruptible state
> */
> - rv = wait_for_completion_interruptible(&dev->bulk_in_completion);
> - if (rv < 0)
> + retval = wait_for_completion_interruptible(&dev->bulk_in_completion);
> + if (retval < 0)
> goto exit;
> /*
> * by waiting we also semiprocessed the urb
> @@ -288,12 +298,12 @@ retry:
> }
>
> /* errors must be reported */
> - rv = dev->errors;
> - if (rv < 0) {
> + retval = dev->errors;
> + if (retval < 0) {
> /* any error is reported once */
> dev->errors = 0;
> - /* to preserve notifications about reset */
> - rv = (rv == -EPIPE) ? rv : -EIO;
> + /* to preseretvale notifications about reset */
> + retval = (retval == -EPIPE) ? retval : -EIO;
> /* no data to deliver */
> dev->bulk_in_filled = 0;
> /* report it */
> @@ -315,8 +325,8 @@ retry:
> * all data has been used
> * actual IO needs to be done
> */
> - rv = skel_do_read_io(dev, count);
> - if (rv < 0)
> + retval = skel_do_read_io(dev, count);
> + if (retval < 0)
> goto exit;
> else
> goto retry;
> @@ -329,9 +339,9 @@ retry:
> if (copy_to_user(buffer,
> dev->bulk_in_buffer + dev->bulk_in_copied,
> chunk))
> - rv = -EFAULT;
> + retval = -EFAULT;
> else
> - rv = chunk;
> + retval = chunk;
>
> dev->bulk_in_copied += chunk;
>
> @@ -343,16 +353,16 @@ retry:
> skel_do_read_io(dev, count - chunk);
> } else {
> /* no data in the buffer */
> - rv = skel_do_read_io(dev, count);
> - if (rv < 0)
> + retval = skel_do_read_io(dev, count);
> + if (retval < 0)
> goto exit;
> else if (!(file->f_flags & O_NONBLOCK))
> goto retry;
> - rv = -EAGAIN;
> + retval = -EAGAIN;
> }
> exit:
> mutex_unlock(&dev->io_mutex);
> - return rv;
> + return retval;
> }
>
> static void skel_write_bulk_callback(struct urb *urb)
> @@ -366,7 +376,7 @@ static void skel_write_bulk_callback(struct urb *urb)
> if (!(urb->status == -ENOENT ||
> urb->status == -ECONNRESET ||
> urb->status == -ESHUTDOWN))
> - dev_err(&dev->interface->dev,
> + dev_err(&urb->dev->dev,
> "%s - nonzero write bulk status received: %d\n",
> __func__, urb->status);
>
> @@ -384,14 +394,12 @@ static void skel_write_bulk_callback(struct urb *urb)
> static ssize_t skel_write(struct file *file, const char *user_buffer,
> size_t count, loff_t *ppos)
> {
> - struct usb_skel *dev;
> + struct usb_skel *dev = file->private_data;
> int retval = 0;
> struct urb *urb = NULL;
> char *buf = NULL;
> size_t writesize = min(count, (size_t)MAX_TRANSFER);
>
> - dev = file->private_data;
> -
> /* verify that we actually have some data to write */
> if (count == 0)
> goto exit;
> @@ -444,9 +452,7 @@ static ssize_t skel_write(struct file *file, const char *user_buffer,
> }
>
> /* this lock makes sure we don't submit URBs to gone devices */
> - mutex_lock(&dev->io_mutex);
> - if (!dev->interface) { /* disconnect() was called */
> - mutex_unlock(&dev->io_mutex);
> + if (!dev->connected) { /* disconnect() was called */
> retval = -ENODEV;
> goto error;
> }
> @@ -460,9 +466,8 @@ static ssize_t skel_write(struct file *file, const char *user_buffer,
>
> /* send the data out the bulk port */
> retval = usb_submit_urb(urb, GFP_KERNEL);
> - mutex_unlock(&dev->io_mutex);
> if (retval) {
> - dev_err(&dev->interface->dev,
> + dev_err(&dev->udev->dev,
> "%s - failed submitting write urb, error %d\n",
> __func__, retval);
> goto error_unanchor;
> @@ -496,6 +501,7 @@ static const struct file_operations skel_fops = {
> .write = skel_write,
> .open = skel_open,
> .release = skel_release,
> + .fsync = skel_fsync,
> .flush = skel_flush,
> .llseek = noop_llseek,
> };
> @@ -534,7 +540,8 @@ static int skel_probe(struct usb_interface *interface,
> init_completion(&dev->bulk_in_completion);
>
> dev->udev = usb_get_dev(interface_to_usbdev(interface));
> - dev->interface = interface;
> + dev->connected = true;
> + dev->in_use = false;
>
> /* set up the endpoint information */
> /* use only the first bulk-in and bulk-out endpoints */
> @@ -603,35 +610,26 @@ error:
> static void skel_disconnect(struct usb_interface *interface)
> {
> struct usb_skel *dev;
> - int minor = interface->minor;
>
> - dev = usb_get_intfdata(interface);
> - usb_set_intfdata(interface, NULL);
> + dev_info(&interface->dev, "USB Skeleton disconnect #%d",
> + interface->minor);
>
> /* give back our minor */
> usb_deregister_dev(interface, &skel_class);
>
> - /* prevent more I/O from starting */
> - mutex_lock(&dev->io_mutex);
> - dev->interface = NULL;
> - mutex_unlock(&dev->io_mutex);
> -
> + dev = usb_get_intfdata(interface);
> usb_kill_anchored_urbs(&dev->submitted);
>
> - /* decrement our usage count */
> - kref_put(&dev->kref, skel_delete);
> -
> - dev_info(&interface->dev, "USB Skeleton #%d now disconnected", minor);
> -}
> + /* lock against skel_open() and skel_release() */
> + mutex_lock(&sync_mutex);
> + usb_set_intfdata(interface, NULL);
>
> -static void skel_draw_down(struct usb_skel *dev)
> -{
> - int time;
> + /* prevent more I/O from starting */
> + dev->connected = false;
>
> - time = usb_wait_anchor_empty_timeout(&dev->submitted, 1000);
> - if (!time)
> - usb_kill_anchored_urbs(&dev->submitted);
> - usb_kill_urb(dev->bulk_in_urb);
> + /* decrement our usage count */
> + kref_put(&dev->kref, skel_delete);
> + mutex_unlock(&sync_mutex);
> }
>
> static int skel_suspend(struct usb_interface *intf, pm_message_t message)
> --
> 1.7.8.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists