RE: [PATCH v2 3/5] IB/hfi1: Add ioctl() interface for user commands

[Hefty, Sean]

> On Thu, May 12, 2016 at 10:18:47AM -0700, Dennis Dalessandro wrote:
> > +	case HFI1_IOCTL_EP_INFO:
> > +	case HFI1_IOCTL_EP_ERASE_CHIP:
> > +	case HFI1_IOCTL_EP_ERASE_RANGE:
> > +	case HFI1_IOCTL_EP_READ_RANGE:
> > +	case HFI1_IOCTL_EP_WRITE_RANGE:
> > +		if (!capable(CAP_SYS_ADMIN))
> > +			return -EPERM;
> > +		if (copy_from_user(&ucmd,
> > +				   (struct hfi11_cmd __user *)arg,
> > +				   sizeof(ucmd)))
> > +			return -EFAULT;
> > +		return handle_eprom_command(fp, &ucmd);
> 
> I thought we agreed to get rid of this as well? It certainly does not
> belong here, and as a general rule, I don't think ioctls should be
> doing capable tests..

At least the drm ioctl code has similar capable test

http://lxr.free-electrons.com/source/drivers/gpu/drm/drm_ioctl.c#L519


> > +static inline int check_ioctl_access(unsigned int cmd, unsigned long
> arg)
> > +{
> > +	int read_cmd, write_cmd, read_ok, write_ok;
> > +
> > +	read_cmd = _IOC_DIR(cmd) & _IOC_READ;
> > +	write_cmd = _IOC_DIR(cmd) & _IOC_WRITE;
> > +	write_ok = access_ok(VERIFY_WRITE, (void __user *)arg,
> _IOC_SIZE(cmd));
> > +	read_ok = access_ok(VERIFY_READ, (void __user *)arg, _IOC_SIZE(cmd));
> > +
> > +	if ((read_cmd && !write_ok) || (write_cmd && !read_ok))
> > +		return -EFAULT;
> 
> This seems kind of goofy, didn't Ira say this is performance senstive?
> 
> Driver shouldn't be open coding __get_user like that, IMHO.

FWIW, drm keeps an ioctl 'descriptor', which maintains a kernel copy of the ioctl cmd.  It uses the kernel's version of the cmd for processing, instead of the cmd value passed in from user space.

It doesn't open code get_user or do checks similar to what's here.  But if there's concern that the cmd value cannot be trusted, a similar descriptor mechanism could be used here.

- Sean