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

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-11-16 09:44:20)
> 
> On 16/11/2017 09:36, Chris Wilson wrote:
> > 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.
> 
> Oh right.. but looking at access_ok it seems that it is working with 
> unsigned longs. Not that it is documents explicitly, it is just a macro 
> and happens to use that under the covers (and via mm_segment_t type as 
> well). So it happens to match with size_t, but perhaps it indeed 
> warrants a comment and changing it to ULONG_MAX.

GAH! size_t as you probably know was chosen for kmalloc_array. Ok, we
are not using those functions here, so ulongs it is.
-Chris