[Intel-gfx] [PATCH] drm/i915/ttm: Fix access_memory null pointer exception

[Andi Shyti]

Hi Matt,

On Fri, Oct 14, 2022 at 10:44:11AM +0100, Matthew Auld wrote:
> On 14/10/2022 09:56, Andi Shyti wrote:
> > On Fri, Oct 14, 2022 at 09:39:52AM +0100, Matthew Auld wrote:
> > > On 13/10/2022 18:56, Jonathan Cavitt wrote:
> > > > i915_ttm_to_gem can return a NULL pointer, which is
> > > > dereferenced in i915_ttm_access_memory without first
> > > > checking if it is NULL.  Inspecting
> > > > i915_ttm_io_mem_reserve, it appears the correct
> > > > behavior in this case is to return -EINVAL.
> > > 
> > > The GEM object has already been dereferenced before this point, if you look
> > > at the caller (vm_access_ttm). The NULL obj thing is to identify "ttm ghost
> > > objects", and I don't think a normal userpace object can suddenly become one
> > > (access_memory comes from ptrace). AFAIK ghost objects are just for
> > > temporarily hanging on to some memory/state, while the dma-resv is busy. In
> > > the places where ttm is the one giving us the object, then it might be
> > > possible to see these types of objects, since ttm could in theory pass one
> > > in (like during eviction).
> > 
> > True that, but because from a code persepctive we can still receive
> > NULL, I think the check is correct, perhaps we could:
> > 
> > 	if (unlikely(!obj))
> > 		return -EINVAL;
> 
> Hmm, so that will dereference some pointer, and then later check if it is
> NULL here? Or do you mean to move this into vm_access()? If we are given a
> "ghost object" for ptrace this would likely mean we have a very nasty bug
> somewhere (unless I'm misunderstanding something), and so returning a normal
> user error here doesn't seem right to me (maybe this just hides the issue)?
> Letting it crash seems fine to me tbh. It also makes the code harder to
> understand IMO, because looking at this it now suggests that it is somehow
> possible to have a "ghost object" here. Also there are a fair few places
> calling i915_ttm_to_gem() which already don't check for NULL, since it
> should be impossible, like it should be here.

By just analyzing the code, getting NULL is not impossible. In
that case even a GEM_BUG_ON would have worked. But the NULL
pointer, as it is, needs to be checked.

Anyway, I see that an agreement has been reached with Nirmoy, so
that it doesn't matter anymore :)

Andi