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@xxxxxxxxxxxxxxxxxx>
>
> Seems to make a lot of sense to me, patch is Reviewed-by: Christian König
> <christian.koenig@xxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
