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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
