From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752730AbdLHVK3 (ORCPT ); Fri, 8 Dec 2017 16:10:29 -0500 Received: from mail-ua0-f195.google.com ([209.85.217.195]:47095 "EHLO mail-ua0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752255AbdLHVKZ (ORCPT ); Fri, 8 Dec 2017 16:10:25 -0500 X-Google-Smtp-Source: AGs4zMYiOYoEjxTNaa8o0ni6se9vNH/Q4D0fKh8C91Db4UbLJg7597hNe7bEH6CfciC2crfUouIeotBF07Hnel+MU6A= MIME-Version: 1.0 In-Reply-To: References: <20171206202850.GA38365@beast> From: Kees Cook Date: Fri, 8 Dec 2017 13:10:23 -0800 X-Google-Sender-Auth: -RbTQd-xR7ugiGqf-lz7h3zJDAw Message-ID: Subject: Re: [PATCH] drm/i915: Use copy_from_user() in fence copying To: David Laight Cc: Jason Ekstrand , Chris Wilson , "intel-gfx@lists.freedesktop.org" , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Dec 8, 2017 at 2:17 AM, David Laight 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 >> Cc: Chris Wilson >> Signed-off-by: Kees Cook >> --- >> 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 -- Kees Cook Pixel Security