linux-kernel - Re: [PATCH] x86: fix user address masking non-canonical speculation issue

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: <20241024110822.GBZxoqppmxp0xxG7ew@fat_crate.local>
Date: Thu, 24 Oct 2024 13:08:22 +0200
From: Borislav Petkov <bp@...en8.de>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	x86@...nel.org, Andrew Cooper <andrew.cooper3@...rix.com>,
	Josh Poimboeuf <jpoimboe@...nel.org>
Subject: Re: [PATCH] x86: fix user address masking non-canonical speculation
 issue

On Wed, Oct 23, 2024 at 06:31:59PM -0700, Linus Torvalds wrote:
> +static inline void __user *mask_user_address(const void __user *ptr)
> +{
> +	void __user *ret;
> +	asm("cmp %1,%0; sbb %0,%0; or %1,%0"
> +		:"=r" (ret)
> +		:"r" (ptr),
> +		 "0" (runtime_const_ptr(USER_PTR_MAX)));
> +	return ret;
> +}

Can we make this more readable pls?

Something like this:

static inline void __user *mask_user_address(const void __user *ptr)
{       
        void __user *ret;

        asm("cmp %[ptr],%[ret]\n\t"
            "sbb %[ret],%[ret]\n\t"
            "or  %[ptr],%[ret]"
                : [ret] "=r" (ret)
                : [ptr] "r" (ptr),
                  "0" (runtime_const_ptr(USER_PTR_MAX)));

        return ret;
}

This way the compiler-generated asm is more readable too due to the newlines
and tabs.

In any case, it looks good, I single-stepped strnlen_user() and I got:

# move the constant
movabs $0x7ffffffff000,%rdi

# the user pointer: rax = 0x7ffcb6839fdf
cmp    %rax,%rdi

sbb    %rdi,%rdi
# rdi = 0x0

or     %rax,%rdi
# rdi = 0x7ffcb6839fdf

stac

and user pointer is in %rdi.

Then, on the next breakpoint, I modified the user pointer:

gdb> set $rax = 0xfffcb6839fd9
cmp    %rax,%rdi

sbb    %rdi,%rdi
# rdi = 0xffffffffffffffff  -1

# flags are set
eflags         0x297               [ IOPL=0 IF SF AF PF CF ]

or     %rax,%rdi

# user pointer is -1, do_strnlen_user() will try to work with -1 and fault...

> @@ -2389,6 +2390,15 @@ void __init arch_cpu_finalize_init(void)
>  	alternative_instructions();
>  
>  	if (IS_ENABLED(CONFIG_X86_64)) {
> +		unsigned long USER_PTR_MAX = TASK_SIZE_MAX;
> +
> +		/*
> +		 * Enable this when LAM is gated on LASS support
> +		if (cpu_feature_enabled(X86_FEATURE_LAM))
> +			USER_PTR_MAX = (1ul << 63) - PAGE_SIZE;
> +		 */
> +		runtime_const_init(ptr, USER_PTR_MAX);

Looking at Documentation/arch/x86/x86_64/mm.rst, 5 level page tables define
USR_PTR_MAX as 0x00ffffffffffffff, i.e., bits [55:0].

So I guess that USER_PTR_MAX needs to look at X86_FEATURE_LA57, no?

Thx.

-- 
Regards/Gruss,
    Boris.

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