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

[Borislav Petkov]

On Thu, Sep 10, 2020 at 12:22:53PM -0500, Josh Poimboeuf wrote:
> The x86 uaccess code uses barrier_nospec() in various places to prevent
> speculative dereferencing of user-controlled pointers (which might be
> combined with further gadgets or CPU bugs to leak data).
> 
> There are some issues with the current implementation:
> 
> - The barrier_nospec() in copy_from_user() was inadvertently removed
>   with: 4b842e4e25b1 ("x86: get rid of small constant size cases in
>   raw_copy_{to,from}_user()")
> 
> - copy_to_user() and friends should also have a speculation barrier,
>   because a speculative write to a user-controlled address can still
>   populate the cache line with the original data.
> 
> - The LFENCE in barrier_nospec() is overkill, when more lightweight user
>   pointer masking can be used instead.
> 
> Remove all existing barrier_nospec() usage, and instead do user pointer
> masking, throughout the x86 uaccess code.  This is similar to what arm64
> is already doing with uaccess_mask_ptr().
> 
> barrier_nospec() is now unused, and can be removed.
> 
> Fixes: 4b842e4e25b1 ("x86: get rid of small constant size cases in raw_copy_{to,from}_user()")
> Suggested-by: Will Deacon <will@xxxxxxxxxx>
> Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> ---
> v3:
> 
> - Rebased on vfs#for-next, using TASK_SIZE_MAX now that set_fs() is
>   gone.  I considered just clearing the most significant bit, but that
>   only works for 64-bit, so in the interest of common code I went with
>   the more straightforward enforcement of the TASK_SIZE_MAX limit.
> 
> - Rename the macro to force_user_ptr(), which is more descriptive, and
>   also more distinguishable from a planned future macro for sanitizing
>   __user pointers on syscall entry.
> 
>  Documentation/admin-guide/hw-vuln/spectre.rst |  6 ++--
>  arch/x86/include/asm/barrier.h                |  3 --
>  arch/x86/include/asm/checksum_32.h            |  6 ++--
>  arch/x86/include/asm/futex.h                  |  5 +++
>  arch/x86/include/asm/uaccess.h                | 35 ++++++++++++-------
>  arch/x86/include/asm/uaccess_64.h             | 16 ++++-----
>  arch/x86/lib/csum-wrappers_64.c               |  5 +--
>  arch/x86/lib/getuser.S                        | 10 +++---
>  arch/x86/lib/putuser.S                        |  8 +++++
>  arch/x86/lib/usercopy_32.c                    |  6 ++--
>  arch/x86/lib/usercopy_64.c                    |  7 ++--
>  lib/iov_iter.c                                |  2 +-
>  12 files changed, 65 insertions(+), 44 deletions(-)

After clarifying some stuff on IRC:

Acked-by: Borislav Petkov <bp@xxxxxxx>

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette