linux-kernel - Re: [PATCH] x86/smap: Fix the smap

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] [day] [month] [year] [list]

Message-ID: <20200916072805.GH2674@hirez.programming.kicks-ass.net>
Date:   Wed, 16 Sep 2020 09:28:05 +0200
From:   peterz@...radead.org
To:     Andy Lutomirski <luto@...nel.org>
Cc:     x86@...nel.org, LKML <linux-kernel@...r.kernel.org>,
        Bill Wendling <morbo@...gle.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Greg Thelen <gthelen@...gle.com>,
        John Sperbeck <jsperbeck@...gle.com>, stable@...r.kernel.org
Subject: Re: [PATCH] x86/smap: Fix the smap_save() asm

On Tue, Sep 15, 2020 at 01:55:58PM -0700, Andy Lutomirski wrote:
> The old smap_save() code was:
> 
>   pushf
>   pop %0
> 
> with %0 defined by an "=rm" constraint.  This is fine if the
> compiler picked the register option, but it was incorrect with an
> %rsp-relative memory operand.  With some intentional abuse, I can
> get both gcc and clang to generate code along these lines:
> 
>   pushfq
>   popq 0x8(%rsp)
>   mov 0x8(%rsp),%rax
> 
> which is incorrect and will not work as intended.

We need another constraint :-)

> Fix it by removing the memory option.  This issue is exacerbated by
> a clang optimization bug:
> 
>   https://bugs.llvm.org/show_bug.cgi?id=47530
> 
> Fixes: e74deb11931f ("x86/uaccess: Introduce user_access_{save,restore}()")
> Cc: stable@...r.kernel.org
> Reported-by: Bill Wendling <morbo@...gle.com> # I think
> Signed-off-by: Andy Lutomirski <luto@...nel.org>
> ---
>  arch/x86/include/asm/smap.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/smap.h b/arch/x86/include/asm/smap.h
> index 8b58d6975d5d..be6d675ae3ac 100644
> --- a/arch/x86/include/asm/smap.h
> +++ b/arch/x86/include/asm/smap.h
> @@ -61,7 +61,7 @@ static __always_inline unsigned long smap_save(void)
>  		      ALTERNATIVE("jmp 1f", "", X86_FEATURE_SMAP)
>  		      "pushf; pop %0; " __ASM_CLAC "\n\t"
>  		      "1:"
> -		      : "=rm" (flags) : : "memory", "cc");
> +		      : "=r" (flags) : : "memory", "cc");

native_save_fl() has the exact same code; you'll need to fix both.