Hi, Emil,
On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>
>
> Currently vmw_execbuf_ioctl() open-codes the permission checking,
> size
> extending and copying that is already done in core drm.
>
> Kill all the duplication, adding a few comments for clarity.
Ah, there is core functionality for this now.
What worries me though with the core approach is that the sizes are not
capped by the size of the kernel argument definition, which makes
mailicious user-space being able to force kmallocs() the size of the
maximum ioctl size. Should probably be fixed before pushing this.
>
> Cc: "VMware Graphics" <linux-graphics-maintainer@xxxxxxxxxx>
> Cc: Thomas Hellstrom <thellstrom@xxxxxxxxxx>
> Cc: Daniel Vetter <daniel@xxxxxxxx>
> Signed-off-by: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>
> ---
> Thomas, VMware team,
>
> Please give this some testing on your end. I've only tested it
> against
> mesa-master.
I'll review tomorrow and do some testing. Need to see if I can dig up
user-space apps with version 0...
Thanks,
Thomas
>
> Thanks
> Emil
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 12 +-----
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 4 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 52 +++++++++------------
> ----
> 3 files changed, 23 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index d3f108f7e52d..2cb6ae219e43 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -186,7 +186,7 @@ static const struct drm_ioctl_desc vmw_ioctls[] =
> {
> DRM_RENDER_ALLOW),
> VMW_IOCTL_DEF(VMW_REF_SURFACE, vmw_surface_reference_ioctl,
> DRM_AUTH | DRM_RENDER_ALLOW),
> - VMW_IOCTL_DEF(VMW_EXECBUF, NULL, DRM_AUTH |
> + VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl, DRM_AUTH |
> DRM_RENDER_ALLOW),
> VMW_IOCTL_DEF(VMW_FENCE_WAIT, vmw_fence_obj_wait_ioctl,
> DRM_RENDER_ALLOW),
> @@ -1140,15 +1140,7 @@ static long vmw_generic_ioctl(struct file
> *filp, unsigned int cmd,
> &vmw_ioctls[nr - DRM_COMMAND_BASE];
>
> if (nr == DRM_COMMAND_BASE + DRM_VMW_EXECBUF) {
> - ret = (long) drm_ioctl_permit(ioctl->flags,
> file_priv);
> - if (unlikely(ret != 0))
> - return ret;
> -
> - if (unlikely((cmd & (IOC_IN | IOC_OUT)) !=
> IOC_IN))
> - goto out_io_encoding;
> -
> - return (long) vmw_execbuf_ioctl(dev, arg,
> file_priv,
> - _IOC_SIZE(cmd))
> ;
> + return ioctl_func(filp, cmd, arg);
> } else if (nr == DRM_COMMAND_BASE +
> DRM_VMW_UPDATE_LAYOUT) {
> if (!drm_is_current_master(file_priv) &&
> !capable(CAP_SYS_ADMIN))
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index 9be2176cc260..f5bfac85f793 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -910,8 +910,8 @@ static inline struct page *vmw_piter_page(struct
> vmw_piter *viter)
> * Command submission - vmwgfx_execbuf.c
> */
>
> -extern int vmw_execbuf_ioctl(struct drm_device *dev, unsigned long
> data,
> - struct drm_file *file_priv, size_t size);
> +extern int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_priv);
> extern int vmw_execbuf_process(struct drm_file *file_priv,
> struct vmw_private *dev_priv,
> void __user *user_commands,
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> index 2ff7ba04d8c8..767e2b99618d 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> @@ -3977,54 +3977,40 @@ void vmw_execbuf_release_pinned_bo(struct
> vmw_private *dev_priv)
> mutex_unlock(&dev_priv->cmdbuf_mutex);
> }
>
> -int vmw_execbuf_ioctl(struct drm_device *dev, unsigned long data,
> - struct drm_file *file_priv, size_t size)
> +int vmw_execbuf_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_priv)
> {
> struct vmw_private *dev_priv = vmw_priv(dev);
> - struct drm_vmw_execbuf_arg arg;
> + struct drm_vmw_execbuf_arg *arg = data;
> int ret;
> - static const size_t copy_offset[] = {
> - offsetof(struct drm_vmw_execbuf_arg, context_handle),
> - sizeof(struct drm_vmw_execbuf_arg)};
> struct dma_fence *in_fence = NULL;
>
> - if (unlikely(size < copy_offset[0])) {
> - VMW_DEBUG_USER("Invalid command size, ioctl %d\n",
> - DRM_VMW_EXECBUF);
> - return -EINVAL;
> - }
> -
> - if (copy_from_user(&arg, (void __user *) data, copy_offset[0])
> != 0)
> - return -EFAULT;
> -
> /*
> * Extend the ioctl argument while maintaining backwards
> compatibility:
> - * We take different code paths depending on the value of
> arg.version.
> + * We take different code paths depending on the value of arg-
> >version.
> + *
> + * Note: The ioctl argument is extended and zeropadded by core
> DRM.
> */
> - if (unlikely(arg.version > DRM_VMW_EXECBUF_VERSION ||
> - arg.version == 0)) {
> + if (unlikely(arg->version > DRM_VMW_EXECBUF_VERSION ||
> + arg->version == 0)) {
> VMW_DEBUG_USER("Incorrect execbuf version.\n");
> return -EINVAL;
> }
>
> - if (arg.version > 1 &&
> - copy_from_user(&arg.context_handle,
> - (void __user *) (data + copy_offset[0]),
> - copy_offset[arg.version - 1] -
> copy_offset[0]) != 0)
> - return -EFAULT;
> -
> - switch (arg.version) {
> + switch (arg->version) {
> case 1:
> - arg.context_handle = (uint32_t) -1;
> + /* For v1 core DRM have extended + zeropadded the data
> */
> + arg->context_handle = (uint32_t) -1;
> break;
> case 2:
> default:
> + /* For v2 and later core DRM would have correctly
> copied it */
> break;
> }
>
> /* If imported a fence FD from elsewhere, then wait on it */
> - if (arg.flags & DRM_VMW_EXECBUF_FLAG_IMPORT_FENCE_FD) {
> - in_fence = sync_file_get_fence(arg.imported_fence_fd);
> + if (arg->flags & DRM_VMW_EXECBUF_FLAG_IMPORT_FENCE_FD) {
> + in_fence = sync_file_get_fence(arg->imported_fence_fd);
>
> if (!in_fence) {
> VMW_DEBUG_USER("Cannot get imported fence\n");
> @@ -4041,11 +4027,11 @@ int vmw_execbuf_ioctl(struct drm_device *dev,
> unsigned long data,
> return ret;
>
> ret = vmw_execbuf_process(file_priv, dev_priv,
> - (void __user *)(unsigned
> long)arg.commands,
> - NULL, arg.command_size,
> arg.throttle_us,
> - arg.context_handle,
> - (void __user *)(unsigned
> long)arg.fence_rep,
> - NULL, arg.flags);
> + (void __user *)(unsigned long)arg-
> >commands,
> + NULL, arg->command_size, arg-
> >throttle_us,
> + arg->context_handle,
> + (void __user *)(unsigned long)arg-
> >fence_rep,
> + NULL, arg->flags);
>
> ttm_read_unlock(&dev_priv->reservation_sem);
> if (unlikely(ret != 0))
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
