On Fri, May 24, 2019 at 8:05 AM Thomas Hellstrom <thellstrom@xxxxxxxxxx> wrote: > > On Wed, 2019-05-22 at 21:09 +0200, Daniel Vetter wrote: > > 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 ... > > After checking the drm_ioctl code I realize that what I thought was new > behaviour actually has been around for a couple of years, so > fixing isn't really tied to this patch series... > > What caused me to react was that previously we used to have this > > e4fda9f264e1 ("drm: Perform ioctl command validation on the stored > kernel values") > > and we seem to have lost that now, if not for the io flags then at > least for the size part. For the size of the ioctl arguments, I think > in general if the kernel only touches a subset of the user-space > specified size I see no reason why we should malloc / copy more than > that? I guess we could optimize that, but we'd probably still need to zero clear the added size for forward compat with newer userspace. Iirc we've had some issues in this area. > Now, given the fact that the maximum ioctl argument size is quite > limited, that might not be a big problem or a problem at all. Otherwise > it would be pretty easy for a malicious process to allocate most or all > of a system's resident memory? The biggest you can allocate from kmalloc is limited by the largest contiguous chunk alloc_pages gives you, which is limited by MAX_ORDER from the page buddy allocator. You need lots of process to be able to exhaust memory like that (and like I said, the entire kernel would be broken if we'd consider this a security issue). If you want to make sure that a process group can't exhaust memory this way then you need to set appropriate cgroups limits. -Daniel > > /Thomas > > > > > > > > > -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(c > > > > md)) > > > > ; > > > > + 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