Re: [patch v1 1/2] drivers: jtag: Add JTAG core driver
flat view
From: Arnd Bergmann <arnd@arndb.de>
Date: 2017-08-02 15:37:21
Also in:
linux-arm-kernel, linux-serial, lkml, openbmc
On Wed, Aug 2, 2017 at 3:18 PM, Oleksandr Shamray [off-list ref] wrote:
quoted hunk ↗ jump to hunk
+ +static void *jtag_copy_from_user(void __user *udata, unsigned long bit_size) +{ + void *kdata; + unsigned long size; + unsigned long err; + + size = DIV_ROUND_UP(bit_size, BITS_PER_BYTE); + kdata = kzalloc(size, GFP_KERNEL); + if (!kdata) + return NULL; + + err = copy_from_user(kdata, udata, size); + if (!err) + return kdata; + + kfree(kdata); + return NULL; +}
You can use memdup_user() here to simplify this, or just change the callers to use that directly.
quoted hunk ↗ jump to hunk
+static long jtag_ioctl(struct file *file, unsigned int cmd, unsigned long arg) +{ + struct jtag *jtag = file->private_data; + struct jtag_run_test_idle idle; + struct jtag_xfer xfer; + void *user_tdio_data; + unsigned long value; + int err; + + switch (cmd) { + case JTAG_GIOCFREQ: + if (jtag->ops->freq_get) + err = jtag->ops->freq_get(jtag, &value); + else + err = -EOPNOTSUPP; + if (err) + break; + + err = __put_user(value, (unsigned long __user *)arg); + break;
Use put_user() instead of __put_user() everywhere please. To avoid using so many casts, just use a temporary variable that holds the pointer. Also, you should never use 'unsigned long' pointers in the arguments, use either '__u32' or '__u64', whichever makes more sense here. I see that your command definition has 'unsigned int', so it's already broken on 64-bit architectures.
quoted hunk ↗ jump to hunk
+ case JTAG_IOCXFER: + if (copy_from_user(&xfer, (void __user *)arg, + sizeof(struct jtag_xfer))) + return -EFAULT; + + user_tdio_data = xfer.tdio; + xfer.tdio = jtag_copy_from_user((void __user *)user_tdio_data, + xfer.length); + if (!xfer.tdio) + return -ENOMEM;
You should enforce an upper bound for the length here, to prevent users from draining kernel memory with giant buffers.
quoted hunk ↗ jump to hunk
+static struct jtag *jtag_get_dev(int id) +{ + struct jtag *jtag; + + mutex_lock(&jtag_mutex); + list_for_each_entry(jtag, &jtag_list, list) { + if (jtag->id == id) + goto found; + } + jtag = NULL; +found: + mutex_unlock(&jtag_mutex); + return jtag; +}
I'm pretty sure there is a better way to look up the data from the chardev inode, though I now forget how that is best done.
quoted hunk ↗ jump to hunk
+static const struct file_operations jtag_fops = { + .owner = THIS_MODULE, + .llseek = no_llseek, + .unlocked_ioctl = jtag_ioctl, + .open = jtag_open, + .release = jtag_release, +};
add a compat_ioctl pointer here, after ensuring that all ioctl commands are compatible between 32-bit and 64-bit user space. In turn, no_llseek is the default, you can drop that.
quoted hunk ↗ jump to hunk
+struct jtag *jtag_alloc(size_t priv_size, const struct jtag_ops *ops) +{ + struct jtag *jtag = kzalloc(sizeof(*jtag) + priv_size, GFP_KERNEL); + + if (!jtag) + return NULL; + + jtag->ops = ops; + return jtag; +} +EXPORT_SYMBOL_GPL(jtag_alloc);
Please add some padding behind 'struct jtag' to ensure
the private data is aligned to ARCH_DMA_MINALIGN,
Arnd