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. 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 + * reason as above. */ eb->vma[i + 1].vma = NULL; } -- 2.45.2
