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@chris-wilson.co.uk --- 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)