linux-kernel - Re: [PATCH] drm/i915: Minimize uaccess exposure in i915_gem_execbuffer2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <158284236096.19174.6917853940060252533@skylake-alporthouse-com>
Date:   Thu, 27 Feb 2020 22:26:00 +0000
From:   Chris Wilson <chris@...is-wilson.co.uk>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     linux-kernel@...r.kernel.org, Randy Dunlap <rdunlap@...radead.org>,
        Peter Zijlstra <peterz@...radead.org>,
        intel-gfx@...ts.freedesktop.org
Subject: Re: [PATCH] drm/i915: Minimize uaccess exposure in
 i915_gem_execbuffer2_ioctl()

Quoting Josh Poimboeuf (2020-02-27 22:08:26)
> With CONFIG_CC_OPTIMIZE_FOR_SIZE, objtool reports:
> 
>   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: i915_gem_execbuffer2_ioctl()+0x5b7: call to gen8_canonical_addr() with UACCESS enabled
> 
> This means i915_gem_execbuffer2_ioctl() is calling gen8_canonical_addr()
> -- and indirectly, sign_extend64() -- from the user_access_begin/end
> critical region (i.e, with SMAP disabled).
> 
> While it's probably harmless in this case, in general we like to avoid
> extra function calls in SMAP-disabled regions because it can open up
> inadvertent security holes.
> 
> Fix it by moving the gen8_canonical_addr() conversion to a separate loop
> before user_access_begin() is called.
> 
> Note that gen8_canonical_addr() is now called *before* masking off the
> PIN_OFFSET_MASK bits.  That should be ok because it just does a sign
> extension and ignores the masked lower bits anyway.
> 
> Reported-by: Randy Dunlap <rdunlap@...radead.org>
> Signed-off-by: Josh Poimboeuf <jpoimboe@...hat.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index d5a0f5ae4a8b..183cab13e028 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -2947,6 +2947,13 @@ i915_gem_execbuffer2_ioctl(struct drm_device *dev, void *data,
>                         u64_to_user_ptr(args->buffers_ptr);
>                 unsigned int i;
>  
> +               /*
> +                * Do the call to gen8_canonical_addr() outside the
> +                * uaccess-enabled region to minimize uaccess exposure.
> +                */
> +               for (i = 0; i < args->buffer_count; i++)
> +                       exec2_list[i].offset = gen8_canonical_addr(exec2_list[i].offset);


Another loop over all the objects, where we intentionally try and skip
unmodified entries? To save 2 instructions from inside the second loop?

Colour me skeptical.
-Chris