On Wed, Aug 2, 2017 at 3:18 PM, Oleksandr Shamray
<oleksandrs@xxxxxxxxxxxx> wrote:
> +
> +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.
> +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.
> + 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.
> +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.
> +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.
> +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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
