[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:
> > We check whether the multiplies will overflow prior to calling
> > kmalloc_array so that we can respond with -EINVAL for the invalid user
> > arguments rather than treating it as an -ENOMEM that would otherwise
> > occur. However, as Dan Carpenter pointed out, we did an addition on the
> > unsigned int prior to passing to kmalloc_array where it would be
> > promoted to size_t for the calculation, thereby allowing it to overflow
> > and underallocate.
> > 
> > v2: buffer_count is currently limited to INT_MAX because we treat it as
> > signaled variable for LUT_HANDLE in eb_lookup_vma
> > 
> > Reported-by: Dan Carpenter <dan.carpenter at oracle.com>
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 33 ++++++++++++++++--------------
> >   1 file changed, 18 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 435ed95df144..a8dec9abe33d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -2074,7 +2074,7 @@ static struct drm_syncobj **
> >   get_fence_array(struct drm_i915_gem_execbuffer2 *args,
> >               struct drm_file *file)
> >   {
> > -     const unsigned int nfences = args->num_cliprects;
> > +     const size_t nfences = args->num_cliprects;
> >       struct drm_i915_gem_exec_fence __user *user;
> >       struct drm_syncobj **fences;
> >       unsigned int n;
> > @@ -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), 2*sizeof(u32)))
> 
> What is 2 * sizeof(u32) ?

I had a reason at one point. Memory says the code was allocating an
array of int[2]. The first check doesn't need it (since it not tied to
an allocation anymore), but the access_ok() should be using sizeof(*user).
-Chris