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

[Chris Wilson]

Quoting Chris Wilson (2017-11-15 16:00:10)
> 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
> v3: Move common checks for eb1/eb2 into the same function
> 
> 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 | 58 +++++++++++++++++++-----------
>  1 file changed, 37 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 435ed95df144..2e088c48c81d 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;
> @@ -2087,10 +2087,10 @@ get_fence_array(struct drm_i915_gem_execbuffer2 *args,
>                 return ERR_PTR(-EINVAL);
>  
>         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);

Grr, of course that's where the 2*sizeof(u32) came from. I need that
check back.
-Chris