[PATCH] drm: Don't overwrite user ioctl arg unless requested

[Daniel Vetter]

On Thu, Jul 14, 2016 at 09:46:24AM +0200, Christian König wrote:
> Am 12.07.2016 um 16:59 schrieb Chris Wilson:
> > Currently, we completely ignore the user when it comes to the in/out
> > direction of the ioctl argument, as we simply cannot trust userspace.
> > (For example, they might request a copy of the modified ioctl argument
> > when the driver is not expecting such and so leak kernel stack.)
> > However, blindly copying over the target address may also lead to a
> > spurious EFAULT, and a failure after the ioctl was completed
> > successfully. This is important in order to avoid an ABI break when
> > extending an ioctl from IOR to IORW. Similar to how we only copy the
> > intersection of the kernel arg size and the user arg size, we only want
> > to copy back the kernel arg data iff both the kernel and userspace
> > request the copy.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> 
> Seems to make a lot of sense to me, patch is Reviewed-by: Christian König
> <christian.koenig at amd.com>

Applied to drm-misc, thanks.
-Daniel

> 
> Regards,
> Christian.
> 
> > ---
> >   drivers/gpu/drm/drm_ioctl.c | 50 ++++++++++++++++++++-------------------------
> >   1 file changed, 22 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index 2c87c1df0910..33af4a5ddca1 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -648,7 +648,7 @@ long drm_ioctl(struct file *filp,
> >   	int retcode = -EINVAL;
> >   	char stack_kdata[128];
> >   	char *kdata = NULL;
> > -	unsigned int usize, asize, drv_size;
> > +	unsigned int in_size, out_size, drv_size, ksize;
> >   	bool is_driver_ioctl;
> >   	dev = file_priv->minor->dev;
> > @@ -671,9 +671,12 @@ long drm_ioctl(struct file *filp,
> >   	}
> >   	drv_size = _IOC_SIZE(ioctl->cmd);
> > -	usize = _IOC_SIZE(cmd);
> > -	asize = max(usize, drv_size);
> > -	cmd = ioctl->cmd;
> > +	out_size = in_size = _IOC_SIZE(cmd);
> > +	if ((cmd & ioctl->cmd & IOC_IN) == 0)
> > +		in_size = 0;
> > +	if ((cmd & ioctl->cmd & IOC_OUT) == 0)
> > +		out_size = 0;
> > +	ksize = max(max(in_size, out_size), drv_size);
> >   	DRM_DEBUG("pid=%d, dev=0x%lx, auth=%d, %s\n",
> >   		  task_pid_nr(current),
> > @@ -693,30 +696,24 @@ long drm_ioctl(struct file *filp,
> >   	if (unlikely(retcode))
> >   		goto err_i1;
> > -	if (cmd & (IOC_IN | IOC_OUT)) {
> > -		if (asize <= sizeof(stack_kdata)) {
> > -			kdata = stack_kdata;
> > -		} else {
> > -			kdata = kmalloc(asize, GFP_KERNEL);
> > -			if (!kdata) {
> > -				retcode = -ENOMEM;
> > -				goto err_i1;
> > -			}
> > +	if (ksize <= sizeof(stack_kdata)) {
> > +		kdata = stack_kdata;
> > +	} else {
> > +		kdata = kmalloc(ksize, GFP_KERNEL);
> > +		if (!kdata) {
> > +			retcode = -ENOMEM;
> > +			goto err_i1;
> >   		}
> > -		if (asize > usize)
> > -			memset(kdata + usize, 0, asize - usize);
> >   	}
> > -	if (cmd & IOC_IN) {
> > -		if (copy_from_user(kdata, (void __user *)arg,
> > -				   usize) != 0) {
> > -			retcode = -EFAULT;
> > -			goto err_i1;
> > -		}
> > -	} else if (cmd & IOC_OUT) {
> > -		memset(kdata, 0, usize);
> > +	if (copy_from_user(kdata, (void __user *)arg, in_size) != 0) {
> > +		retcode = -EFAULT;
> > +		goto err_i1;
> >   	}
> > +	if (ksize > in_size)
> > +		memset(kdata + in_size, 0, ksize - in_size);
> > +
> >   	/* Enforce sane locking for kms driver ioctls. Core ioctls are
> >   	 * too messy still. */
> >   	if ((drm_core_check_feature(dev, DRIVER_MODESET) && is_driver_ioctl) ||
> > @@ -728,11 +725,8 @@ long drm_ioctl(struct file *filp,
> >   		mutex_unlock(&drm_global_mutex);
> >   	}
> > -	if (cmd & IOC_OUT) {
> > -		if (copy_to_user((void __user *)arg, kdata,
> > -				 usize) != 0)
> > -			retcode = -EFAULT;
> > -	}
> > +	if (copy_to_user((void __user *)arg, kdata, out_size) != 0)
> > +		retcode = -EFAULT;
> >         err_i1:
> >   	if (!ioctl)
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch