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

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

[Chris Wilson]

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@oracle.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@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)))
 		return ERR_PTR(-EINVAL);
 
 	user = u64_to_user_ptr(args->cliprects_ptr);
 	if (!access_ok(VERIFY_READ, user, nfences * 2 * sizeof(u32)))
 		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) {
+		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);
 		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;
 	}
-- 
2.15.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects

[Patchwork]

== Series Details ==

Series: drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects
URL   : https://patchwork.freedesktop.org/series/33840/
State : warning

== Summary ==

Series 33840v1 drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects
https://patchwork.freedesktop.org/api/1.0/series/33840/revisions/1/mbox/

Test chamelium:
        Subgroup dp-crc-fast:
                fail       -> PASS       (fi-kbl-7500u) fdo#102514
Test kms_busy:
        Subgroup basic-flip-a:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup basic-flip-b:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup basic-flip-c:
                skip       -> PASS       (fi-hsw-4770r)
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup basic-busy-flip-before-cursor-legacy:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup basic-flip-after-cursor-atomic:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup basic-flip-after-cursor-legacy:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup basic-flip-after-cursor-varying-size:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup basic-flip-before-cursor-atomic:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup basic-flip-before-cursor-legacy:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup basic-flip-before-cursor-varying-size:
                skip       -> PASS       (fi-hsw-4770r)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup basic-flip-vs-modeset:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup basic-flip-vs-wf_vblank:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup basic-plain-flip:
                skip       -> PASS       (fi-hsw-4770r)
Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> SKIP       (fi-ivb-3520m)
        Subgroup force-edid:
                pass       -> SKIP       (fi-ivb-3520m)
        Subgroup force-load-detect:
                pass       -> SKIP       (fi-ivb-3520m)
        Subgroup prune-stale-modes:
                pass       -> SKIP       (fi-ivb-3520m)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                skip       -> PASS       (fi-hsw-4770r)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup hang-read-crc-pipe-b:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup hang-read-crc-pipe-c:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup nonblocking-crc-pipe-a:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup nonblocking-crc-pipe-a-frame-sequence:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup nonblocking-crc-pipe-b:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup nonblocking-crc-pipe-c:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup nonblocking-crc-pipe-c-frame-sequence:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup read-crc-pipe-a:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup read-crc-pipe-a-frame-sequence:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup read-crc-pipe-b:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup read-crc-pipe-b-frame-sequence:
                skip       -> PASS       (fi-hsw-4770r) fdo#102332
        Subgroup read-crc-pipe-c:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup read-crc-pipe-c-frame-sequence:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup suspend-read-crc-pipe-a:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup suspend-read-crc-pipe-b:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup suspend-read-crc-pipe-c:
                skip       -> PASS       (fi-hsw-4770r)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                skip       -> PASS       (fi-hsw-4770r)
        Subgroup basic-rte:
                skip       -> PASS       (fi-hsw-4770r)
Test prime_vgem:
        Subgroup basic-fence-flip:
                skip       -> PASS       (fi-hsw-4770r)

fdo#102514 https://bugs.freedesktop.org/show_bug.cgi?id=102514
fdo#102332 https://bugs.freedesktop.org/show_bug.cgi?id=102332

WARNING: Long output truncated
fi-cnl-y failed to connect after reboot

5f7fd97a513ac1f0b84c61fae3fc83d4432e602c drm-tip: 2017y-11m-14d-21h-04m-27s UTC integration manifest
6e76cdcdb645 drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7133/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@oracle.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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:
> > @@ -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" ?

> > v2: buffer_count is currently limited to INT_MAX because we treat it as
> > signaled variable for LUT_HANDLE in eb_lookup_vma

The +1 is not used for the lookup, so we only need to check against
INT_MAX and not INT_MAX-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?

We need before the allocation so that we report -EINVAL not -ENOMEM.
(Because I forgot about the duplication after finding the INT_MAX limit.)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@oracle.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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@oracle.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@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);
 
-	fences = kvmalloc_array(args->num_cliprects, sizeof(*fences),
+	fences = kvmalloc_array(nfences, sizeof(*fences),
 				__GFP_NOWARN | GFP_KERNEL);
 	if (!fences)
 		return ERR_PTR(-ENOMEM);
@@ -2447,6 +2447,26 @@ i915_gem_do_execbuffer(struct drm_device *dev,
 	return err;
 }
 
+static size_t eb_element_size(void)
+{
+	return (sizeof(struct drm_i915_gem_exec_object2) +
+		sizeof(struct i915_vma *) +
+		sizeof(unsigned int));
+}
+
+static bool check_buffer_count(size_t count)
+{
+	const size_t sz = eb_element_size();
+
+	/*
+	 * When using LUT_HANDLE, we impose a limit of INT_MAX for the lookup
+	 * array size (see eb_create()). Otherwise, we can accept an array as
+	 * large as can be addressed (though use large arrays at your peril)!
+	 */
+
+	return !(count < 1 || count > INT_MAX || count > SIZE_MAX / sz - 1);
+}
+
 /*
  * Legacy execbuffer just creates an exec2 list from the original exec object
  * list array and passes it to the real function.
@@ -2455,18 +2475,16 @@ int
 i915_gem_execbuffer(struct drm_device *dev, void *data,
 		    struct drm_file *file)
 {
-	const size_t sz = (sizeof(struct drm_i915_gem_exec_object2) +
-			   sizeof(struct i915_vma *) +
-			   sizeof(unsigned int));
 	struct drm_i915_gem_execbuffer *args = 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);
+	if (!check_buffer_count(count)) {
+		DRM_DEBUG("execbuf2 with %zd buffers\n", count);
 		return -EINVAL;
 	}
 
@@ -2485,9 +2503,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, eb_element_size(),
 				    __GFP_NOWARN | GFP_KERNEL);
 	if (exec_list == NULL || exec2_list == NULL) {
 		DRM_DEBUG("Failed to allocate exec list for %d buffers\n",
@@ -2498,7 +2516,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);
@@ -2548,16 +2566,14 @@ int
 i915_gem_execbuffer2(struct drm_device *dev, void *data,
 		     struct drm_file *file)
 {
-	const size_t sz = (sizeof(struct drm_i915_gem_exec_object2) +
-			   sizeof(struct i915_vma *) +
-			   sizeof(unsigned int));
 	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 (!check_buffer_count(count)) {
+		DRM_DEBUG("execbuf2 with %zd buffers\n", count);
 		return -EINVAL;
 	}
 
@@ -2565,17 +2581,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, eb_element_size(),
 				    __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;
 	}
-- 
2.15.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@oracle.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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
v4: Put the check back for nfence*sizeof(user_fence) overflow

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 60 +++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 435ed95df144..afd32f85f612 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), sizeof(*user)))
 		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);
 
-	fences = kvmalloc_array(args->num_cliprects, sizeof(*fences),
+	fences = kvmalloc_array(nfences, sizeof(*fences),
 				__GFP_NOWARN | GFP_KERNEL);
 	if (!fences)
 		return ERR_PTR(-ENOMEM);
@@ -2447,6 +2447,26 @@ i915_gem_do_execbuffer(struct drm_device *dev,
 	return err;
 }
 
+static size_t eb_element_size(void)
+{
+	return (sizeof(struct drm_i915_gem_exec_object2) +
+		sizeof(struct i915_vma *) +
+		sizeof(unsigned int));
+}
+
+static bool check_buffer_count(size_t count)
+{
+	const size_t sz = eb_element_size();
+
+	/*
+	 * When using LUT_HANDLE, we impose a limit of INT_MAX for the lookup
+	 * array size (see eb_create()). Otherwise, we can accept an array as
+	 * large as can be addressed (though use large arrays at your peril)!
+	 */
+
+	return !(count < 1 || count > INT_MAX || count > SIZE_MAX / sz - 1);
+}
+
 /*
  * Legacy execbuffer just creates an exec2 list from the original exec object
  * list array and passes it to the real function.
@@ -2455,18 +2475,16 @@ int
 i915_gem_execbuffer(struct drm_device *dev, void *data,
 		    struct drm_file *file)
 {
-	const size_t sz = (sizeof(struct drm_i915_gem_exec_object2) +
-			   sizeof(struct i915_vma *) +
-			   sizeof(unsigned int));
 	struct drm_i915_gem_execbuffer *args = 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);
+	if (!check_buffer_count(count)) {
+		DRM_DEBUG("execbuf2 with %zd buffers\n", count);
 		return -EINVAL;
 	}
 
@@ -2485,9 +2503,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, eb_element_size(),
 				    __GFP_NOWARN | GFP_KERNEL);
 	if (exec_list == NULL || exec2_list == NULL) {
 		DRM_DEBUG("Failed to allocate exec list for %d buffers\n",
@@ -2498,7 +2516,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);
@@ -2548,16 +2566,14 @@ int
 i915_gem_execbuffer2(struct drm_device *dev, void *data,
 		     struct drm_file *file)
 {
-	const size_t sz = (sizeof(struct drm_i915_gem_exec_object2) +
-			   sizeof(struct i915_vma *) +
-			   sizeof(unsigned int));
 	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 (!check_buffer_count(count)) {
+		DRM_DEBUG("execbuf2 with %zd buffers\n", count);
 		return -EINVAL;
 	}
 
@@ -2565,17 +2581,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, eb_element_size(),
 				    __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;
 	}
-- 
2.15.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects (rev3)
URL   : https://patchwork.freedesktop.org/series/33840/
State : success

== Summary ==

Series 33840v3 drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects
https://patchwork.freedesktop.org/api/1.0/series/33840/revisions/3/mbox/

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:441s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:459s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:381s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:542s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:276s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:502s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:506s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:498s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:490s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:434s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:265s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:545s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:429s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:442s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:430s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:483s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:467s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:487s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:535s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:475s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:540s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:578s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:458s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:545s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:567s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:522s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:496s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:464s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:558s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:422s
Blacklisted hosts:
fi-cfl-s         total:289  pass:254  dwarn:3   dfail:0   fail:0   skip:32  time:543s

06718a287f282ad31264560c5dc18a38474cb562 drm-tip: 2017y-11m-15d-11h-36m-39s UTC integration manifest
83e8aeeafcd0 drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7146/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: warning for drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects (rev3)
URL   : https://patchwork.freedesktop.org/series/33840/
State : warning

== Summary ==

Test drv_module_reload:
        Subgroup basic-reload-inject:
                pass       -> DMESG-WARN (shard-hsw) fdo#102707
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-primscrn-pri-indfb-draw-pwrite:
                pass       -> SKIP       (shard-hsw)
Test kms_atomic_transition:
        Subgroup plane-all-modeset-transition-fencing:
                dmesg-warn -> PASS       (shard-hsw) fdo#102614

fdo#102707 https://bugs.freedesktop.org/show_bug.cgi?id=102707
fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614

shard-hsw        total:2584 pass:1470 dwarn:3   dfail:1   fail:10  skip:1100 time:9573s
Blacklisted hosts:
shard-apl        total:2565 pass:1600 dwarn:3   dfail:1   fail:24  skip:936 time:13135s
shard-kbl        total:2500 pass:1659 dwarn:5   dfail:3   fail:22  skip:809 time:10385s
shard-snb        total:2584 pass:1209 dwarn:2   dfail:1   fail:12  skip:1360 time:7882s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7146/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 15/11/2017 16:08, 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
> v3: Move common checks for eb1/eb2 into the same function
> v4: Put the check back for nfence*sizeof(user_fence) overflow
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 60 +++++++++++++++++++-----------
>   1 file changed, 38 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 435ed95df144..afd32f85f612 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), 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);
>   
> -	fences = kvmalloc_array(args->num_cliprects, sizeof(*fences),
> +	fences = kvmalloc_array(nfences, sizeof(*fences),
>   				__GFP_NOWARN | GFP_KERNEL);
>   	if (!fences)
>   		return ERR_PTR(-ENOMEM);
> @@ -2447,6 +2447,26 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>   	return err;
>   }
>   
> +static size_t eb_element_size(void)
> +{
> +	return (sizeof(struct drm_i915_gem_exec_object2) +
> +		sizeof(struct i915_vma *) +
> +		sizeof(unsigned int));
> +}
> +
> +static bool check_buffer_count(size_t count)
> +{
> +	const size_t sz = eb_element_size();
> +
> +	/*
> +	 * When using LUT_HANDLE, we impose a limit of INT_MAX for the lookup
> +	 * array size (see eb_create()). Otherwise, we can accept an array as
> +	 * large as can be addressed (though use large arrays at your peril)!
> +	 */
> +
> +	return !(count < 1 || count > INT_MAX || count > SIZE_MAX / sz - 1);
> +}
> +
>   /*
>    * Legacy execbuffer just creates an exec2 list from the original exec object
>    * list array and passes it to the real function.
> @@ -2455,18 +2475,16 @@ int
>   i915_gem_execbuffer(struct drm_device *dev, void *data,
>   		    struct drm_file *file)
>   {
> -	const size_t sz = (sizeof(struct drm_i915_gem_exec_object2) +
> -			   sizeof(struct i915_vma *) +
> -			   sizeof(unsigned int));
>   	struct drm_i915_gem_execbuffer *args = 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);
> +	if (!check_buffer_count(count)) {
> +		DRM_DEBUG("execbuf2 with %zd buffers\n", count);
>   		return -EINVAL;
>   	}
>   
> @@ -2485,9 +2503,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, eb_element_size(),
>   				    __GFP_NOWARN | GFP_KERNEL);
>   	if (exec_list == NULL || exec2_list == NULL) {
>   		DRM_DEBUG("Failed to allocate exec list for %d buffers\n",
> @@ -2498,7 +2516,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);
> @@ -2548,16 +2566,14 @@ int
>   i915_gem_execbuffer2(struct drm_device *dev, void *data,
>   		     struct drm_file *file)
>   {
> -	const size_t sz = (sizeof(struct drm_i915_gem_exec_object2) +
> -			   sizeof(struct i915_vma *) +
> -			   sizeof(unsigned int));
>   	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 (!check_buffer_count(count)) {
> +		DRM_DEBUG("execbuf2 with %zd buffers\n", count);
>   		return -EINVAL;
>   	}
>   
> @@ -2565,17 +2581,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, eb_element_size(),
>   				    __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;
>   	}
> 

Rest looks fine.

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v5] drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects

[Chris Wilson]

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
v4: Put the check back for nfence*sizeof(user_fence) overflow
v5: access_ok uses ULONG_MAX but kvmalloc_array uses SIZE_MAX

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 64 +++++++++++++++++++-----------
 1 file changed, 41 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 435ed95df144..4e516fec67af 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -2074,23 +2074,25 @@ 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 unsigned long nfences = args->num_cliprects;
 	struct drm_i915_gem_exec_fence __user *user;
 	struct drm_syncobj **fences;
-	unsigned int n;
+	unsigned long n;
 	int err;
 
 	if (!(args->flags & I915_EXEC_FENCE_ARRAY))
 		return NULL;
 
-	if (nfences > SIZE_MAX / sizeof(*fences))
+	/* Check multiplication overflow for access_ok() and kvmalloc_array() */
+	if (nfences > min(ULONG_MAX / sizeof(*user),
+			  SIZE_MAX / sizeof(*fences)))
 		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);
 
-	fences = kvmalloc_array(args->num_cliprects, sizeof(*fences),
+	fences = kvmalloc_array(nfences, sizeof(*fences),
 				__GFP_NOWARN | GFP_KERNEL);
 	if (!fences)
 		return ERR_PTR(-ENOMEM);
@@ -2447,6 +2449,26 @@ i915_gem_do_execbuffer(struct drm_device *dev,
 	return err;
 }
 
+static size_t eb_element_size(void)
+{
+	return (sizeof(struct drm_i915_gem_exec_object2) +
+		sizeof(struct i915_vma *) +
+		sizeof(unsigned int));
+}
+
+static bool check_buffer_count(size_t count)
+{
+	const size_t sz = eb_element_size();
+
+	/*
+	 * When using LUT_HANDLE, we impose a limit of INT_MAX for the lookup
+	 * array size (see eb_create()). Otherwise, we can accept an array as
+	 * large as can be addressed (though use large arrays at your peril)!
+	 */
+
+	return !(count < 1 || count > INT_MAX || count > SIZE_MAX / sz - 1);
+}
+
 /*
  * Legacy execbuffer just creates an exec2 list from the original exec object
  * list array and passes it to the real function.
@@ -2455,18 +2477,16 @@ int
 i915_gem_execbuffer(struct drm_device *dev, void *data,
 		    struct drm_file *file)
 {
-	const size_t sz = (sizeof(struct drm_i915_gem_exec_object2) +
-			   sizeof(struct i915_vma *) +
-			   sizeof(unsigned int));
 	struct drm_i915_gem_execbuffer *args = 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);
+	if (!check_buffer_count(count)) {
+		DRM_DEBUG("execbuf2 with %zd buffers\n", count);
 		return -EINVAL;
 	}
 
@@ -2485,9 +2505,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, eb_element_size(),
 				    __GFP_NOWARN | GFP_KERNEL);
 	if (exec_list == NULL || exec2_list == NULL) {
 		DRM_DEBUG("Failed to allocate exec list for %d buffers\n",
@@ -2498,7 +2518,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);
@@ -2548,16 +2568,14 @@ int
 i915_gem_execbuffer2(struct drm_device *dev, void *data,
 		     struct drm_file *file)
 {
-	const size_t sz = (sizeof(struct drm_i915_gem_exec_object2) +
-			   sizeof(struct i915_vma *) +
-			   sizeof(unsigned int));
 	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 (!check_buffer_count(count)) {
+		DRM_DEBUG("execbuf2 with %zd buffers\n", count);
 		return -EINVAL;
 	}
 
@@ -2565,17 +2583,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, eb_element_size(),
 				    __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;
 	}
-- 
2.15.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects (rev4)
URL   : https://patchwork.freedesktop.org/series/33840/
State : success

== Summary ==

Series 33840v4 drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects
https://patchwork.freedesktop.org/api/1.0/series/33840/revisions/4/mbox/

Warning: Kernel 32bit buildtest failed

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:440s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:455s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:382s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:535s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:279s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:509s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:507s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:502s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:489s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:430s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:265s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:538s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:430s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:437s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:427s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:487s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:466s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:485s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:530s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:475s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:540s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:576s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:456s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:546s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:565s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:521s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:499s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:464s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:558s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:417s
fi-cfl-s failed to connect after reboot

7916df8b49cb8bb9783a8425e7345cc47159f3f0 drm-tip: 2017y-11m-15d-20h-05m-48s UTC integration manifest
8adfe6d37a01 drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7155/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6] drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects

[Chris Wilson]

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
v4: Put the check back for nfence*sizeof(user_fence) overflow
v5: access_ok uses ULONG_MAX but kvmalloc_array uses SIZE_MAX
v6: size_t and unsigned long are not type-equivalent on 32b

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 66 +++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 435ed95df144..53ccb27bfe91 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -2074,23 +2074,27 @@ 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 unsigned long nfences = args->num_cliprects;
 	struct drm_i915_gem_exec_fence __user *user;
 	struct drm_syncobj **fences;
-	unsigned int n;
+	unsigned long n;
 	int err;
 
 	if (!(args->flags & I915_EXEC_FENCE_ARRAY))
 		return NULL;
 
-	if (nfences > SIZE_MAX / sizeof(*fences))
+	/* Check multiplication overflow for access_ok() and kvmalloc_array() */
+	BUILD_BUG_ON(sizeof(size_t) > sizeof(unsigned long));
+	if (nfences > min_t(unsigned long,
+			    ULONG_MAX / sizeof(*user),
+			    SIZE_MAX / sizeof(*fences)))
 		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);
 
-	fences = kvmalloc_array(args->num_cliprects, sizeof(*fences),
+	fences = kvmalloc_array(nfences, sizeof(*fences),
 				__GFP_NOWARN | GFP_KERNEL);
 	if (!fences)
 		return ERR_PTR(-ENOMEM);
@@ -2447,6 +2451,26 @@ i915_gem_do_execbuffer(struct drm_device *dev,
 	return err;
 }
 
+static size_t eb_element_size(void)
+{
+	return (sizeof(struct drm_i915_gem_exec_object2) +
+		sizeof(struct i915_vma *) +
+		sizeof(unsigned int));
+}
+
+static bool check_buffer_count(size_t count)
+{
+	const size_t sz = eb_element_size();
+
+	/*
+	 * When using LUT_HANDLE, we impose a limit of INT_MAX for the lookup
+	 * array size (see eb_create()). Otherwise, we can accept an array as
+	 * large as can be addressed (though use large arrays at your peril)!
+	 */
+
+	return !(count < 1 || count > INT_MAX || count > SIZE_MAX / sz - 1);
+}
+
 /*
  * Legacy execbuffer just creates an exec2 list from the original exec object
  * list array and passes it to the real function.
@@ -2455,18 +2479,16 @@ int
 i915_gem_execbuffer(struct drm_device *dev, void *data,
 		    struct drm_file *file)
 {
-	const size_t sz = (sizeof(struct drm_i915_gem_exec_object2) +
-			   sizeof(struct i915_vma *) +
-			   sizeof(unsigned int));
 	struct drm_i915_gem_execbuffer *args = 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);
+	if (!check_buffer_count(count)) {
+		DRM_DEBUG("execbuf2 with %zd buffers\n", count);
 		return -EINVAL;
 	}
 
@@ -2485,9 +2507,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, eb_element_size(),
 				    __GFP_NOWARN | GFP_KERNEL);
 	if (exec_list == NULL || exec2_list == NULL) {
 		DRM_DEBUG("Failed to allocate exec list for %d buffers\n",
@@ -2498,7 +2520,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);
@@ -2548,16 +2570,14 @@ int
 i915_gem_execbuffer2(struct drm_device *dev, void *data,
 		     struct drm_file *file)
 {
-	const size_t sz = (sizeof(struct drm_i915_gem_exec_object2) +
-			   sizeof(struct i915_vma *) +
-			   sizeof(unsigned int));
 	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 (!check_buffer_count(count)) {
+		DRM_DEBUG("execbuf2 with %zd buffers\n", count);
 		return -EINVAL;
 	}
 
@@ -2565,17 +2585,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, eb_element_size(),
 				    __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;
 	}
-- 
2.15.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects (rev5)

[Patchwork]

== Series Details ==

Series: drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects (rev5)
URL   : https://patchwork.freedesktop.org/series/33840/
State : success

== Summary ==

Series 33840v5 drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects
https://patchwork.freedesktop.org/api/1.0/series/33840/revisions/5/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:447s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:453s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:379s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:542s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:280s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:502s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:510s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:498s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:492s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:433s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:266s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:541s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:430s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:440s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:424s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:484s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:462s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:480s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:530s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:471s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:537s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:573s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:460s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:545s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:563s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:521s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:502s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:458s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:563s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:419s

7916df8b49cb8bb9783a8425e7345cc47159f3f0 drm-tip: 2017y-11m-15d-20h-05m-48s UTC integration manifest
70a77fa98b84 drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7156/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: warning for drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects (rev4)
URL   : https://patchwork.freedesktop.org/series/33840/
State : warning

== Summary ==

Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-shrfb-draw-blt:
                pass       -> FAIL       (shard-snb) fdo#101623
        Subgroup fbc-1p-offscren-pri-indfb-draw-mmap-wc:
                skip       -> PASS       (shard-hsw)
Test drv_module_reload:
        Subgroup basic-reload-inject:
                dmesg-warn -> PASS       (shard-snb) fdo#102707
Test kms_busy:
        Subgroup extended-modeset-hang-oldfb-with-reset-render-c:
                pass       -> DMESG-WARN (shard-hsw) fdo#102249
        Subgroup extended-modeset-hang-newfb-with-reset-render-b:
                dmesg-warn -> PASS       (shard-hsw) fdo#103038
Test drv_selftest:
        Subgroup mock_sanitycheck:
                pass       -> DMESG-WARN (shard-hsw) fdo#103719
Test kms_vblank:
        Subgroup wait-busy:
                skip       -> PASS       (shard-hsw)
Test kms_concurrent:
        Subgroup pipe-c:
                skip       -> PASS       (shard-hsw)
Test pm_rpm:
        Subgroup gem-mmap-cpu:
                skip       -> PASS       (shard-hsw)
Test kms_chv_cursor_fail:
        Subgroup pipe-a-256x256-bottom-edge:
                skip       -> PASS       (shard-hsw)
        Subgroup pipe-c-128x128-bottom-edge:
                skip       -> PASS       (shard-hsw)
Test kms_cursor_legacy:
        Subgroup flip-vs-cursor-toggle:
                skip       -> PASS       (shard-hsw) fdo#102670
Test kms_flip:
        Subgroup rcs-wf_vblank-vs-dpms-interruptible:
                dmesg-warn -> PASS       (shard-hsw) fdo#102614
Test kms_plane:
        Subgroup plane-panning-bottom-right-suspend-pipe-a-planes:
                fail       -> PASS       (shard-snb) fdo#102365
        Subgroup plane-panning-bottom-right-suspend-pipe-c-planes:
                fail       -> PASS       (shard-hsw) fdo#103375
Test kms_cursor_crc:
        Subgroup cursor-64x64-suspend:
                pass       -> SKIP       (shard-snb)
                pass       -> SKIP       (shard-hsw)

fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#102707 https://bugs.freedesktop.org/show_bug.cgi?id=102707
fdo#102249 https://bugs.freedesktop.org/show_bug.cgi?id=102249
fdo#103038 https://bugs.freedesktop.org/show_bug.cgi?id=103038
fdo#103719 https://bugs.freedesktop.org/show_bug.cgi?id=103719
fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670
fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
fdo#102365 https://bugs.freedesktop.org/show_bug.cgi?id=102365
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375

shard-hsw        total:2584 pass:1469 dwarn:5   dfail:1   fail:9   skip:1100 time:9547s
shard-snb        total:2584 pass:1257 dwarn:1   dfail:1   fail:13  skip:1312 time:8019s
Blacklisted hosts:
shard-apl        total:2584 pass:1622 dwarn:1   dfail:2   fail:23  skip:936 time:13457s
shard-kbl        total:2565 pass:1699 dwarn:4   dfail:1   fail:25  skip:835 time:10529s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7155/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects (rev5)

[Patchwork]

== Series Details ==

Series: drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects (rev5)
URL   : https://patchwork.freedesktop.org/series/33840/
State : success

== Summary ==

Test kms_vblank:
        Subgroup wait-busy:
                skip       -> PASS       (shard-hsw)
Test kms_concurrent:
        Subgroup pipe-c:
                skip       -> PASS       (shard-hsw)
Test pm_rpm:
        Subgroup gem-mmap-cpu:
                skip       -> PASS       (shard-hsw)
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-indfb-draw-mmap-wc:
                skip       -> PASS       (shard-hsw)
        Subgroup fbc-1p-offscren-pri-shrfb-draw-blt:
                pass       -> FAIL       (shard-snb) fdo#101623
Test kms_chv_cursor_fail:
        Subgroup pipe-a-256x256-bottom-edge:
                skip       -> PASS       (shard-hsw)
        Subgroup pipe-c-128x128-bottom-edge:
                skip       -> PASS       (shard-hsw)
Test kms_cursor_legacy:
        Subgroup flip-vs-cursor-toggle:
                skip       -> PASS       (shard-hsw) fdo#102670
Test kms_flip:
        Subgroup rcs-wf_vblank-vs-dpms-interruptible:
                dmesg-warn -> PASS       (shard-hsw) fdo#102614
        Subgroup modeset-vs-vblank-race:
                pass       -> FAIL       (shard-hsw) fdo#103060
        Subgroup flip-vs-absolute-wf_vblank-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#100368
Test kms_plane:
        Subgroup plane-panning-bottom-right-suspend-pipe-a-planes:
                fail       -> PASS       (shard-snb) fdo#102365
        Subgroup plane-panning-bottom-right-suspend-pipe-c-planes:
                fail       -> PASS       (shard-hsw) fdo#103375
Test kms_setmode:
        Subgroup basic:
                pass       -> FAIL       (shard-hsw) fdo#99912
Test drv_selftest:
        Subgroup mock_sanitycheck:
                pass       -> DMESG-WARN (shard-snb) fdo#103717
                pass       -> DMESG-WARN (shard-hsw) fdo#103719

fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670
fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#102365 https://bugs.freedesktop.org/show_bug.cgi?id=102365
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
fdo#103717 https://bugs.freedesktop.org/show_bug.cgi?id=103717
fdo#103719 https://bugs.freedesktop.org/show_bug.cgi?id=103719

shard-hsw        total:2584 pass:1466 dwarn:6   dfail:1   fail:12  skip:1099 time:9518s
shard-snb        total:2584 pass:1256 dwarn:3   dfail:1   fail:13  skip:1311 time:8063s
Blacklisted hosts:
shard-apl        total:2565 pass:1605 dwarn:2   dfail:1   fail:20  skip:936 time:13067s
shard-kbl        total:2497 pass:1652 dwarn:5   dfail:2   fail:26  skip:810 time:10233s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7156/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6] drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects

[Tvrtko Ursulin]

On 16/11/2017 10:50, 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
> v3: Move common checks for eb1/eb2 into the same function
> v4: Put the check back for nfence*sizeof(user_fence) overflow
> v5: access_ok uses ULONG_MAX but kvmalloc_array uses SIZE_MAX
> v6: size_t and unsigned long are not type-equivalent on 32b
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 66 +++++++++++++++++++-----------
>   1 file changed, 43 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 435ed95df144..53ccb27bfe91 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -2074,23 +2074,27 @@ 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 unsigned long nfences = args->num_cliprects;
>   	struct drm_i915_gem_exec_fence __user *user;
>   	struct drm_syncobj **fences;
> -	unsigned int n;
> +	unsigned long n;
>   	int err;
>   
>   	if (!(args->flags & I915_EXEC_FENCE_ARRAY))
>   		return NULL;
>   
> -	if (nfences > SIZE_MAX / sizeof(*fences))
> +	/* Check multiplication overflow for access_ok() and kvmalloc_array() */
> +	BUILD_BUG_ON(sizeof(size_t) > sizeof(unsigned long));
> +	if (nfences > min_t(unsigned long,
> +			    ULONG_MAX / sizeof(*user),
> +			    SIZE_MAX / sizeof(*fences)))
>   		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);
>   
> -	fences = kvmalloc_array(args->num_cliprects, sizeof(*fences),
> +	fences = kvmalloc_array(nfences, sizeof(*fences),
>   				__GFP_NOWARN | GFP_KERNEL);
>   	if (!fences)
>   		return ERR_PTR(-ENOMEM);
> @@ -2447,6 +2451,26 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>   	return err;
>   }
>   
> +static size_t eb_element_size(void)
> +{
> +	return (sizeof(struct drm_i915_gem_exec_object2) +
> +		sizeof(struct i915_vma *) +
> +		sizeof(unsigned int));
> +}
> +
> +static bool check_buffer_count(size_t count)
> +{
> +	const size_t sz = eb_element_size();
> +
> +	/*
> +	 * When using LUT_HANDLE, we impose a limit of INT_MAX for the lookup
> +	 * array size (see eb_create()). Otherwise, we can accept an array as
> +	 * large as can be addressed (though use large arrays at your peril)!
> +	 */
> +
> +	return !(count < 1 || count > INT_MAX || count > SIZE_MAX / sz - 1);
> +}
> +
>   /*
>    * Legacy execbuffer just creates an exec2 list from the original exec object
>    * list array and passes it to the real function.
> @@ -2455,18 +2479,16 @@ int
>   i915_gem_execbuffer(struct drm_device *dev, void *data,
>   		    struct drm_file *file)
>   {
> -	const size_t sz = (sizeof(struct drm_i915_gem_exec_object2) +
> -			   sizeof(struct i915_vma *) +
> -			   sizeof(unsigned int));
>   	struct drm_i915_gem_execbuffer *args = 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);
> +	if (!check_buffer_count(count)) {
> +		DRM_DEBUG("execbuf2 with %zd buffers\n", count);
>   		return -EINVAL;
>   	}
>   
> @@ -2485,9 +2507,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, eb_element_size(),
>   				    __GFP_NOWARN | GFP_KERNEL);
>   	if (exec_list == NULL || exec2_list == NULL) {
>   		DRM_DEBUG("Failed to allocate exec list for %d buffers\n",
> @@ -2498,7 +2520,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);
> @@ -2548,16 +2570,14 @@ int
>   i915_gem_execbuffer2(struct drm_device *dev, void *data,
>   		     struct drm_file *file)
>   {
> -	const size_t sz = (sizeof(struct drm_i915_gem_exec_object2) +
> -			   sizeof(struct i915_vma *) +
> -			   sizeof(unsigned int));
>   	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 (!check_buffer_count(count)) {
> +		DRM_DEBUG("execbuf2 with %zd buffers\n", count);
>   		return -EINVAL;
>   	}
>   
> @@ -2565,17 +2585,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, eb_element_size(),
>   				    __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;
>   	}
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v6] drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-11-16 14:00:50)
> 
> On 16/11/2017 10:50, 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
> > v3: Move common checks for eb1/eb2 into the same function
> > v4: Put the check back for nfence*sizeof(user_fence) overflow
> > v5: access_ok uses ULONG_MAX but kvmalloc_array uses SIZE_MAX
> > v6: size_t and unsigned long are not type-equivalent on 32b
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
...
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Ta. Certainly makes type-safe languages appealing.

Pushed,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx