[Intel-gfx] [resend] drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-11-15 15:33:28)
> 
> On 14/11/2017 23:00, Chris Wilson wrote:
> > @@ -2462,11 +2462,13 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> >       struct drm_i915_gem_execbuffer2 exec2;
> >       struct drm_i915_gem_exec_object *exec_list = NULL;
> >       struct drm_i915_gem_exec_object2 *exec2_list = NULL;
> > +     const size_t count = args->buffer_count;
> >       unsigned int i;
> >       int err;
> >   
> > -     if (args->buffer_count < 1 || args->buffer_count > SIZE_MAX / sz - 1) {
> > -             DRM_DEBUG("execbuf2 with %d buffers\n", args->buffer_count);
> > +     /* Lookups via HANDLE_LUT are limited to INT_MAX (see eb_create()) */
> > +     if (count < 1 || count > INT_MAX || count > SIZE_MAX / sz - 1) {
> 
> Does it need to be "count >= INT_MAX" since below we do "count + 1" ?

> > v2: buffer_count is currently limited to INT_MAX because we treat it as
> > signaled variable for LUT_HANDLE in eb_lookup_vma

The +1 is not used for the lookup, so we only need to check against
INT_MAX and not INT_MAX-1.

> 
> I am not sure though why this check. kvmalloc_array takes size_t for 
> both parameters, so where does an INT_MAX come into picture to start 
> with? Should it be count >= SIZE_MAX in this check since count is size_t?
> 
> > +             DRM_DEBUG("execbuf2 with %zd buffers\n", count); >              return -EINVAL;
> >       }
> >   
> > @@ -2485,9 +2487,9 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> >               return -EINVAL;
> >   
> >       /* Copy in the exec list from userland */
> > -     exec_list = kvmalloc_array(args->buffer_count, sizeof(*exec_list),
> > +     exec_list = kvmalloc_array(count, sizeof(*exec_list),
> >                                  __GFP_NOWARN | GFP_KERNEL);
> > -     exec2_list = kvmalloc_array(args->buffer_count + 1, sz,
> > +     exec2_list = kvmalloc_array(count + 1, sz,
> >                                   __GFP_NOWARN | GFP_KERNEL);
> >       if (exec_list == NULL || exec2_list == NULL) {
> >               DRM_DEBUG("Failed to allocate exec list for %d buffers\n",
> > @@ -2498,7 +2500,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> >       }
> >       err = copy_from_user(exec_list,
> >                            u64_to_user_ptr(args->buffers_ptr),
> > -                          sizeof(*exec_list) * args->buffer_count);
> > +                          sizeof(*exec_list) * count);
> >       if (err) {
> >               DRM_DEBUG("copy %d exec entries failed %d\n",
> >                         args->buffer_count, err);
> > @@ -2554,10 +2556,11 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
> >       struct drm_i915_gem_execbuffer2 *args = data;
> >       struct drm_i915_gem_exec_object2 *exec2_list;
> >       struct drm_syncobj **fences = NULL;
> > +     const size_t count = args->buffer_count;
> >       int err;
> >   
> > -     if (args->buffer_count < 1 || args->buffer_count > SIZE_MAX / sz - 1) {
> > -             DRM_DEBUG("execbuf2 with %d buffers\n", args->buffer_count);
> > +     if (count < 1 || count > SIZE_MAX / sz - 1) {
> > +             DRM_DEBUG("execbuf2 with %zd buffers\n", count);
> 
> Why is the check different between eb and eb2? Move to helper if the 
> same check is actually correct?

We need before the allocation so that we report -EINVAL not -ENOMEM.
(Because I forgot about the duplication after finding the INT_MAX limit.)
-Chris