Re: [PATCH] x86/smap: Fix the smap_save() asm

[Nick Desaulniers]

On Tue, Sep 15, 2020 at 1:56 PM Andy Lutomirski <luto@xxxxxxxxxx> 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.

It is incorrect because ... (I think mentioning the point about the
red zone would be good, unless there were additional concerns?)

The patch should be fine, so

Reviewed-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>

regardless of whether or not you choose to amend the commit message.
I suspect that the use of "r" exclusively without "m" could lead to
register exhaustion (though it's more likely the compiler will spill
some other variable to stack), which is why it's common to use it in
conjunction with "m."  As Bill's patch notes, getting the "m" version
is poor for performance of this pattern, or at least for
native_{save|restore}_fl.  Being able to use the compiler builtins is
another possibility here.

> 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.
>
> 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

This is something we should fix.  Bill, James, and I are discussing
this internally.  Thank you for filing a bug; I owe you a beer just
for that.

>
> Fixes: e74deb11931f ("x86/uaccess: Introduce user_access_{save,restore}()")
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: Bill Wendling <morbo@xxxxxxxxxx> # I think

LOL, yes, the comment can be dropped...though I guess someone else may
have reported the problem to Bill?

> Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxx>
> ---
>  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");
>
>         return flags;
>  }
> --
> 2.26.2
>


-- 
Thanks,
~Nick Desaulniers