Hi Krzysztof, ...
> In eb_lookup_vmas(), eb->vma[i].vma is set to NULL if either the helper > function eb_lookup_vma() or eb_validate_vma() fails. eb->vma[i+1].vma is > set to NULL in case i915_gem_object_userptr_submit_init() fails; the > current one needs to be cleaned up by eb_release_vmas() at this point, > so the next one is set. If eb_add_vma() fails, neither the current nor > the next vma is nullified, which is a source of a NULL deref bug /nullified/set to NULL/ > described in [1]. The [1] reference is out of the commit log :-) > When entering eb_lookup_vmas(), the vma pointers are set to the slab > poison value, instead of NULL. This doesn't matter for the actual > lookup, since it gets overwritten anyway, however the eb_release_vmas() > function only recognizes NULL as the stopping value, hence the pointers > are being nullified as they go in case of intermediate failure. This /nullified/set to NULL/ > patch changes the approach to filling them all with NULL at the start > instead, rather than handling that manually during failure. ... > static int eb_lock_vmas(struct i915_execbuffer *eb) > @@ -3375,7 +3360,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, > > eb.exec = exec; > eb.vma = (struct eb_vma *)(exec + args->buffer_count + 1); > - eb.vma[0].vma = NULL; > + memset(eb.vma, 0x00, args->buffer_count * sizeof(struct eb_vma)); /0x00/0/ Should this be buffer_count + 1? > + > eb.batch_pool = NULL; > > eb.invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS; > @@ -3584,7 +3570,18 @@ i915_gem_execbuffer2_ioctl(struct drm_device *dev, > void *data, > if (err) > return err; > > - /* Allocate extra slots for use by the command parser */ > + /* > + * Allocate extra slots for use by the command parser. > + * > + * Note that this allocation handles two different arrays (the > + * exec2_list array, and the eventual eb.vma array introduced in > + * i915_gem_do_execubuffer()), that reside in virtually contiguous /execubuffer/execbuffer/ > + * memory. Also note that the allocation intentionally doesn't fill the > + * area with zeros (because the exec2_list part doesn't need to be, as > + * it's immediately overwritten by user data a few lines below). No need to put this last sentence within brackets, it's just a continuation sentence. Except for the "+ 1" note, everything is trivial: Reviewed-by: Andi Shyti <[email protected]> Given that this has failed in patchwork, can you please resend a v5 if there are no other comments from your side? Thanks, Andi > + * However, the eb.vma part is explicitly zeroed later in > + * i915_gem_do_execbuffer(). > + */ > exec2_list = kvmalloc_array(count + 2, eb_element_size(), > __GFP_NOWARN | GFP_KERNEL); > if (exec2_list == NULL) { > -- > 2.45.2 >
