Re: [PATCH][RFC 2] char: Add Dell Systems Management Base driver

[Greg KH]

On Sun, Jun 26, 2005 at 06:05:44PM -0500, Doug Warzecha wrote:
> This patch adds the Dell Systems Management Base driver.
> 
> The Dell Systems Management Base driver is a character driver that
> implements ioctls for Dell systems management software to use to
> communicate with the driver.

Ugh.  Why new ioctls?  Can't most of these be done other ways?  Like
sysfs or open/write/read/close?

Adding new ioctls is generally frowned apon greatly.

> +static struct pci_dev tvm_dev;

Woah, you are creating your own pci_dev?  No.  Only the pci core can do
that.  Lots of bad things happen if you are doing this.  I'm amazed that
your code even works this way, please fix it.

> +	pr_debug("%s: phys: %x size: %u\n",
> +		__FUNCTION__, tvm_dma_buf_phys_addr, tvm_dma_buf_size);

Use dev_dbg() and friends instead of pr_debug().  Also, use dev_err()
and dev_warn() for your other calls to printk().

> +	case ESM_TVM_READ_MEM:
> +		if (down_interruptible(&tvm_lock))
> +			return -ERESTARTSYS;
> +		ireq->hdr.status = tvm_read_dma_buf(&ireq->data.tvm_mem_read);
> +		up(&tvm_lock);
> +		break;
> +
> +	case ESM_TVM_WRITE_MEM:
> +		if (down_interruptible(&tvm_lock))
> +			return -ERESTARTSYS;
> +		ireq->hdr.status = tvm_write_dma_buf(&ireq->data.tvm_mem_write);
> +		up(&tvm_lock);
> +		break;

Can't these two calls be done by lookin in the existing /proc/iomem
file?  Or, if not that one, some other /proc/*mem file?  Not sure
exactly what you are doing with this read/write stuff...

> +	case ESM_TVM_ALLOC_MEM:
> +		if (down_interruptible(&tvm_lock))
> +			return -ERESTARTSYS;

Why use down_interruptible() everywhere?  Can this stuff be called from
an interrupt somehow?

> +static int dcdbas_do_ioctl(struct file *filp, unsigned int cmd,
> +			   unsigned long arg)
> +{
> +	struct dcdbas_ioctl_req *ubuf = (struct dcdbas_ioctl_req *)arg;
> +	struct dcdbas_ioctl_req *kbuf = NULL;
> +	struct dcdbas_ioctl_hdr hdr;
> +	unsigned long size;
> +	int ret;

So, any user can do any of these ioctl calls?  Isn't that a little bit
insecure?

Also, please run this through sparse and fix the warnings it give you.

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/