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; }
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;@@ -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
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
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
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
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
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
linux-stable-mirror@lists.linaro.org
-
Chris Wilson -
Joonas Lahtinen -
Tvrtko Ursulin