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

[Christian König]

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>

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)