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)
