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

[Matthew Auld <matthew.auld@intel.com>]

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).

> 
> Fixes: 26b15eb0 ("drm/i915/ttm: implement access_memory")
> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> Suggested-by: John C Harrison <John.C.Harrison@intel.com>
> CC: Matthew Auld <matthew.auld@intel.com>
> CC: Andrzej Hajda <andrzej.hajda@intel.com>
> CC: Nirmoy Das <nirmoy.das@intel.com>
> CC: Andi Shyti <andi.shyti@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index d63f30efd631..b569624f2ed9 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -704,11 +704,16 @@ static int i915_ttm_access_memory(struct ttm_buffer_object *bo,
>   				  int len, int write)
>   {
>   	struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
> -	resource_size_t iomap = obj->mm.region->iomap.base -
> -		obj->mm.region->region.start;
> +	resource_size_t iomap;
>   	unsigned long page = offset >> PAGE_SHIFT;
>   	unsigned long bytes_left = len;
>   
> +	if (!obj)
> +		return -EINVAL;
> +
> +	iomap = obj->mm.region->iomap.base -
> +		obj->mm.region->region.start;
> +
>   	/*
>   	 * TODO: For now just let it fail if the resource is non-mappable,
>   	 * otherwise we need to perform the memcpy from the gpu here, without