Re: [Intel-gfx] [PATCH] drm/i915: Sanity check mmap length against object size

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 14/03/2019 07:58, Chris Wilson wrote:
> We assumed that vm_mmap() would reject an attempt to mmap past the end of
> the filp (our object), but we were wrong.
> 
> Reported-by: Antonio Argenziano <antonio.argenziano@intel.com>
> Testcase: igt/gem_mmap/bad-size
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Antonio Argenziano <antonio.argenziano@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: stable@vger.kernel.org
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index b38c9531b5e8..b7086c8d4726 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1639,8 +1639,13 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
>   	 * pages from.
>   	 */
>   	if (!obj->base.filp) {
> -		i915_gem_object_put(obj);
> -		return -ENXIO;
> +		addr = -ENXIO;
> +		goto err;
> +	}
> +
> +	if (range_overflows(args->offset, args->size, (u64)obj->base.size)) {
> +		addr = -EINVAL;
> +		goto err;
>   	}
>   
>   	addr = vm_mmap(obj->base.filp, 0, args->size,
> @@ -1654,8 +1659,8 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
>   		struct vm_area_struct *vma;
>   
>   		if (down_write_killable(&mm->mmap_sem)) {
> -			i915_gem_object_put(obj);
> -			return -EINTR;
> +			addr = -EINTR;
> +			goto err;
>   		}
>   		vma = find_vma(mm, addr);
>   		if (vma && __vma_matches(vma, obj->base.filp, addr, args->size))
> @@ -1673,12 +1678,10 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
>   	i915_gem_object_put(obj);
>   
>   	args->addr_ptr = (u64)addr;
> -
>   	return 0;
>   
>   err:
>   	i915_gem_object_put(obj);
> -
>   	return addr;
>   }
>   
> 

Patch is good, and certainly for our use cases we can afford to check at 
this level.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

I am only wondering what happens to reads/write to the trailing area? 
Does shmemfs expands the backing store for this mmap and we just end up 
with otherwise unused chunk at the end?

Regards,

Tvrtko