linux-kernel - Re: [PATCH v3] x86/uaccess: Use pointer masking to limit uaccess speculation

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]

Date:   Mon, 14 Sep 2020 14:50:59 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     X86 ML <x86@...nel.org>, Al Viro <viro@...iv.linux.org.uk>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Will Deacon <will@...nel.org>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Waiman Long <longman@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andrew Cooper <andrew.cooper3@...rix.com>,
        Andy Lutomirski <luto@...nel.org>,
        Christoph Hellwig <hch@....de>,
        David Laight <David.Laight@...lab.com>,
        Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH v3] x86/uaccess: Use pointer masking to limit uaccess
 speculation

On Mon, Sep 14, 2020 at 12:06:56PM -0700, Dan Williams wrote:
> > +++ b/arch/x86/include/asm/checksum_32.h
> > @@ -49,7 +49,8 @@ static inline __wsum csum_and_copy_from_user(const void __user *src,
> >         might_sleep();
> >         if (!user_access_begin(src, len))
> >                 return 0;
> > -       ret = csum_partial_copy_generic((__force void *)src, dst, len);
> > +       ret = csum_partial_copy_generic((__force void *)force_user_ptr(src),
> > +                                       dst, len);
> 
> I look at this and wonder if the open-coded "(__force void *)" should
> be subsumed in the new macro.

I'm not sure how we could do that?  This is kind of a special case
anyway.  Most users of the macro don't need to cast the return value
because the macro already casts it to the original type.

> It also feels like the name should be "enforce" to distinguish it from
> the type cast case?

Yeah, although, to me, "enforce" has other connotations which make it
less clear.  (I can't quite put my finger on why, but "enforce" sounds
like it doesn't return a value.)  I wouldn't be opposed to changing it
if others agree.

> > --- a/arch/x86/include/asm/uaccess.h
> > +++ b/arch/x86/include/asm/uaccess.h
> > @@ -6,6 +6,7 @@
> >   */
> >  #include <linux/compiler.h>
> >  #include <linux/kasan-checks.h>
> > +#include <linux/nospec.h>
> >  #include <linux/string.h>
> >  #include <asm/asm.h>
> >  #include <asm/page.h>
> > @@ -66,12 +67,23 @@ static inline bool pagefault_disabled(void);
> >   * Return: true (nonzero) if the memory block may be valid, false (zero)
> >   * if it is definitely invalid.
> >   */
> > -#define access_ok(addr, size)                                  \
> 
> unnecessary whitespace change?

True, though it's not uncommon to fix whitespace close to where you're
changing something.  Better than spinning a whitespace-only patch IMO.

> Other than that and the optional s/force/enforce/ rename + cast
> collapse you can add:
> 
> Reviewed-by: Dan Williams <dan.j.williams@...el.com>

Thanks for the review!

-- 
Josh