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@xxxxxxxxxx>
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
