On 31/10/2025 11:42, Krzysztof Karas wrote:
Hi Krzysztof,
Set eb->vma[i+1].vma to NULL to prevent eb_release_vmas() from
processing unitialized data, leading to a potential NULL dereference.
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 processes buffer.
processes -> processed
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
which one has failed first. The NULL is set if the vma cannot be set or
is invalid in some way, but during and after the eb_add_vma() call, it
is set to a valid pointer for the currently processed eb_vma.
This means that during the check in eb_release_vmas(), the buffer that
failed eb_add_vma() (say, eb->vma[i]) is processed (and rightfully so,
since the vma associated with it still needs cleanup), but eb->vma[i+1]
is left completely uninitialized (since the loop was broken prematurely
after failing on eb_add_vma() for eb->vma[i]). Therefore
eb->vma[i+1].vma has junk in it, and if that junk is not NULL, that vma
will be processed by eb_release_vmas(), leading to memory corruption.
Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/15062
Reported-by: 김강민 <[email protected]>
Signed-off-by: Krzysztof Niemiec <[email protected]>
---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 39c7c32e1e74..0f8f02e22c03 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -960,18 +960,27 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
}
err = eb_add_vma(eb, ¤t_batch, i, vma);
- if (err)
+ 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;
+ }
+
return err;
+ }
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.
+ * Set the next vma to null, for the
same
null -> NULL to be more explicit and to match the previous
comment.
+ * reason as above.
*/
eb->vma[i + 1].vma = NULL;
}
After above are addressed (maybe during merge, to avoid
re-sending?):
Reviewed-by: Krzysztof Karas <[email protected]>
Could you please also figure out the right Fixes: / stable?
I initially suspected either 544460c33821 or ed29c2691188 could be
related. In any case bug looks serious enough to warrant backporting for
stable.
Regards,
Tvrtko
