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

[Tvrtko Ursulin]

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) ?

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

Hm it's here as well, weird.. is it not sizeof(fence) ?

>   		return ERR_PTR(-EFAULT);
>   
> -	fences = kvmalloc_array(args->num_cliprects, sizeof(*fences),
> +	fences = kvmalloc_array(nfences, sizeof(*fences),
>   				__GFP_NOWARN | GFP_KERNEL);
>   	if (!fences)
>   		return ERR_PTR(-ENOMEM);
> @@ -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" ?

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?

>   		return -EINVAL;
>   	}
>   
> @@ -2565,17 +2568,17 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
>   		return -EINVAL;
>   
>   	/* Allocate an extra slot for use by the command parser */
> -	exec2_list = kvmalloc_array(args->buffer_count + 1, sz,
> +	exec2_list = kvmalloc_array(count + 1, sz,
>   				    __GFP_NOWARN | GFP_KERNEL);
>   	if (exec2_list == NULL) {
> -		DRM_DEBUG("Failed to allocate exec list for %d buffers\n",
> -			  args->buffer_count);
> +		DRM_DEBUG("Failed to allocate exec list for %zd buffers\n",
> +			  count);
>   		return -ENOMEM;
>   	}
>   	if (copy_from_user(exec2_list,
>   			   u64_to_user_ptr(args->buffers_ptr),
> -			   sizeof(*exec2_list) * args->buffer_count)) {
> -		DRM_DEBUG("copy %d exec entries failed\n", args->buffer_count);
> +			   sizeof(*exec2_list) * count)) {
> +		DRM_DEBUG("copy %zd exec entries failed\n", count);
>   		kvfree(exec2_list);
>   		return -EFAULT;
>   	}
> 

Regards,

Tvrtko