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

[Josh Poimboeuf <jpoimboe@redhat.com>]

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@intel.com>

Thanks for the review!

-- 
Josh