[Intel-gfx] [bug report] drm/i915: Eliminate lots of iterations over the execobjects array

[Chris Wilson]

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