Initialize eb->vma[].vma pointers to NULL when the eb structure is first set up.
During the execution of eb_lookup_vmas(), the eb->vma array is successively filled up with struct eb_vma objects. This process includes calling eb_add_vma(), which might fail; however, even in the event of failure, eb->vma[i].vma is set for the currently processed buffer.
If eb_add_vma() fails, eb_lookup_vmas() returns with an error, which prompts a call to eb_release_vmas() to clean up the mess. Since eb_lookup_vmas() might fail during processing any (possibly not first) buffer, eb_release_vmas() checks whether a buffer's vma is NULL to know at what point did the lookup function fail.
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 described in [1].
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 patch changes the approach to filling them all with NULL at the start instead, rather than handling that manually during failure.
Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/15062 Fixes: 544460c33821 ("drm/i915: Multi-BB execbuf") Reported-by: Gangmin Kim km.kim1503@gmail.com Cc: stable@vger.kernel.org # 5.16.x Signed-off-by: Krzysztof Niemiec krzysztof.niemiec@intel.com --- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 27 ++++++------------- 1 file changed, 8 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index b057c2fa03a4..02120203af55 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -951,13 +951,13 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb) vma = eb_lookup_vma(eb, eb->exec[i].handle); if (IS_ERR(vma)) { err = PTR_ERR(vma); - goto err; + return err; }
err = eb_validate_vma(eb, &eb->exec[i], vma); if (unlikely(err)) { i915_vma_put(vma); - goto err; + return err; }
err = eb_add_vma(eb, ¤t_batch, i, vma); @@ -966,19 +966,8 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
if (i915_gem_object_is_userptr(vma->obj)) { err = i915_gem_object_userptr_submit_init(vma->obj); - if (err) { - if (i + 1 < eb->buffer_count) { - /* - * Execbuffer code expects last vma entry to be NULL, - * since we already initialized this entry, - * set the next value to NULL or we mess up - * cleanup handling. - */ - eb->vma[i + 1].vma = NULL; - } - + if (err) return err; - }
eb->vma[i].flags |= __EXEC_OBJECT_USERPTR_INIT; eb->args->flags |= __EXEC_USERPTR_USED; @@ -986,10 +975,6 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb) }
return 0; - -err: - eb->vma[i].vma = NULL; - return err; }
static int eb_lock_vmas(struct i915_execbuffer *eb) @@ -3362,6 +3347,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, struct sync_file *out_fence = NULL; int out_fence_fd = -1; int err; + int i;
BUILD_BUG_ON(__EXEC_INTERNAL_FLAGS & ~__I915_EXEC_ILLEGAL_FLAGS); BUILD_BUG_ON(__EXEC_OBJECT_INTERNAL_FLAGS & @@ -3375,7 +3361,10 @@ 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; + + for (i = 0; i < args->buffer_count; i++) + eb.vma[i].vma = NULL; + eb.batch_pool = NULL;
eb.invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS;
Re-sending because of my response unintentionally HTML formatted, with correct email address of Tvrtko by the way.
Hi Krzysztof,
On Tuesday, 25 November 2025 14:33:38 CET Krzysztof Niemiec wrote:
Initialize eb->vma[].vma pointers to NULL when the eb structure is first set up.
During the execution of eb_lookup_vmas(), the eb->vma array is successively filled up with struct eb_vma objects. This process includes calling eb_add_vma(), which might fail; however, even in the event of failure, eb->vma[i].vma is set for the currently processed buffer.
If eb_add_vma() fails, eb_lookup_vmas() returns with an error, which prompts a call to eb_release_vmas() to clean up the mess. Since eb_lookup_vmas() might fail during processing any (possibly not first) buffer, eb_release_vmas() checks whether a buffer's vma is NULL to know at what point did the lookup function fail.
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 described in [1].
When entering eb_lookup_vmas(), the vma pointers are set to the slab poison value, instead of NULL.
Your commit description still doesn't answer my question why the whole memory area allocated to the table of VMAs is not initialized to 0 on allocation, only left populated with poison values.
Thanks, Janusz
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 patch changes the approach to filling them all with NULL at the start instead, rather than handling that manually during failure.
Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/15062 Fixes: 544460c33821 ("drm/i915: Multi-BB execbuf") Reported-by: Gangmin Kim km.kim1503@gmail.com Cc: stable@vger.kernel.org # 5.16.x Signed-off-by: Krzysztof Niemiec krzysztof.niemiec@intel.com
.../gpu/drm/i915/gem/i915_gem_execbuffer.c | 27 ++++++------------- 1 file changed, 8 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index b057c2fa03a4..02120203af55 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -951,13 +951,13 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb) vma = eb_lookup_vma(eb, eb->exec[i].handle); if (IS_ERR(vma)) { err = PTR_ERR(vma);
goto err;
return err;err = eb_validate_vma(eb, &eb->exec[i], vma); if (unlikely(err)) { i915_vma_put(vma);
goto err;
return err;err = eb_add_vma(eb, ¤t_batch, i, vma); @@ -966,19 +966,8 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb) if (i915_gem_object_is_userptr(vma->obj)) { err = i915_gem_object_userptr_submit_init(vma->obj);
if (err) {if (i + 1 < eb->buffer_count) {/** Execbuffer code expects last vma entry to be NULL,* since we already initialized this entry,* set the next value to NULL or we mess up* cleanup handling.*/eb->vma[i + 1].vma = NULL;}
if (err) return err;
}eb->vma[i].flags |= __EXEC_OBJECT_USERPTR_INIT; eb->args->flags |= __EXEC_USERPTR_USED; @@ -986,10 +975,6 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb) } return 0;
-err:
- eb->vma[i].vma = NULL;
- return err;
} static int eb_lock_vmas(struct i915_execbuffer *eb) @@ -3362,6 +3347,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, struct sync_file *out_fence = NULL; int out_fence_fd = -1; int err;
- int i;
BUILD_BUG_ON(__EXEC_INTERNAL_FLAGS & ~__I915_EXEC_ILLEGAL_FLAGS); BUILD_BUG_ON(__EXEC_OBJECT_INTERNAL_FLAGS & @@ -3375,7 +3361,10 @@ 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;
- for (i = 0; i < args->buffer_count; i++)
eb.vma[i].vma = NULL;- eb.batch_pool = NULL;
eb.invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS;
linux-stable-mirror@lists.linaro.org
-
Janusz Krzysztofik -
Krzysztof Niemiec