| 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 15:50:51 +0200
From: Oliver Neukum <oneukum@...e.de>
To: stefani@...bold.net
Cc: linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org,
alan@...rguk.ukuu.org.uk, linux-usb@...r.kernel.org
Subject: Re: [PATCH] fix usb skeleton driver
Am Mittwoch, 6. Juni 2012, 15:23:18 schrieb stefani@...bold.net:
> Grek ask me to do this in more pieces, but i will post it for a first RFC.
I've put comments into the code.
> @@ -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);
>
> - 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;
> + }
For an example driver we don't want exclusive open by default.
>
> 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 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;
And here we have a very subtle SMP race
which can be seen only in another place.
> spin_unlock(&dev->err_lock);
>
> complete(&dev->bulk_in_completion);
> @@ -195,7 +211,7 @@ static void skel_read_bulk_callback(struct urb *urb)
>
> 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;
And here we hit the race pointed out above. And this one is for the
gourmets of races. On some architectures we are hitting a memory
ordering race here.
You cannot be sure that dev->errors is valid if ongoing_read == false
because there is no locking involved. The CPU on which the interrupt handler
has run may have scheduled the write to ongoing_read before the write
to dev->error and you can run into the window.
You must use smp_wmb() between the writes and smp_rmb() between
the reads.
(And Greg will come after me for this suggestion and wield his canoo as a cudgel)
> + 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;
> }
>
Regards
Oliver
--
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