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

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-11-16 09:00:08)
> 
> On 15/11/2017 16:08, Chris Wilson wrote:
> > @@ -2083,14 +2083,14 @@ get_fence_array(struct drm_i915_gem_execbuffer2 *args,
> >       if (!(args->flags & I915_EXEC_FENCE_ARRAY))
> >               return NULL;
> >   
> > -     if (nfences > SIZE_MAX / sizeof(*fences))
> > +     if (nfences > SIZE_MAX / max(sizeof(*fences), sizeof(*user)))
> >               return ERR_PTR(-EINVAL);
> 
> This check is just to stay in some reasonable limits, yes? Doesn't seem 
> to be directly related to the implementation below. I am not saying we 
> should allow more fences, just wondering whether it is worthy of a 
> comment? And maybe in that case it is not -EINVAL (ABI) but -ENOMEM (to 
> fake implementation cannot support it / doesn't want to support it)? 
> Assuming I did not miss something.

> >       user = u64_to_user_ptr(args->cliprects_ptr);
> > -     if (!access_ok(VERIFY_READ, user, nfences * 2 * sizeof(u32)))
> > +     if (!access_ok(VERIFY_READ, user, nfences * sizeof(*user)))
> >               return ERR_PTR(-EFAULT);
> >   

For access_ok we use nfences * sizeof(*user), therefore we have to make
sure that doesn't overflow and produce small value which then passes the
check and allows the caller to wander off into the badlands. So really
the question is it EFAULT for the invalid pointer or EINVAL or the
unusable parameters. We've (I've) taken the stance that we wanted to
differentiate between the parameters being bogus and the result of using
those parameters being bogus.
-Chris