Re: is avoiding compat ioctls possible?

Arnd Bergmann
2009-10-30T10:13:56Z

On Friday 30 October 2009, Dave Airlie wrote:
> Btw when I mentioned ioctls I meant more than radeon, all the KMS
> ioctls in the common drm_crtc.c file suffer from this problem as well.
>
> Hence why I still believe either my drm specific inline or something
> more generic (granted I can see why a generic solution would be ugly).
>
> You patch below does suffer from a lot of #ifdefs and cut-n-paste
> that is a lot better suited to doing in an inline or macro. We can then
> comment that inline saying if anyone else does this we will be most
> unhappy.

I think it would be better to do a conversion of the pointers in
a separate compat handler, but then call the regular function, which
in case of drm already take a kernel pointer. That would be much simpler
than the compat_alloc_user_space tricks in the current code and
also cleaner than trying to handle both cases in one function using
is_compat_task().

See the (not even compile-tested) example below as an illustration
of what I think you should do. If you convert all functions in
drm_ioc32.c to this scheme, you can replace drm_compat_ioctl and
drm_compat_ioctls[] with drm_generic_compat_ioctl.

Arnd <><

diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c
index 282d9fd..334345b 100644
--- a/drivers/gpu/drm/drm_ioc32.c
+++ b/drivers/gpu/drm/drm_ioc32.c
@@ -1040,6 +1040,122 @@ static int compat_drm_wait_vblank(struct file *file, unsigned int cmd,
return 0;
}

+static int compat_drm_mode_getblob_ioctl(struct drm_device *dev,
+ struct drm_mode_get_blob __user *out_resp_user,
+ struct drm_file *file_priv)
+{
+ struct drm_mode_get_blob out_resp;
+ int ret;
+
+ ret = copy_from_user(&out_resp, out_resp_user, sizeof(out_resp))
+ if (ret)
+ return -EFAULT;
+
+ out_resp.data = (unsigned long)compat_ptr(out_resp.data);
+
+ ret = drm_mode_getblob_ioctl(dev, &out_resp, file_priv);
+ if (ret)
+ return ret;
+
+ ret = copy_to_user(out_resp_user, &out_resp, sizeof(out_resp))
+ if (ret)
+ return -EFAULT;
+ return 0;
+}
+
+static int compat_drm_mode_gamma_set_ioctl(struct drm_device *dev,
+ struct drm_mode_crtc_lut __user *crtc_lut_user,
+ struct drm_file *file_priv)
+{
+ struct drm_mode_crtc_lut crtc_lut;
+
+ ret = copy_from_user(&crtc_lut, crtc_lut_user, sizeof(crtc_lut))
+ if (ret)
+ return -EFAULT;
+
+ crtc_lut.red = (unsigned long)compat_ptr(crtc_lut.red);
+ crtc_lut.green = (unsigned long)compat_ptr(crtc_lut.green);
+ crtc_lut.blue = (unsigned long)compat_ptr(crtc_lut.blue);
+
+ ret = drm_mode_gamma_set_ioctl(dev, &crtc_lut, file_priv);
+
+ return ret;
+}
+
+static int compat_drm_mode_gamma_get_ioctl(struct drm_device *dev,
+ struct drm_mode_crtc_lut __user *crtc_lut_user,
+ struct drm_file *file_priv)
+{
+ struct drm_mode_crtc_lut crtc_lut;
+
+ ret = copy_from_user(&crtc_lut, crtc_lut_user, sizeof(crtc_lut))
+ if (ret)
+ return -EFAULT;
+
+ crtc_lut.red = (unsigned long)compat_ptr(crtc_lut.red);
+ crtc_lut.green = (unsigned long)compat_ptr(crtc_lut.green);
+ crtc_lut.blue = (unsigned long)compat_ptr(crtc_lut.blue);
+
+ ret = drm_mode_gamma_get_ioctl(dev, &crtc_lut, file_priv);
+
+ return ret;
+}
+
+static int drm_generic_compat_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ struct drm_file *file_priv = filp->private_data;
+ struct drm_device *dev = file_priv->minor->dev;
+ unsigned int nr = DRM_IOCTL_NR(cmd);
+ int ret;
+
+ atomic_inc(&dev->ioctl_count);
+ atomic_inc(&dev->counts[_DRM_STAT_IOCTLS]);
+ ++file_priv->ioctl_count;
+
+ if ((nr >= DRM_CORE_IOCTL_COUNT) &&
+ ((nr < DRM_COMMAND_BASE) || (nr >= DRM_COMMAND_END)))
+ goto err_i1;
+ if ((nr >= DRM_COMMAND_BASE) && (nr < DRM_COMMAND_END) &&
+ (nr < DRM_COMMAND_BASE + dev->driver->num_ioctls))
+ ioctl = &dev->driver->ioctls[nr - DRM_COMMAND_BASE];
+ else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) {
+ ioctl = &drm_ioctls[nr];
+ cmd = ioctl->cmd;
+ } else
+ goto err_i1;
+
+ if (((ioctl->flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN)) ||
+ ((ioctl->flags & DRM_AUTH) && !file_priv->authenticated) ||
+ ((ioctl->flags & DRM_MASTER) && !file_priv->is_master) ||
+ (!(ioctl->flags & DRM_CONTROL_ALLOW) && (file_priv->minor->type == DRM_MINOR_CONTROL))) {
+ ret = -EACCES;
+ goto err_il;
+ }
+
+ switch (cmd) {
+ case DRM_IOCTL_MODE_GETPROPBLOB:
+ ret = compat_drm_mode_getblob_ioctl(dev,
+ compat_ptr(arg), file_priv);
+ break;
+
+ case DRM_IOCTL_MODE_GETGAMMA:
+ ret = compat_drm_mode_gamma_set_ioctl(dev,
+ compat_ptr(arg), file_priv);
+ break;
+
+ case DRM_IOCTL_MODE_GETGAMMA:
+ ret = compat_drm_mode_gamma_get_ioctl(dev,
+ compat_ptr(arg), file_priv);
+ break;
+ }
+
+ err_i1:
+ atomic_dec(&dev->ioctl_count);
+
+ return ret;
+}
+
drm_ioctl_compat_t *drm_compat_ioctls[] = {
[DRM_IOCTL_NR(DRM_IOCTL_VERSION32)] = compat_drm_version,
[DRM_IOCTL_NR(DRM_IOCTL_GET_UNIQUE32)] = compat_drm_getunique,
@@ -1072,6 +1188,9 @@ drm_ioctl_compat_t *drm_compat_ioctls[] = {
[DRM_IOCTL_NR(DRM_IOCTL_UPDATE_DRAW32)] = compat_drm_update_draw,
#endif
[DRM_IOCTL_NR(DRM_IOCTL_WAIT_VBLANK32)] = compat_drm_wait_vblank,
+ [DRM_IOCTL_MODE_GETPROPBLOB] = drm_generic_compat_ioctl,
+ [DRM_IOCTL_MODE_GETGAMMA] = drm_generic_compat_ioctl,
+ [DRM_IOCTL_MODE_SETGAMMA] = drm_generic_compat_ioctl,
};

/**