| Message ID | 20190314075829.16838-1-chris@chris-wilson.co.uk |
|---|---|
| State | Accepted |
| Commit | 794a11cb67201ad1bb61af510bb8460280feb3f3 |
| Headers | show |
| Series |
"drm/i915: Sanity check mmap length against object size"
( rev:
2
1
)
in
Intel GFX |
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; > } > 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
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 >
Hi,
[This is an automated email]
This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all
The bot has tested the following trees: v5.0.3, v4.19.30, v4.14.107, v4.9.164, v4.4.176, v3.18.136.
v5.0.3: Failed to apply! Possible dependencies:
739f3abdbfcf ("drm/i915: small isolated c99 types to kernel types switch")
ebfb6977801d ("drm/i915: Handle vm_mmap error during I915_GEM_MMAP ioctl with WC set")
v4.19.30: Failed to apply! Possible dependencies:
739f3abdbfcf ("drm/i915: small isolated c99 types to kernel types switch")
ebfb6977801d ("drm/i915: Handle vm_mmap error during I915_GEM_MMAP ioctl with WC set")
f28ec6f4ea48 ("drm/i915: Constify power well descriptors")
v4.14.107: Failed to apply! Possible dependencies:
0d6fc92a73e0 ("drm/i915: Separate RPS and RC6 handling for VLV")
274b2462a049 ("drm/i915: Object w/o backing storage is banned by -ENXIO")
3e8ddd9e5071 ("drm/i915: Nuke some bogus tabs from the pcode defines")
48469eced282 ("drm/i915: Use cdclk_state->voltage on CNL")
5161d058dff4 ("drm/i915: Fix BXT lane latency optimal setting with MST")
53e9bf5e8159 ("drm/i915: Adjust system agent voltage on CNL if required by DDI ports")
61843f0e6212 ("drm/i915: Name the IPS_PCODE_CONTROL bit")
739f3abdbfcf ("drm/i915: small isolated c99 types to kernel types switch")
960e54652cee ("drm/i915: Separate RPS and RC6 handling for gen6+")
9f817501bd7f ("drm/i915: Move rps.hw_lock to dev_priv and s/hw_lock/pcu_lock")
d305e0614601 ("drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"")
d46b00dc38c8 ("drm/i915: Separate RPS and RC6 handling for CHV")
v4.9.164: Failed to apply! Possible dependencies:
0e70447605f4 ("drm/i915: Move common code out of i915_gpu_error.c")
1b36595ffb35 ("drm/i915: Show RING registers through debugfs")
3b3f1650b1ca ("drm/i915: Allocate intel_engine_cs structure only for the enabled engines")
9c870d03674f ("drm/i915: Use RPM as the barrier for controlling user mmap access")
bb6dc8d96b68 ("drm/i915: Implement pread without struct-mutex")
d636951ec01b ("drm/i915: Cleanup instdone collection")
f0cd518206e1 ("drm/i915: Use lockless object free")
f9e613728090 ("drm/i915: Try to print INSTDONE bits for all slice/subslice")
v4.4.176: Failed to apply! Possible dependencies:
03ac0642f67a ("drm/i915: Wrap drm_gem_object_lookup in i915_gem_object_lookup")
1b5708ffb103 ("drm/amdgpu: export amd_powerplay_func to amdgpu and other ip block")
1ea863fd736e ("drm/amdgpu: keep the prefered/allowed domains in the BO")
1f7371b2a5fa ("drm/amd/powerplay: add basic powerplay framework")
288912cb95d1 ("drm/amdgpu: use $(src) in Makefile (v2)")
2a7d9bdabec2 ("drm/amdgpu: cleanup amdgpu_cs_parser_relocs")
2f4b9400336e ("drm/amdgpu: clean up hw semaphore support in driver")
36409d122cb8 ("drm/amdgpu: cleanup amdgpu_cs_list_validate")
3a2c788d95a2 ("drm/amdgpu: share struct amdgpu_pm_state_type with powerplay module")
3af76f23a45b ("drm/amdgpu: export fan control functions to amdgpu")
3c0eea6c35d9 ("drm/amdgpu: put VM page tables directly into duplicates list")
4ff37a83f19d ("drm/amdgpu: fix VM faults caused by vm_grab_id() v4")
56467ebfb254 ("drm/amdgpu: split VM PD and PT handling during CS")
636ce25c3001 ("drm/amdgpu: cleanup bo list bucket handling")
758ac17f963f ("drm/amdgpu: fix and cleanup user fence handling v2")
8d0a7cea824a ("drm/amdgpu: grab VMID before submitting job v5")
a8ad0bd84f98 ("drm: Remove unused drm_device from drm_gem_object_lookup()")
be86c606b50a ("drm/amdgpu: cleanup amdgpu_sync_rings V2")
c5637837ba5d ("drm/amdgpu: keep vm in job instead of ib (v2)")
cc325d191347 ("drm/amdgpu: check userptrs mm earlier")
d8e0cae64550 ("drm/amdgpu: validate duplicates first")
e61710c59dd2 ("drm/amdgpu: support per device powerplay enablement (v2)")
edf600dac65e ("drm/amd: cleanup remaining spaces and tabs v2")
ee1782c3f27f ("drm/amdgpu: keep the PTs validation list in the VM v2")
f69f90a113f2 ("drm/amdgpu: fix amdgpu_cs_get_threshold_for_moves handling")
v3.18.136: Failed to apply! Possible dependencies:
03ac0642f67a ("drm/i915: Wrap drm_gem_object_lookup in i915_gem_object_lookup")
049fc527b464 ("drm/amdgpu: dispatch jobs in cs")
1d263474c441 ("drm/amdgpu: unwind properly in amdgpu_cs_parser_init()")
2a7d9bdabec2 ("drm/amdgpu: cleanup amdgpu_cs_parser_relocs")
3cb485f34049 ("drm/amdgpu: fix context switch")
46651cc5dbee ("drm/amdgpu fix amdgpu.dpm=0 (v2)")
564ea7900cff ("drm/amdgpu: enable uvd dpm and powergating")
5fc3aeeb9e55 ("drm/amdgpu: rename amdgpu_ip_funcs to amd_ip_funcs (v2)")
636ce25c3001 ("drm/amdgpu: cleanup bo list bucket handling")
72efa7ebdea0 ("drm/amdgpu: check context id for context switching (v2)")
81629cba1f12 ("drm/amdgpu: add amdgpu uapi header (v4)")
840d51445f15 ("drm/amdgpu: fix bug occurs when bo_list is NULL")
8e9198d0698a ("drm/amdgpu: move some atombios definitions to common folder (v2)")
97b2e202fba0 ("drm/amdgpu: add amdgpu.h (v2)")
a2e73f56fa62 ("drm/amdgpu: Add support for CIK parts")
a3348bb801ba ("drm/amdgpu: don't need to use bo_list_clone any more")
a5b750583eb4 ("drm/amdgpu: validate duplicates in the CS as well")
a8ad0bd84f98 ("drm: Remove unused drm_device from drm_gem_object_lookup()")
a961ea7349d0 ("drm/amdgpu: fix userptr lockup")
aa2bdb247620 ("drm/amdgpu: add CE preamble flag v3")
aaa36a976bbb ("drm/amdgpu: Add initial VI support")
b80d8475c1fd ("drm/amdgpu: add scheduler initialization")
c1b69ed0c62f ("drm/amdgpu: add backend implementation of gpu scheduler (v2)")
cc325d191347 ("drm/amdgpu: check userptrs mm earlier")
d2edb07b10fc ("drm/amdgpu: cleanup HDP flush handling")
d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
d7006964d46d ("drm/amdgpu: fix issue with overlapping userptrs")
d919ad49ac04 ("drm/amdgpu: fix dereference before check")
de807f818b95 ("drm/amdgpu: add flags for amdgpu_ib structure")
How should we proceed with this patch?
--
Thanks,
Sasha
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(-)