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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 12 May 2016 18:12:36 +0000
From:	"Hefty, Sean" <sean.hefty@...el.com>
To:	Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
	"Dalessandro, Dennis" <dennis.dalessandro@...el.com>
CC:	"dledford@...hat.com" <dledford@...hat.com>,
	"Marciniszyn, Mike" <mike.marciniszyn@...el.com>,
	"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
	"Haralanov, Mitko" <mitko.haralanov@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	"Weiny, Ira" <ira.weiny@...el.com>
Subject: RE: [PATCH v2 3/5] IB/hfi1: Add ioctl() interface for user commands

> 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