On 09/03/2022 11:19, Katragadda, MastanX wrote:
Hi,
can we have ack? or we need to do anything further to get r-o-b.
There was just the potential strangeness around len <= 0, and exactly
how we are meant to handle that, but if you are confident that is
already covered in a sane way, then feel free to add,
Reviewed-by: Matthew Auld <matthew.auld@xxxxxxxxx>
Thanks,
Mastan
-----Original Message-----
From: Katragadda, MastanX
Sent: 09 March 2022 07:16
To: Auld, Matthew <matthew.auld@xxxxxxxxx>; Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Surendrakumar Upadhyay, TejaskumarX <tejaskumarx.surendrakumar.upadhyay@xxxxxxxxx>
Subject: RE: [v2] drm/i915/gem: missing boundary check in vm_access leads to OOB read/write
On 03/03/2022 09:00, Tvrtko Ursulin wrote:
+ Matt
On 03/03/2022 06:04, Mastan Katragadda wrote:
Intel ID: PSIRT-PTK0002429
A missing bounds check in vm_access()can lead to an out-of-bounds
read or write in the adjacent memory area.The len attribute is not
validated before the memcpy at [1]or [2] occurs.
s/[1]or [2]/later in the function/ ?
[ 183.637831] BUG: unable to handle page fault for address:
ffffc90000c86000
[ 183.637934] #PF: supervisor read access in kernel mode [
183.637997] #PF: error_code(0x0000) - not-present page [ 183.638059]
PGD 100000067 P4D 100000067 PUD 100258067 PMD 106341067 PTE 0 [
183.638144] Oops: 0000 [#2] PREEMPT SMP NOPTI
[ 183.638201] CPU: 3 PID: 1790 Comm: poc Tainted: G D
5.17.0-rc6-ci-drm-11296+ #1
[ 183.638298] Hardware name: Intel Corporation CoffeeLake Client
Platform/CoffeeLake H DDR4 RVP, BIOS CNLSFWR1.R00.X208.B00.1905301319
05/30/2019
[ 183.638430] RIP: 0010:memcpy_erms+0x6/0x10 [ 183.640213] RSP:
0018:ffffc90001763d48 EFLAGS: 00010246 [ 183.641117] RAX:
ffff888109c14000 RBX: ffff888111bece40 RCX:
0000000000000ffc
[ 183.642029] RDX: 0000000000001000 RSI: ffffc90000c86000 RDI:
ffff888109c14004
[ 183.642946] RBP: 0000000000000ffc R08: 800000000000016b R09:
0000000000000000
[ 183.643848] R10: ffffc90000c85000 R11: 0000000000000048 R12:
0000000000001000
[ 183.644742] R13: ffff888111bed190 R14: ffff888109c14000 R15:
0000000000001000
[ 183.645653] FS: 00007fe5ef807540(0000) GS:ffff88845b380000(0000)
knlGS:0000000000000000
[ 183.646570] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [
183.647481] CR2: ffffc90000c86000 CR3: 000000010ff02006 CR4:
00000000003706e0
[ 183.648384] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[ 183.649271] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
0000000000000400
[ 183.650142] Call Trace:
[ 183.650988] <TASK>
[ 183.651793] vm_access+0x1f0/0x2a0 [i915] [ 183.652726]
__access_remote_vm+0x224/0x380 [ 183.653561]
mem_rw.isra.0+0xf9/0x190 [ 183.654402] vfs_read+0x9d/0x1b0 [
183.655238] ksys_read+0x63/0xe0 [ 183.656065]
do_syscall_64+0x38/0xc0 [ 183.656882]
entry_SYSCALL_64_after_hwframe+0x44/0xae
[ 183.657663] RIP: 0033:0x7fe5ef725142 [ 183.659351] RSP:
002b:00007ffe1e81c7e8 EFLAGS: 00000246 ORIG_RAX:
0000000000000000
[ 183.660227] RAX: ffffffffffffffda RBX: 0000557055dfb780 RCX:
00007fe5ef725142
[ 183.661104] RDX: 0000000000001000 RSI: 00007ffe1e81d880 RDI:
0000000000000005
[ 183.661972] RBP: 00007ffe1e81e890 R08: 0000000000000030 R09:
0000000000000046
[ 183.662832] R10: 0000557055dfc2e0 R11: 0000000000000246 R12:
0000557055dfb1c0
[ 183.663691] R13: 00007ffe1e81e980 R14: 0000000000000000 R15:
0000000000000000
[ 183.664566] </TASK>
Changes since v1:
- Updated if condition with range_overflows_t [Chris Wilson]
Signed-off-by: Mastan Katragadda <mastanx.katragadda@xxxxxxxxx>
Suggested-by: Adam Zabrocki <adamza@xxxxxxxxxxxxx>
Reported-by: Jackson Cody <cody.jackson@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Bloomfield Jon <jon.bloomfield@xxxxxxxxx>
Cc: Dutt Sudeep <sudeep.dutt@xxxxxxxxx>
Fixes: 9f909e215fea ("drm/i915: Implement vm_ops->access for gdb
access into mmaps")
Cc: <stable@xxxxxxxxxxxxxxx> # v5.8+
Right?
There was a selftest added with the referenced patch and it sounds
like it would be a good idea to extend it to cover this scenario. As
a separate patch though so this one is easy to backport.
Agreed, a simple regression test(either selftest or igt) for this would be nice, if possible.
Regards,
Tvrtko
---
drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index efe69d6b86f4..c3ea243d414d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -455,7 +455,7 @@ vm_access(struct vm_area_struct *area, unsigned
long addr,
return -EACCES;
addr -= area->vm_start;
- if (addr >= obj->base.size)
+ if (range_overflows_t(u64, addr, len, obj->base.size))
return -EINVAL;
Other users like ttm_bo_vm_access are also checking if len <= 0, should we also add an explicit check for that here? Otherwise LGTM.
I think no need to add here len<=0, we already validating same range_overflows_t . converted following condition to range_overflow_t.
if (len < 1 || len > obj->base.size ||
addr >= obj->base.size ||
addr + len > obj->base.size)
i915_gem_ww_ctx_init(&ww, true);
