On Wed, May 22, 2019 at 9:01 PM Thomas Hellstrom <thellstrom@xxxxxxxxxx> wrote:
>
> 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.
Hm I always worked under the assumption that kmalloc and friends
should be userspace hardened. Otherwise stuff like kmalloc_array
doesn't make any sense, everyone just feeds it unchecked input and
expects that helper to handle overflows.
If we assume kmalloc isn't hardened against that, then we have a much
bigger problem than just vmwgfx ioctls ...
-Daniel
>
>
> >
> > 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))
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
