[PATCH] drm/i915: Sanity check mmap length against object size

[PATCH] drm/i915: Sanity check mmap length against object size

[Chris Wilson]

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;
 }
 
-- 
2.20.1

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

[Tvrtko Ursulin]

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

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

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-03-14 11:33:43)
> 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?

My expectation would be that they generate a SIGBUS since the filp
should not be extended to cover the absent pages. So it would be the
equivalent of mmaping a file then calling ftruncate(0).

I admit it's not obvious if shmem_getpage_gfp (backing shmem_fault)
would prevent allocation of fresh backing pages beyond the initial filp
size. Afaict, we would end up at alloc_page_vma() without rejecting an
index beyond the end of the file.
-Chris

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

[Chris Wilson]

Quoting Chris Wilson (2019-03-14 11:44:37)
> Quoting Tvrtko Ursulin (2019-03-14 11:33:43)
> > 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?
> 
> My expectation would be that they generate a SIGBUS since the filp
> should not be extended to cover the absent pages. So it would be the
> equivalent of mmaping a file then calling ftruncate(0).

Ok, having just checked, what actually happens is that shmemfs quite
happily allocates the extra page beyond the end of the object and
userspace can freely read/write into that address space with only the
mere consequence that those pages are not mapped to the GPU.
-Chris

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

[Chris Wilson]

Quoting Chris Wilson (2019-03-18 12:10:12)
> Quoting Chris Wilson (2019-03-14 11:44:37)
> > Quoting Tvrtko Ursulin (2019-03-14 11:33:43)
> > > 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?
> > 
> > My expectation would be that they generate a SIGBUS since the filp
> > should not be extended to cover the absent pages. So it would be the
> > equivalent of mmaping a file then calling ftruncate(0).
> 
> Ok, having just checked, what actually happens is that shmemfs quite
> happily allocates the extra page beyond the end of the object and
> userspace can freely read/write into that address space with only the
> mere consequence that those pages are not mapped to the GPU.

Or egg-on-face moment, wrong kernel (already had the safety check!)

ickle@kabylake:~/intel-gpu-tools$ sudo ./build/tests/gem_mmap --run bad-size
IGT-Version: 1.23-g3fc026d3e (x86_64) (Linux: 5.0.0+ x86_64)
Starting subtest: bad-size
Received signal SIGBUS.
Stack trace:
 #0 [fatal_sig_handler+0xd5]
 #1 [killpg+0x40]
 #2 [__real_main119+0x1b6]
 #3 [main+0x44]
 #4 [__libc_start_main+0xeb]
 #5 [_start+0x2a]
Subtest bad-size: CRASH (0.001s)

SIGBUS!
-Chris

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

[Chris Wilson]

Quoting Chris Wilson (2019-03-14 07:58:29)
> We assumed that vm_mmap() would reject an attempt to mmap past the end of
> the filp (our object), but we were wrong.

Applications that tried to use the mmap beyond the end of the object
would be greeted by a SIGBUS.

> 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

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

[Joonas Lahtinen]

Quoting Chris Wilson (2019-03-14 09:58:29)
> 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

With the SIGBUS => EINVAL difference documented this is:

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas

> ---
>  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;
>  }
>  
> -- 
> 2.20.1
>