On Thu, 2015-03-19 at 03:07 -0700, Keith Packard wrote:
Hi,
unfortunately there are a few issues with this driver.
Please see the comments in the quoted code.
Regards
Oliver
> +#define CK_DEBUG(mask, fmt, args...) do { \
> + if (unlikely(debug & (mask))) \
> + printk(KERN_INFO " chaoskey: %s: %d " fmt, __func__,
> __LINE__, ##args); \
> + } while (0)
> +
> +#define CK_ENTER(mask, what) CK_DEBUG(mask, "Enter " what)
> +#define CK_LEAVE(mask, what,fmt,ret) CK_DEBUG(mask, "Leave " what ":" fmt,
> ret)
> +#define CK_LEAVE_INT(mask, what,ret) CK_LEAVE(mask, what,"%d",ret)
> +#define CK_LEAVE_SIZE(mask, what,ret) CK_LEAVE(mask, what,"%zu",ret)
> +#define CK_LEAVE_SSIZE(mask, what,ret) CK_LEAVE(mask, what,"%zd",ret)
> +#define CK_LEAVE_VOID(mask, what) CK_DEBUG(mask, "Leave " what)
Do we really need yet another version of home grown debug macros?
> +/* Driver-local specific stuff */
> +struct chaoskey {
> + struct usb_device *udev;
> + struct usb_interface *interface;
> + char in_ep;
> + struct mutex lock;
> + struct mutex rng_lock;
> + int open; /* open count */
> + int present; /* device not disconnected */
> + int size; /* size of buf */
> + int valid; /* bytes of buf read */
> + int used; /* bytes of buf consumed */
> + char *name; /* product + serial */
> + struct hwrng hwrng; /* Embedded struct for hwrng */
> + int hwrng_registered; /* registered with hwrng API */
> + wait_queue_head_t wait_q; /* for timeouts */
> + char buf[0];
That is a violation of the DMA rules on non-coherent architectures.
The buffer must be allocated separately.
> +};
> +static int chaoskey_probe(struct usb_interface *interface,
> + const struct usb_device_id *id)
> +{
> + struct usb_device *udev = interface_to_usbdev(interface);
> + struct usb_host_interface *altsetting = interface->cur_altsetting;
> + int i;
> + int in_ep = -1;
> + struct chaoskey *dev;
> + int result;
> + int size;
> +
> + CK_ENTER(DEBUG_INIT, "probe");
> +
> + /* Find the first bulk IN endpoint and its packet size */
> + for (i = 0; i < altsetting->desc.bNumEndpoints; i++) {
> + if (usb_endpoint_is_bulk_in(&altsetting->endpoint[i].desc)) {
> + in_ep = altsetting->endpoint[i].desc.bEndpointAddress;
> + size = altsetting->endpoint[i].desc.wMaxPacketSize;
> + break;
> + }
> + }
> +
> + /* Validate endpoint and size */
> + if (in_ep == -1) {
> + CK_LEAVE_INT(DEBUG_INIT, "probe (ep)", -ENODEV);
> + return -ENODEV;
> + }
> + if (size <= 0) {
> + CK_LEAVE_INT(DEBUG_INIT, "probe (size)", -ENODEV);
> + return -ENODEV;
> + }
> +
> + /* Looks good, allocate and initialize */
> + CK_DEBUG(DEBUG_INIT, "New chaoskey, in_ep %d size %d\n", in_ep, size);
> +
> + dev = kzalloc (sizeof (struct chaoskey) + size, GFP_KERNEL);
> +
You do the test for the plausibility of size after you've used it.
> + if (dev == NULL) {
> + CK_LEAVE_INT(DEBUG_INIT, "probe (dev)", -ENOMEM);
> + return -ENOMEM;
> + }
> +
> + dev->udev = udev;
This can be easily deduced from the interface.
> + dev->interface = interface;
> +
> + dev->in_ep = in_ep;
> +
> + if (size > CHAOSKEY_BUF_LEN) {
> + CK_DEBUG(DEBUG_INIT, "chaoskey size %d reduced to %d\n",
> size,
> + CHAOSKEY_BUF_LEN);
> + size = CHAOSKEY_BUF_LEN;
> + }
> +
> + dev->size = size;
> + dev->present = 1;
> +
> + init_waitqueue_head(&dev->wait_q);
> +
> + usb_set_intfdata(interface, dev);
> +
> + result = usb_register_dev(interface, &chaoskey_class);
> + if (result) {
> + dev_err(&interface->dev, "Unable to allocate minor number.\n");
> + usb_set_intfdata(interface, NULL);
> + chaoskey_free(dev);
> + CK_LEAVE_INT(DEBUG_INIT, "probe (register)", result);
> + return result;
> + }
> +
That is a race condition. At this point the device can be opened and
open() uses the mutexes.
> + mutex_init(&dev->lock);
> + mutex_init(&dev->rng_lock);
Shift this.
> +
> + /* Construct a name using the product and serial values. Each
> + * device needs a unique name for the hwrng code
> + */
> + dev->name = NULL;
> +
> + if (udev->product && udev->serial) {
> + dev->name = kmalloc(strlen(udev->product) + 1 +
> strlen(udev->serial) + 1, GFP_KERNEL);
> + if (dev->name) {
> + strcpy(dev->name, udev->product);
> + strcat(dev->name, "-");
> + strcat(dev->name, udev->serial);
> + }
> + }
> +
> + dev->hwrng.name = dev->name ? dev->name : chaoskey_driver.name;
> + dev->hwrng.read = chaoskey_rng_read;
> +
> + /* Set the 'quality' metric. Quality is measured in units of
> + * 1/1024's of a bit ("mills"). This should be set to 1024,
> + * but there is a bug in the hwrng core which masks it with
> + * 1023.
> + *
> + * The patch that has been merged to the crypto development
> + * tree for that bug limits the value to 1024 at most, so by
> + * setting this to 1024 + 1023, we get 1023 before the fix is
> + * merged and 1024 afterwards. We'll patch this driver once
> + * both bits of code are in the same tree.
> + */
> + dev->hwrng.quality = 1024 + 1023;
> +
> + dev->hwrng_registered = (hwrng_register(&dev->hwrng) == 0);
> + if (!dev->hwrng_registered)
> + CK_DEBUG(DEBUG_INIT, "probe hwrng register failed: %d\n",
> result);
> +
> + usb_enable_autosuspend(udev);
> +
> + CK_LEAVE_INT(DEBUG_INIT, "probe", 0);
> + return 0;
> +}
> +
> +static ssize_t chaoskey_read(struct file *file,
> + char __user *buffer,
> + size_t count,
> + loff_t * ppos)
> +{
> + struct chaoskey *dev;
> + int intr;
> + ssize_t read_count = 0;
> + int this_time;
> + int result;
> +
> + CK_ENTER(DEBUG_DEV, "read");
> + dev = file->private_data;
> +
> + if (dev == NULL || !dev->present) {
> + CK_LEAVE_INT(DEBUG_DEV, "read (present)", -ENODEV);
> + return -ENODEV;
> + }
> +
> +
> + CK_DEBUG(DEBUG_DEV, "read %zu\n", count);
> +
> + while (count > 0) {
> +
> + /* Grab the rng_lock briefly to ensure that the hwrng interface
> + * gets priority over other user access
> + */
> + intr = mutex_lock_interruptible(&dev->rng_lock);
> + if (intr) {
> + CK_LEAVE_INT(DEBUG_DEV, "read (rng_lock)", -EINTR);
> + return -EINTR;
At this point you may have already copied data to user space. A signal
should cause a short read in that case, not -EINTR.
Also the mutex is still held.
> + }
> + mutex_unlock(&dev->rng_lock);
> +
> + intr = mutex_lock_interruptible(&dev->lock);
> + if (intr) {
> + CK_LEAVE_INT(DEBUG_DEV, "read (lock)", -EINTR);
The same issue as above.
> + return -EINTR;
> + }
> + if (dev->valid == dev->used) {
> + result = _chaoskey_fill(dev);
> + if (result) {
> + if (read_count) {
> + CK_LEAVE_SIZE(DEBUG_DEV, "read (count)",
> + read_count);
> + return read_count;
The mutex is still held.
> + }
> + CK_LEAVE_INT(DEBUG_DEV, "read (result)",
> result);
> + return result;
The mutex is still held.
> + }
> +
> + /* Read returned zero bytes */
> + if (dev->used == dev->valid) {
> + CK_LEAVE_SSIZE(DEBUG_DEV, "read (zero)",
> read_count);
> + return read_count;
> + }
> + }
> +
> + this_time = dev->valid - dev->used;
> + if (this_time > count)
> + this_time = count;
> +
> + if (copy_to_user(buffer, dev->buf + dev->used, this_time)) {
> + mutex_unlock(&dev->lock);
> + CK_LEAVE_INT(DEBUG_DEV, "read (copyout)", -EFAULT);
> + return -EFAULT;
> + }
> +
> + count -= this_time;
> + read_count += this_time;
> + buffer += this_time;
> + dev->used += this_time;
> + mutex_unlock(&dev->lock);
> + }
> +
> + CK_LEAVE_SSIZE(DEBUG_DEV, "read", read_count);
> + return read_count;
> +}
> +
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
