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]> -- Best Regards, Krzysztof
