| Message ID | 20171206202850.GA38365@beast (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
From: Kees Cook > 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'. 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.) David > Fixes: cf6e7bac6357 ("drm/i915: Add support for drm syncobjs") > Cc: Jason Ekstrand <jason@jlekstrand.net> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 435ed95df144..1da703213b17 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -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
On Fri, Dec 8, 2017 at 2:17 AM, David Laight <David.Laight@aculab.com> wrote: > From: Kees Cook >> 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 > 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 > > David > >> Fixes: cf6e7bac6357 ("drm/i915: Add support for drm syncobjs") >> Cc: Jason Ekstrand <jason@jlekstrand.net> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Signed-off-by: Kees Cook <keescook@chromium.org> >> --- >> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> index 435ed95df144..1da703213b17 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> @@ -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
On Wed, 2017-12-06 at 12:28 -0800, Kees Cook wrote: > 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
From: Kees Cook > Sent: 08 December 2017 21:10 > >> 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
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 435ed95df144..1da703213b17 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -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; }
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 <jason@jlekstrand.net> Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)