Re: [PATCH 1/1] Add mkopci driver

From: Greg KH
Date: Thu Mar 26 2015 - 18:47:43 EST

On Fri, Mar 20, 2015 at 03:10:26PM +0300, sergej.bauer@xxxxxxxxx wrote:
> mkopci (MB11.xx) device (RC Module project) provides data transference through a serial bus bar according to MIL-STD-1553.
> the driver used for operating devices, reads PCI configuration space and pass interrupts to user-space applications.

Why can't this be a UIO driver? Isn't that what the UIO interface
already does?

>
> Please consider adding this patch to the linux-next queue.
>
> Signed-off-by: Sergej Bauer <sergej.bauer@xxxxxxxxx>
> ---
> Documentation/misc-devices/mkopci.txt | 51 ++
> drivers/misc/Kconfig | 9 +
> drivers/misc/Makefile | 1 +
> drivers/misc/mkopci.c | 1272 +++++++++++++++++++++++++++++++++
> include/misc/mkopci.h | 81 +++
> 5 files changed, 1414 insertions(+)
> create mode 100644 Documentation/misc-devices/mkopci.txt
> create mode 100644 drivers/misc/mkopci.c
> create mode 100644 include/misc/mkopci.h
> diff --git a/Documentation/misc-devices/mkopci.txt b/Documentation/misc-devices/mkopci.txt
> new file mode 100644
> index 0000000..0fcec6d
> --- /dev/null
> +++ b/Documentation/misc-devices/mkopci.txt
> @@ -0,0 +1,51 @@
> + PCI-based MKO bus driver.
> +
> +
> +For dealing with driver without using of root's account it will be helpfull
> +to add group `mkopci' with appropriate users and put file, say 60-mkopci.rules to
> +/etc/udev/rules.d in your system.
> +--- cut 60-mkopci.rules ---
> +# MKO devices
> +KERNEL=="mkopci*", SUBSYSTEM=="mkopci", ACTION=="add", DRIVERS=="?*", ATTRS{idVendor}=="0x6403"
> +GROUP="mkopci"
> +---
> +
> +
> +Kernel module parameters
> +
> +Kernel module can take parameters 'v' and 'omited'
> +- 'v' is 0 to 3, and affects the amount of information
> +output to the system log.
> +0 - (default) comletely silent
> +1 - prints only detected BARs, reports loading / unloading
> +2 - + prints IRQ and memory mapping events
> +3 - + prints ioctl events

Please use the built-in dynamic debugging levels instead of custom ones
like this.

> +
> +- 'plx9050bug_quirk' controls workaround controller PLX9050. Can
> +the following values:
> +0 - workaround is disabled (default)
> +1 - workaround is enabled
> +2 - forced bug
> +
> +- 'omited' tells the driver which device should not be initialized
> +at load time.
> +The value of this parameter to the kernel module 2.4 is a physical address
> +device on the bus, such as "0x10800" without the quotes.
> +In kernel versions 2.6+ can exclude multiple devices, transferring them
> +values separated by commas, but not more than 4.
> +
> +The parameter values can be specified as follows:
> +$ insmod/modprobe mkopci.[ko/o] parameter1_name=value [parameter2_name=value]
> +
> +Record the physical address of the device in the file /proc/mkopci/core
> +also controls "visibility" for user programs. Missed return device
> +You can command 'echo ADDR > /proc/mkopci/core'. ADDR can be either a simple
> +number of device, but always in hexadecimal, or (for Linux-2.6+)
> +the number of devices in a standard format Linux kind NM:XY.z.
> +
> +
> +PROC filesystem
> +
> +/proc/mkopci/devices - read only text device table
> +/proc/mkopci/core - device table for user space applications

As others have said, proc is not for devices, we can't take new drivers
that use this, sorry.

> +
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 006242c..7db247e 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -124,6 +124,15 @@ config PHANTOM
> If you choose to build module, its name will be phantom. If unsure,
> say N here.
>
> +config MKOPCI

Trailing whitespace :(

> + tristate "Module PCI bus driver"
> + depends on PCI && PROC_FS
> + help
> + Say Y here if you want to build a driver for Module(RC) MKOPCI devices.
> +
> + If you choose to build module, its name will be mkopci. If unsure,
> + say N here.
> +
> config INTEL_MID_PTI
> tristate "Parallel Trace Interface for MIPI P1149.7 cJTAG standard"
> depends on PCI && TTY && (X86_INTEL_MID || COMPILE_TEST)
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 7d5c4cd..afb92b4 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -56,3 +56,4 @@ obj-$(CONFIG_GENWQE) += genwqe/
> obj-$(CONFIG_ECHO) += echo/
> obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
> obj-$(CONFIG_CXL_BASE) += cxl/
> +obj-$(CONFIG_MKOPCI) += mkopci.o
> diff --git a/drivers/misc/mkopci.c b/drivers/misc/mkopci.c
> new file mode 100644
> index 0000000..d223478
> --- /dev/null
> +++ b/drivers/misc/mkopci.c
> @@ -0,0 +1,1272 @@
> +/*
> + * MKOPCI driver
> + *
> + * Copyright (C) 2007-2015 Sergej Bauer <sergej.bauer@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, as published by
> + * the Free Software Foundation, version 2.
> +*/
> +
> +#include <linux/version.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/proc_fs.h>
> +#include <linux/seq_file.h>
> +#include <linux/pci.h>
> +#include <linux/fs.h>
> +#include <linux/mm.h>
> +#include <linux/sched.h>
> +#include <linux/delay.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/interrupt.h>
> +#include <linux/cdev.h>
> +#include <linux/uaccess.h>
> +#include <linux/io.h>
> +
> +#include "misc/mkopci.h"
> +
> +#define MKO_VENDOR 0x6403
> +#define MKO_DEVICE1 0x0430
> +#define MKO_DEVICE2 0x0431
> +#define MKO_DEVICE3 0x0434
> +
> +#define PLX9050BUG_BAR0 0x1
> +#define PLX9050BUG_BAR1 0x2
> +#define PLX9050BUG_INJECT 0x4
> +
> +#if !defined(__user)
> +#define __user
> +#endif

Why is this needed?

> +
> +static struct pci_device_id ids[] = {
> + {MKO_VENDOR, MKO_DEVICE1, PCI_ANY_ID, PCI_ANY_ID,},
> + {MKO_VENDOR, MKO_DEVICE2, PCI_ANY_ID, PCI_ANY_ID,},
> + {MKO_VENDOR, MKO_DEVICE3, PCI_ANY_ID, PCI_ANY_ID,},
> + {0, 0,}
> +};
> +
> +MODULE_DEVICE_TABLE(pci, ids);
> +
> +#ifdef __LP64__
> +#define PFMT "llx"
> +#else
> +#define PFMT "x"
> +#endif

Are you sure you need this? We have data types to handle this
already...

> +
> +unsigned char drv_version = 0x11;

Global variables?

> +#define mko_pci_addr(bus, device, func, regoffs) (\
> + ((bus & 0xFF) << 16) | ((device & 0x1F) << 11) | \
> + ((func & 0x7) << 8) | (regoffs & 0xFC))
> +
> +static struct kmem_cache *mkopci_device_cache;
> +static dev_t devp;
> +static struct class *mkopci_class;
> +static struct rw_semaphore devices_sem;
> +static LIST_HEAD(devices);
> +static atomic_t devices_nr = ATOMIC_INIT(0);
> +
> +/*** module parameters ***/
> +static int plx9050bug_quirk = 1;
> +/* verbosity level */
> +static int v;
> +module_param(plx9050bug_quirk, int, S_IRUGO);
> +module_param(v, int, S_IRUGO);
> +MODULE_PARM_DESC(plx9050bug_quirk, "PLX9050 bug quirk");
> +MODULE_PARM_DESC(v, "verbosity level");
> +
> +/* only MAX_DEVICES_NR devices at once can be omited */
> +static int omited[MAX_DEVICES_NR];
> +static int omited_nr;
> +module_param_array(omited, int, &omited_nr, S_IRUGO);
> +MODULE_PARM_DESC(omited, "device(s) to omit");
> +#ifndef VM_RESERVED
> +#define VM_RESERVED (VM_DONTEXPAND | VM_DONTDUMP)
> +#endif

Why is this needed?

> +#define __unused __attribute__ ((unused))

We already have this.

> +
> +/***************************** Interrupt handling *****************************/
> +static int mkopci_wait_irq(struct mkopci_device *dev)
> +{
> + volatile unsigned long *LPCI_4C =
> + (unsigned long *)(dev->core.lin_base[0] + 0x4C);
> +
> + *LPCI_4C |= 0x49;
> +
> + if (wait_event_interruptible(dev->wq, *LPCI_4C & 0x24))
> + return -ERESTARTSYS;
> + if (v > 1)
> + pr_info("mkopci%d: irq received\n", dev->core.n_dev);
> + *LPCI_4C &= ~0x40;

You can't directly access memory like this safely, please use the proper
io memory functions instead, otherwise bad things are guaranteed to
happen.

Hint, volatile doesn't mean what you think it does here...

> +
> + return 0;
> +}
> +
> +static irqreturn_t mkopci_int_handler(int __unused irq, void *dev)
> +{
> + struct mkopci_device *mko_dev = (struct mkopci_device *)dev;
> + volatile unsigned long *LPCI_4C =
> + (unsigned long *)((mko_dev->core.lin_base[0] + 0x4C));

Same here, don't use this.

> +
> + if (*LPCI_4C & 0x24) {
> + *LPCI_4C &= ~0x40;

That breaks on lots of architectures :(

> + wake_up_interruptible(&mko_dev->wq);
> + } else
> + return IRQ_NONE;
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int mkopci_request_irq(struct mkopci_device *dev)
> +{
> + int ret = 0;
> +
> + if (dev->core.irq_requested) {
> + pr_err("mkopci%d: irq already requested\n", dev->core.n_dev);
> + return -EBUSY;
> + }
> + ret = request_irq(dev->core.irq, &mkopci_int_handler, IRQF_SHARED,
> + dev->core.name, dev);
> + if (ret) {
> + pr_err("mkopci%d: failed to request irq %d\n", dev->core.n_dev,
> + dev->core.irq);
> + return ret;
> + }
> +
> + dev->core.irq_requested++;
> +
> + if (strcmp(current->comm, "TestSetISR") && v > 1) {
> + pr_info("mkopci%d: irq %d requested (%s)\n", dev->core.n_dev,
> + dev->core.irq, current->comm);
> + }
> +
> + return ret;
> +}
> +
> +static void mkopci_free_irq(struct mkopci_device *dev)
> +{
> + if (dev->core.irq_requested) {
> + synchronize_irq(dev->core.irq);
> + free_irq(dev->core.irq, dev);
> + dev->core.irq_requested--;
> +
> + if (strcmp(current->comm, "TestSetISR") && v > 1) {
> + pr_info("mkopci%d: irq %d released\n", dev->core.n_dev,
> + dev->core.irq);
> + }
> + }
> +}
> +/************************* End of Interrupt handling **************************/
> +
> +/**************************** File operations *********************************/
> +static int mkopci_open(struct inode *inode, struct file *filp)
> +{
> + int ret = 0;
> + struct mkopci_device *dev =
> + container_of(inode->i_cdev, struct mkopci_device, cdev);
> +
> + if (!try_module_get(THIS_MODULE))
> + return -EINVAL;

That's racy, and not needed, and should never be in any kernel code
ever. Please remove.

> +
> + if (dev == NULL) {
> + module_put(THIS_MODULE);
> + return -ENODEV;
> + }
> +
> + down_write(&dev->rwsem);
> + if (dev->omited) {
> + ret = -ENODEV;
> + goto err;
> + }
> + if (!(filp->f_flags & O_NONBLOCK)) {
> + if (dev->core.process) {
> + ret = -EBUSY;
> + goto err;
> + }
> + dev->backdoor = 0;
> + dev->core.process = current->pid;
> + } else {
> + dev->backdoor = current->pid;

This is just funny :)

It also doesn't handle pid namespaces at all :(

> + dev->core.process = 0;
> + }
> +
> + filp->private_data = dev;
> +
> + if (strcmp(current->comm, "TstOpenClose") && v > 1)

This also is pretty funny, please never do this.

> + pr_info("mkopci%d: device opened in %s mode (%s)\n",
> + dev->core.n_dev, dev->backdoor ? "backdoor" : "regular",
> + current->comm);

drivers should always use dev_info() and friends, not pr_info().

> + goto out;
> +
> +err:
> + module_put(THIS_MODULE);
> +out:
> + up_write(&dev->rwsem);
> +
> + return ret;
> +}
> +
> +static int mkopci_release(struct inode __unused *inode, struct file *filp)
> +{
> + struct mkopci_device *dev;
> +
> + if (filp->private_data == NULL)
> + return 0;
> +
> + dev = (struct mkopci_device *)filp->private_data;
> + down_write(&dev->rwsem);
> + dev->core.process = 0;
> + dev->backdoor = 0;
> + dev->core.c_bar = -1;
> + filp->private_data = NULL;
> + mkopci_free_irq(dev);
> +
> + if (strcmp(current->comm, "TstOpenClose") && v > 1)

Also funny :)

> + pr_info("mkopci%d: device released\n", dev->core.n_dev);
> + up_write(&dev->rwsem);
> + module_put(THIS_MODULE);
> +
> + return 0;
> +}
> +
> +static long mkopci_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> + int ret = 0, n = 0;
> + struct mkopci_device *dev =
> + (struct mkopci_device *)filp->private_data, *d;
> +
> + if (_IOC_TYPE(cmd) != MKO_IOC_MAGIC) {
> + pr_err("mkopci%d: _IOC_TYPE(cmd) != MKO_IOC_MAGIC\n",
> + dev->core.n_dev);
> + return -ENOTTY;
> + }
> +
> + if (_IOC_NR(cmd) > MKO_IOC_MAXNR) {
> + pr_err("mkopci%d: _IOC_NR(cmd) > MKO_IOC_MAXNR\n",
> + dev->core.n_dev);
> + return -ENOTTY;
> + }
> +
> + if (_IOC_DIR(cmd) & _IOC_READ) {
> + ret =
> + !access_ok(VERIFY_WRITE, (void __user *)arg,
> + _IOC_SIZE(cmd));
> + } else if (_IOC_DIR(cmd) & _IOC_WRITE) {
> + ret =
> + !access_ok(VERIFY_READ, (void __user *)arg, _IOC_SIZE(cmd));
> + }

We never need to do this type of checking anymore, it's useless now.
This must have been written originally for a very old kernel version.

> +
> + if (ret) {
> + pr_err("mkopci%d: mkopci_ioctl: access_ok failed\n",
> + dev->core.n_dev);
> + return -EIO;
> + }
> +
> + switch (cmd) {
> + case MKOPCI_IOCTL_CWPID:
> + if (v > 2)
> + pr_info("mkopci%d: MKOPCI_IOCTL_CWPID ioctl\n",
> + dev->core.n_dev);
> + down_read(&dev->rwsem);
> + ret = dev->core.process;

Again, broken with pid namespaces :(

> + up_read(&dev->rwsem);
> + break;
> + case MKOPCI_IOCTL_GET_VERSION:
> + if (v > 2)
> + pr_info("mkopci%d: MKOPCI_IOCTL_GET_VERSION ioctl\n",
> + dev->core.n_dev);
> + ret = put_user(drv_version, (int __user *)arg);

version can go lots of other places, it doesn't have to be an ioctl.

> + break;
> + case MKOPCI_IOCTL_GET_BOARDS_COUNT:
> + if (v > 2)
> + pr_info
> + ("mkopci%d: MKOPCI_IOCTL_GET_BOARDS_COUNT ioctl\n",
> + dev->core.n_dev);
> + down_read(&dev->rwsem);
> + ret =
> + put_user((unsigned short)atomic_read(&devices_nr),
> + (int __user *)arg);

You can read this today from sysfs, no need for an ioctl.

> + up_read(&dev->rwsem);
> + break;
> + case MKOPCI_IOCTL_GET_DEVICE_TABLE:
> + if (v > 2)
> + pr_info
> + ("mkopci%d: MKOPCI_IOCTL_GET_DEVICE_TABLE ioctl\n",
> + dev->core.n_dev);
> + down_read(&devices_sem);
> + if (put_user
> + ((unsigned short)atomic_read(&devices_nr),
> + (int __user *)arg)) {
> + up_read(&devices_sem);
> + ret = -ERESTARTSYS;
> + break;

Don't use put_user() if at all possible. Use copy_to_user() please.

> + }
> + list_for_each_entry(d, &devices, list) {
> + down_read(&d->rwsem);
> + if (d->omited) {
> + up_read(&d->rwsem);
> + continue;
> + }
> + if (copy_to_user
> + ((void __user *)arg + sizeof(unsigned short) +
> + n * sizeof(struct mkopci_core), &d->core,
> + sizeof(struct mkopci_core))) {
> + up_read(&d->rwsem);
> + ret = -ERESTARTSYS;

Wrong error value.

> + break;
> + }
> + up_read(&d->rwsem);
> + n++;
> + }
> + up_read(&devices_sem);
> + break;
> + case MKOPCI_IOCTL_ATTACH_IRQ:
> + if (v > 2)
> + pr_info("mkopci%d: MKOPCI_IOCTL_ATTACH_IRQ ioctl\n",
> + dev->core.n_dev);
> + down_write(&dev->rwsem);
> + ret = mkopci_request_irq(dev);

Not a good idea. Look at how UIO does this instead.

> + up_write(&dev->rwsem);
> + break;
> + case MKOPCI_IOCTL_DETACH_IRQ:
> + if (v > 2)
> + pr_info("mkopci%d: MKOPCI_IOCTL_DETACH_IRQ ioctl\n",
> + dev->core.n_dev);
> + down_write(&dev->rwsem);
> + mkopci_free_irq(dev);
> + up_write(&dev->rwsem);
> + break;
> + case MKOPCI_IOCTL_WAIT_IRQ:
> + if (v > 2)
> + pr_info("mkopci%d: MKOPCI_IOCTL_WAIT_IRQ ioctl\n",
> + dev->core.n_dev);
> + ret = mkopci_wait_irq(dev);
> + break;
> + case MKOPCI_IOCTL_REQUEST_BAR:
> + if (v > 2)
> + pr_info("mkopci%d: MKOPCI_IOCTL_REQUEST_BAR ioctl\n",
> + dev->core.n_dev);
> + down_write(&dev->rwsem);
> + ret = get_user(dev->core.c_bar, (int __user *)arg);
> + up_write(&dev->rwsem);
> + break;
> + default:
> + pr_err("mkopci%d: invalid ioctl %d\n", dev->core.n_dev,
> + _IOC_NR(cmd));
> + ret = -EINVAL;
> + break;
> + };
> +
> + return ret;
> +}
> +
> +#ifndef pgprot_noncached
> +static inline pgprot_t pgprot_noncached(pgprot_t _prot)
> +{
> + unsigned long prot = pgprot_val(_prot);
> +
> + if (boot_cpu_data.x86 > 3)
> + prot |= _PAGE_PCD | _PAGE_PWT;
> +
> + return __pgprot(prot);
> +}
> +#endif
> +
> +static int mkopci_mmap(struct file *filp, struct vm_area_struct *vma)
> +{
> + struct mkopci_device *dev = filp->private_data;
> + unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
> +
> + if (dev->core.c_bar == -1) {
> + pr_err("mkopci%d: invalid c_bar number\n", dev->core.n_dev);
> + return -EINVAL;
> + }
> +
> + if (vma->vm_pgoff != 0) {
> + pr_err("mkopci%d: vma->vm_pgoff != 0, aborting mapping\n",
> + dev->core.n_dev);
> + return -EINVAL;
> + }
> +
> + if (PAGE_ALIGN(dev->core.mem_size[dev->core.c_bar]) !=
> + PAGE_ALIGN(vma->vm_end - vma->vm_start)) {
> + pr_err("mkopci%d: PAGE_ALIGN error\n", dev->core.n_dev);
> + return -EINVAL;
> + }
> +
> + if (offset >= __pa(high_memory) || (filp->f_flags & O_SYNC))
> + vma->vm_flags |= VM_IO;
> + vma->vm_flags |= VM_RESERVED | VM_SHARED;
> +
> + if (v > 1 && strcmp("TstOpenClose", current->comm)) {
> + pr_info
> + ("mkopci%d: .mmap BAR%d: [0x%lx - 0x%lx], vma [0x%lx - 0x%lx]\n",
> + dev->core.n_dev, dev->core.c_bar,
> + dev->core.mem_base[dev->core.c_bar],
> + dev->core.mem_base[dev->core.c_bar] +
> + dev->core.mem_size[dev->core.c_bar], vma->vm_start,
> + vma->vm_end);
> + }
> +
> + if (remap_pfn_range
> + (vma, vma->vm_start,
> + virt_to_phys(bus_to_virt(dev->core.mem_base[dev->core.c_bar])) >>
> + PAGE_SHIFT, dev->core.mem_size[dev->core.c_bar],
> + pgprot_noncached(vma->vm_page_prot))) {
> + pr_err("mkopci%d: memory remapping failed\n", dev->core.n_dev);
> + return -EAGAIN;
> + }
> +
> + return 0;
> +}

Again, UIO does this already for you, please move to that framework
instead. I can't take a duplicate one into the kernel tree.

Especially one with these fun security holes :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/