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

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@vger.kernel.org Reported-by: Bill Wendling morbo@google.com # 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@kernel.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");
  
    return flags;
  

}

2.26.2