NULL pointer dereference in i915_gem_request_alloc()

NULL pointer dereference in i915_gem_request_alloc()

[vcaputo]

Hi Chris,

I've uncovered a bug in i915_gem_request_alloc():
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/i915/i915_gem_request.c?h=v4.9#n425

ctx here may be NULL, and i915_gem_context_get() is unconditionally
adding a reference to the supplied ctx, which makes things go boom when
NULL.

This happens for me in practice via:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/i915/intel_display.c?h=v4.9#n12317

It appears engine->last_context may be NULL.

The comment heading i915_gem_request_alloc() states that ctx shall be
NULL and that an appropriate context will be chosen automatically.  This
is not what is currently implemented.

My reproducer is an unaccelerated drm graphics toy which just sets a
mode and submits page flips using dumb buffers.  If I start Xorg first,
the bug doesn't trigger, presumably because engine->last_context gets
initialized.  But running the toy from the console immediately after
booting without starting Xorg, i915 explodes.

I would have only mailed dri-devel but my last email there seems to be
lost in a moderation queue, and I'd rather not subscribe to another
relatively high-volume list.  I've CC'd the list just in case it gets
through.

Thanks,
Vito Caputo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: NULL pointer dereference in i915_gem_request_alloc()

[Chris Wilson]

On Sun, Jan 01, 2017 at 03:16:31PM -0600, vcaputo@pengaru.com wrote:
> Hi Chris,
> 
> I've uncovered a bug in i915_gem_request_alloc():
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/i915/i915_gem_request.c?h=v4.9#n425
> 
> ctx here may be NULL, and i915_gem_context_get() is unconditionally
> adding a reference to the supplied ctx, which makes things go boom when
> NULL.

ctx is not allowed to be NULL.
 
> This happens for me in practice via:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/i915/intel_display.c?h=v4.9#n12317
> 
> It appears engine->last_context may be NULL.

It was meant to be using mmioflip if last_context was NULL, since we can
do that immediately (i.e. lower latency) than via queuing the csflip.
 
> The comment heading i915_gem_request_alloc() states that ctx shall be
> NULL and that an appropriate context will be chosen automatically.  This
> is not what is currently implemented.

Comment is wrong.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: NULL pointer dereference in i915_gem_request_alloc()

[vcaputo]

On Mon, Jan 02, 2017 at 10:09:35AM +0000, Chris Wilson wrote:
> On Sun, Jan 01, 2017 at 03:16:31PM -0600, vcaputo@pengaru.com wrote:
> > Hi Chris,
> > 
> > I've uncovered a bug in i915_gem_request_alloc():
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/i915/i915_gem_request.c?h=v4.9#n425
> > 
> > ctx here may be NULL, and i915_gem_context_get() is unconditionally
> > adding a reference to the supplied ctx, which makes things go boom when
> > NULL.
> 
> ctx is not allowed to be NULL.
>  
> > This happens for me in practice via:
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/i915/intel_display.c?h=v4.9#n12317
> > 
> > It appears engine->last_context may be NULL.
> 
> It was meant to be using mmioflip if last_context was NULL, since we can
> do that immediately (i.e. lower latency) than via queuing the csflip.
>  
> > The comment heading i915_gem_request_alloc() states that ctx shall be
> > NULL and that an appropriate context will be chosen automatically.  This
> > is not what is currently implemented.
> 
> Comment is wrong.

Ok, that makes sense, and thanks for the response.

Is it appropriate to consider it fixed upstream at this point or is
anything else needed from my end?

Cheers,
Vito Caputo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel