* [bug report] drm/i915: Eliminate lots of iterations over the execobjects array
@ 2018-03-15 13:10 Dan Carpenter
2018-03-15 13:17 ` Chris Wilson
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2018-03-15 13:10 UTC (permalink / raw)
To: chris; +Cc: intel-gfx
Hello Chris Wilson,
The patch 2889caa92321: "drm/i915: Eliminate lots of iterations over
the execobjects array" from Jun 16, 2017, leads to the following
static checker warning:
drivers/gpu/drm/i915/i915_gem_execbuffer.c:2546 i915_gem_execbuffer_ioctl()
warn: calling '__copy_to_user()' without access_ok()
drivers/gpu/drm/i915/i915_gem_execbuffer.c
2510 err = copy_from_user(exec_list,
2511 u64_to_user_ptr(args->buffers_ptr),
2512 sizeof(*exec_list) * count);
2513 if (err) {
2514 DRM_DEBUG("copy %d exec entries failed %d\n",
2515 args->buffer_count, err);
2516 kvfree(exec_list);
2517 kvfree(exec2_list);
2518 return -EFAULT;
2519 }
2520
2521 for (i = 0; i < args->buffer_count; i++) {
2522 exec2_list[i].handle = exec_list[i].handle;
2523 exec2_list[i].relocation_count = exec_list[i].relocation_count;
2524 exec2_list[i].relocs_ptr = exec_list[i].relocs_ptr;
2525 exec2_list[i].alignment = exec_list[i].alignment;
2526 exec2_list[i].offset = exec_list[i].offset;
2527 if (INTEL_GEN(to_i915(dev)) < 4)
2528 exec2_list[i].flags = EXEC_OBJECT_NEEDS_FENCE;
2529 else
2530 exec2_list[i].flags = 0;
2531 }
2532
2533 err = i915_gem_do_execbuffer(dev, file, &exec2, exec2_list, NULL);
2534 if (exec2.flags & __EXEC_HAS_RELOC) {
2535 struct drm_i915_gem_exec_object __user *user_exec_list =
2536 u64_to_user_ptr(args->buffers_ptr);
2537
2538 /* Copy the new buffer offsets back to the user's exec list. */
2539 for (i = 0; i < args->buffer_count; i++) {
2540 if (!(exec2_list[i].offset & UPDATE))
2541 continue;
2542
2543 exec2_list[i].offset =
2544 gen8_canonical_addr(exec2_list[i].offset & PIN_OFFSET_MASK);
2545 exec2_list[i].offset &= PIN_OFFSET_MASK;
2546 if (__copy_to_user(&user_exec_list[i].offset,
2547 &exec2_list[i].offset,
^^^^^^^^^^^^^^^^^^^^
2548 sizeof(user_exec_list[i].offset)))
2549 break;
The story of this warning is that one day Linus was grumpy about
security issues and said something like, "We should make it a rule that
code which uses __copy_to_user() should call access_ok() in that exact
same function or it becomes too hard to audit and error prone. Can
someone write a static checker for this?" And so I did. But up to now
I've always just looked at the code and either figured out where the
access_ok() is or just assumed that "Probably it's there if I looked
harder".
But today I've drawn a line in the sand! No more!
Also the error code is wrong, we should return -EFAULT if the copy
fails. ;)
2550 }
2551 }
2552
2553 kvfree(exec_list);
regards,
dan carpenter
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] drm/i915: Eliminate lots of iterations over the execobjects array
2018-03-15 13:10 [bug report] drm/i915: Eliminate lots of iterations over the execobjects array Dan Carpenter
@ 2018-03-15 13:17 ` Chris Wilson
2018-03-15 13:33 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Chris Wilson @ 2018-03-15 13:17 UTC (permalink / raw)
To: Dan Carpenter; +Cc: intel-gfx
Quoting Dan Carpenter (2018-03-15 13:10:30)
> Hello Chris Wilson,
>
> The patch 2889caa92321: "drm/i915: Eliminate lots of iterations over
> the execobjects array" from Jun 16, 2017, leads to the following
> static checker warning:
>
> drivers/gpu/drm/i915/i915_gem_execbuffer.c:2546 i915_gem_execbuffer_ioctl()
> warn: calling '__copy_to_user()' without access_ok()
>
> drivers/gpu/drm/i915/i915_gem_execbuffer.c
> 2510 err = copy_from_user(exec_list,
> 2511 u64_to_user_ptr(args->buffers_ptr),
> 2512 sizeof(*exec_list) * count);
> 2513 if (err) {
> 2514 DRM_DEBUG("copy %d exec entries failed %d\n",
> 2515 args->buffer_count, err);
> 2516 kvfree(exec_list);
> 2517 kvfree(exec2_list);
> 2518 return -EFAULT;
> 2519 }
> 2520
> 2521 for (i = 0; i < args->buffer_count; i++) {
> 2522 exec2_list[i].handle = exec_list[i].handle;
> 2523 exec2_list[i].relocation_count = exec_list[i].relocation_count;
> 2524 exec2_list[i].relocs_ptr = exec_list[i].relocs_ptr;
> 2525 exec2_list[i].alignment = exec_list[i].alignment;
> 2526 exec2_list[i].offset = exec_list[i].offset;
> 2527 if (INTEL_GEN(to_i915(dev)) < 4)
> 2528 exec2_list[i].flags = EXEC_OBJECT_NEEDS_FENCE;
> 2529 else
> 2530 exec2_list[i].flags = 0;
> 2531 }
> 2532
> 2533 err = i915_gem_do_execbuffer(dev, file, &exec2, exec2_list, NULL);
> 2534 if (exec2.flags & __EXEC_HAS_RELOC) {
> 2535 struct drm_i915_gem_exec_object __user *user_exec_list =
> 2536 u64_to_user_ptr(args->buffers_ptr);
> 2537
> 2538 /* Copy the new buffer offsets back to the user's exec list. */
> 2539 for (i = 0; i < args->buffer_count; i++) {
> 2540 if (!(exec2_list[i].offset & UPDATE))
> 2541 continue;
> 2542
> 2543 exec2_list[i].offset =
> 2544 gen8_canonical_addr(exec2_list[i].offset & PIN_OFFSET_MASK);
> 2545 exec2_list[i].offset &= PIN_OFFSET_MASK;
> 2546 if (__copy_to_user(&user_exec_list[i].offset,
> 2547 &exec2_list[i].offset,
> ^^^^^^^^^^^^^^^^^^^^
> 2548 sizeof(user_exec_list[i].offset)))
> 2549 break;
>
> The story of this warning is that one day Linus was grumpy about
> security issues and said something like, "We should make it a rule that
> code which uses __copy_to_user() should call access_ok() in that exact
> same function or it becomes too hard to audit and error prone. Can
> someone write a static checker for this?" And so I did. But up to now
> I've always just looked at the code and either figured out where the
> access_ok() is or just assumed that "Probably it's there if I looked
> harder".
>
> But today I've drawn a line in the sand! No more!
>
> Also the error code is wrong, we should return -EFAULT if the copy
> fails. ;)
There is an access_ok on that address earlier, and you can't return an
error here as the work is already committed to the GPU. Instead the
returned *hint* is merely ignored, and causes in a fixup pass on
the next call.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] drm/i915: Eliminate lots of iterations over the execobjects array
2018-03-15 13:17 ` Chris Wilson
@ 2018-03-15 13:33 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2018-03-15 13:33 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Thu, Mar 15, 2018 at 01:17:23PM +0000, Chris Wilson wrote:
> Quoting Dan Carpenter (2018-03-15 13:10:30)
> > Hello Chris Wilson,
> >
> > The patch 2889caa92321: "drm/i915: Eliminate lots of iterations over
> > the execobjects array" from Jun 16, 2017, leads to the following
> > static checker warning:
> >
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c:2546 i915_gem_execbuffer_ioctl()
> > warn: calling '__copy_to_user()' without access_ok()
> >
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > 2510 err = copy_from_user(exec_list,
> > 2511 u64_to_user_ptr(args->buffers_ptr),
> > 2512 sizeof(*exec_list) * count);
> > 2513 if (err) {
> > 2514 DRM_DEBUG("copy %d exec entries failed %d\n",
> > 2515 args->buffer_count, err);
> > 2516 kvfree(exec_list);
> > 2517 kvfree(exec2_list);
> > 2518 return -EFAULT;
> > 2519 }
> > 2520
> > 2521 for (i = 0; i < args->buffer_count; i++) {
> > 2522 exec2_list[i].handle = exec_list[i].handle;
> > 2523 exec2_list[i].relocation_count = exec_list[i].relocation_count;
> > 2524 exec2_list[i].relocs_ptr = exec_list[i].relocs_ptr;
> > 2525 exec2_list[i].alignment = exec_list[i].alignment;
> > 2526 exec2_list[i].offset = exec_list[i].offset;
> > 2527 if (INTEL_GEN(to_i915(dev)) < 4)
> > 2528 exec2_list[i].flags = EXEC_OBJECT_NEEDS_FENCE;
> > 2529 else
> > 2530 exec2_list[i].flags = 0;
> > 2531 }
> > 2532
> > 2533 err = i915_gem_do_execbuffer(dev, file, &exec2, exec2_list, NULL);
> > 2534 if (exec2.flags & __EXEC_HAS_RELOC) {
> > 2535 struct drm_i915_gem_exec_object __user *user_exec_list =
> > 2536 u64_to_user_ptr(args->buffers_ptr);
> > 2537
> > 2538 /* Copy the new buffer offsets back to the user's exec list. */
> > 2539 for (i = 0; i < args->buffer_count; i++) {
> > 2540 if (!(exec2_list[i].offset & UPDATE))
> > 2541 continue;
> > 2542
> > 2543 exec2_list[i].offset =
> > 2544 gen8_canonical_addr(exec2_list[i].offset & PIN_OFFSET_MASK);
> > 2545 exec2_list[i].offset &= PIN_OFFSET_MASK;
> > 2546 if (__copy_to_user(&user_exec_list[i].offset,
> > 2547 &exec2_list[i].offset,
> > ^^^^^^^^^^^^^^^^^^^^
> > 2548 sizeof(user_exec_list[i].offset)))
> > 2549 break;
> >
> > The story of this warning is that one day Linus was grumpy about
> > security issues and said something like, "We should make it a rule that
> > code which uses __copy_to_user() should call access_ok() in that exact
> > same function or it becomes too hard to audit and error prone. Can
> > someone write a static checker for this?" And so I did. But up to now
> > I've always just looked at the code and either figured out where the
> > access_ok() is or just assumed that "Probably it's there if I looked
> > harder".
> >
> > But today I've drawn a line in the sand! No more!
> >
> > Also the error code is wrong, we should return -EFAULT if the copy
> > fails. ;)
>
> There is an access_ok on that address earlier
Which function?
regards,
dan carpenter
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-03-15 13:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-15 13:10 [bug report] drm/i915: Eliminate lots of iterations over the execobjects array Dan Carpenter
2018-03-15 13:17 ` Chris Wilson
2018-03-15 13:33 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.