drm/i915: Use copy_from_user() in fence copying

PATCH In review Kees Cook 3 participants 5 messages Dec 6, 2017 – Dec 11, 2017

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Kees Cook

author

2017-12-06 20:28 UTC #0 lore raw

PATCH 1/1 drm/i915: Use copy_from_user() in fence copying

There's no good reason to separate the access_ok() from the copy, especially since the access_ok() size is hard-coded instead of using sizeof(). Instead, just use copy_from_user() directly.

Fixes cf6e7bac6357 ("drm/i915: Add support for drm syncobjs")

Cc Jason Ekstrand <[email protected]>

Cc Chris Wilson <[email protected]>

Signed-off-by Kees Cook <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/gpu/drm/i915/i915_gem_execbuffer.c +1 -3

index 435ed95df144..1da703213b17 100644
@@ -2087,8 +2087,6 @@ get_fence_array(struct drm_i915_gem_execbuffer2 *args,
 		return ERR_PTR(-EINVAL);
 
 	user = u64_to_user_ptr(args->cliprects_ptr);
-	if (!access_ok(VERIFY_READ, user, nfences * 2 * sizeof(u32)))
-		return ERR_PTR(-EFAULT);
 
 	fences = kvmalloc_array(args->num_cliprects, sizeof(*fences),
 				__GFP_NOWARN | GFP_KERNEL);

@@ -2099,7 +2097,7 @@ get_fence_array(struct drm_i915_gem_execbuffer2 *args,
 		struct drm_i915_gem_exec_fence fence;
 		struct drm_syncobj *syncobj;
 
-		if (__copy_from_user(&fence, user++, sizeof(fence))) {
+		if (copy_from_user(&fence, user++, sizeof(fence))) {
 			err = -EFAULT;
 			goto err;
 		}

Signature · 10 lines

2.7.4


-- 
Kees Cook
Pixel Security
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Kees Cook

author

2017-12-08 21:10 UTC #2 lore raw

Quoted 9 lines · David Laight

From: Kees Cook
Quoted 5 lines
Sent: 06 December 2017 20:29
There's no good reason to separate the access_ok() from the copy, especially since the access_ok() size is hard-coded instead of using sizeof(). Instead, just use copy_from_user() directly.
Looks like an optimisation to save doing the access_ok() check for every 'fence'.

If it really makes a difference, okay, but access_ok() checks are fast. :P

Quoted 4 lines

OTOH 'user copy hardening' probably makes a much larger performance degradation on this kind of code.
(Might be ok here because &fence probably refers to something in the current stack frame.)

Well, the good news there is that it's using sizeof(fence), so no hardening check is done (it's not a size that changes at runtime). What I didn't like is that the access_ok() doesn't use sizeof(fence) (it is currently correct: 2 u32s == sizeof(fence)) but that kind of fragility keeps me up at night. ;)

So, fixing either would be fine, but if we're going to touch it, it seems best to just do away with the __copy_*() usage instead.

-Kees

Quoted 40 lines

David

Quoted 37 lines

Fixes cf6e7bac6357 ("drm/i915: Add support for drm syncobjs")

Cc Jason Ekstrand <[email protected]>

Cc Chris Wilson <[email protected]>

Signed-off-by Kees Cook <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/gpu/drm/i915/i915_gem_execbuffer.c +1 -4

index 435ed95df144..1da703213b17 100644
@@ -2087,8 +2087,6 @@ get_fence_array(struct drm_i915_gem_execbuffer2 *args,
              return ERR_PTR(-EINVAL);
 
      user = u64_to_user_ptr(args->cliprects_ptr);
-     if (!access_ok(VERIFY_READ, user, nfences * 2 * sizeof(u32)))
-             return ERR_PTR(-EFAULT);
 
      fences = kvmalloc_array(args->num_cliprects, sizeof(*fences),
                              __GFP_NOWARN | GFP_KERNEL);

@@ -2099,7 +2097,7 @@ get_fence_array(struct drm_i915_gem_execbuffer2 *args,
              struct drm_i915_gem_exec_fence fence;
              struct drm_syncobj *syncobj;
 
-             if (__copy_from_user(&fence, user++, sizeof(fence))) {
+             if (copy_from_user(&fence, user++, sizeof(fence))) {
                      err = -EFAULT;
                      goto err;
              }
--

2.7.4

--
Kees Cook
Pixel Security

Signature · 6 lines

Kees Cook
Pixel Security
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

David Laight

2017-12-11 09:41 UTC #3 lore raw

From: Kees Cook

Quoted 9 lines

Sent: 08 December 2017 21:10
Quoted 6 lines
Quoted 3 lines
There's no good reason to separate the access_ok() from the copy, especially since the access_ok() size is hard-coded instead of using sizeof(). Instead, just use copy_from_user() directly.
Looks like an optimisation to save doing the access_ok() check for every 'fence'.
If it really makes a difference, okay, but access_ok() checks are fast. :P

Not compared to get_user() :-)

David

Joonas Lahtinen

maintainer

2017-12-11 09:34 UTC #4 lore raw

In reply to Kees Cook

Quoted 5 lines · Kees Cook

There's no good reason to separate the access_ok() from the copy, especially since the access_ok() size is hard-coded instead of using sizeof(). Instead, just use copy_from_user() directly.
Fixes cf6e7bac6357 ("drm/i915: Add support for drm syncobjs")

There's been request to reduce the amount of Fixes: tags that are not actually fixing bugs. This seems more like an optimization.

References: has been suggested for these cases instead.

Regards, Joonas

Signature · 7 lines

Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx