[Intel-gfx] [PATCH v4] drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Nov 16 09:00:08 UTC 2017
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 at oracle.com>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 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
More information about the Intel-gfx
mailing list