diff mbox series

[v2,5/6] x86: guard against straight-line speculation past RET

Message ID fd18939c-cfc7-6de8-07f2-217f810afde1@suse.com (mailing list archive)
State Superseded
Headers show
Series x86: some assembler macro rework | expand

Commit Message

Jan Beulich Sept. 28, 2020, 12:31 p.m. UTC
Under certain conditions CPUs can speculate into the instruction stream
past a RET instruction. Guard against this just like 3b7dab93f240
("x86/spec-ctrl: Protect against CALL/JMP straight-line speculation")
did - by inserting an "INT $3" insn. It's merely the mechanics of how to
achieve this that differ: A set of macros gets introduced to post-
process RET insns issued by the compiler (or living in assembly files).

Unfortunately for clang this requires further features their built-in
assembler doesn't support: We need to be able to override insn mnemonics
produced by the compiler (which may be impossible, if internally
assembly mnemonics never get generated), and we want to use \(text)
escaping / quoting in the auxiliary macro.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: Should this depend on CONFIG_SPECULATIVE_HARDEN_BRANCH?
TBD: Would be nice to avoid the additions in .init.text, but a query to
     the binutils folks regarding the ability to identify the section
     stuff is in (by Peter Zijlstra over a year ago:
     https://sourceware.org/pipermail/binutils/2019-July/107528.html)
     has been left without helpful replies.
---
v2: Fix build with newer clang. Use int3 mnemonic. Also override retq.

Comments

Roger Pau Monné Oct. 8, 2020, 4:28 p.m. UTC | #1
On Mon, Sep 28, 2020 at 02:31:49PM +0200, Jan Beulich wrote:
> Under certain conditions CPUs can speculate into the instruction stream
> past a RET instruction. Guard against this just like 3b7dab93f240
> ("x86/spec-ctrl: Protect against CALL/JMP straight-line speculation")
> did - by inserting an "INT $3" insn. It's merely the mechanics of how to
> achieve this that differ: A set of macros gets introduced to post-
> process RET insns issued by the compiler (or living in assembly files).
> 
> Unfortunately for clang this requires further features their built-in
> assembler doesn't support: We need to be able to override insn mnemonics
> produced by the compiler (which may be impossible, if internally
> assembly mnemonics never get generated), and we want to use \(text)
> escaping / quoting in the auxiliary macro.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Code LGTM.

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

See below for the TBD.

> ---
> TBD: Should this depend on CONFIG_SPECULATIVE_HARDEN_BRANCH?

I don't see the additions done in 3b7dab93f240 being guarded by
CONFIG_SPECULATIVE_HARDEN_BRANCH, so in that regard I would say no.
However those are already guarded by CONFIG_INDIRECT_THUNK so it's
slightly weird that the addition of such protections cannot be turned
off in any way.

I would be fine with having the additions done in 3b7dab93f240
protected by CONFIG_SPECULATIVE_HARDEN_BRANCH, and then the additions
done here also.

Thanks, Roger.
Jan Beulich Oct. 13, 2020, 10:33 a.m. UTC | #2
On 08.10.2020 18:28, Roger Pau Monné wrote:
> On Mon, Sep 28, 2020 at 02:31:49PM +0200, Jan Beulich wrote:
>> Under certain conditions CPUs can speculate into the instruction stream
>> past a RET instruction. Guard against this just like 3b7dab93f240
>> ("x86/spec-ctrl: Protect against CALL/JMP straight-line speculation")
>> did - by inserting an "INT $3" insn. It's merely the mechanics of how to
>> achieve this that differ: A set of macros gets introduced to post-
>> process RET insns issued by the compiler (or living in assembly files).
>>
>> Unfortunately for clang this requires further features their built-in
>> assembler doesn't support: We need to be able to override insn mnemonics
>> produced by the compiler (which may be impossible, if internally
>> assembly mnemonics never get generated), and we want to use \(text)
>> escaping / quoting in the auxiliary macro.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Code LGTM.
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ---
>> TBD: Should this depend on CONFIG_SPECULATIVE_HARDEN_BRANCH?
> 
> I don't see the additions done in 3b7dab93f240 being guarded by
> CONFIG_SPECULATIVE_HARDEN_BRANCH, so in that regard I would say no.
> However those are already guarded by CONFIG_INDIRECT_THUNK so it's
> slightly weird that the addition of such protections cannot be turned
> off in any way.
> 
> I would be fine with having the additions done in 3b7dab93f240
> protected by CONFIG_SPECULATIVE_HARDEN_BRANCH, and then the additions
> done here also.

Okay, perhaps I'll make a separate patch then to add the conditional
at all respective places.

Jan
Andrew Cooper Oct. 13, 2020, noon UTC | #3
On 28/09/2020 13:31, Jan Beulich wrote:
> Under certain conditions CPUs can speculate into the instruction stream
> past a RET instruction. Guard against this just like 3b7dab93f240
> ("x86/spec-ctrl: Protect against CALL/JMP straight-line speculation")
> did - by inserting an "INT $3" insn. It's merely the mechanics of how to
> achieve this that differ: A set of macros gets introduced to post-
> process RET insns issued by the compiler (or living in assembly files).
>
> Unfortunately for clang this requires further features their built-in
> assembler doesn't support: We need to be able to override insn mnemonics
> produced by the compiler (which may be impossible, if internally
> assembly mnemonics never get generated), and we want to use \(text)
> escaping / quoting in the auxiliary macro.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TBD: Should this depend on CONFIG_SPECULATIVE_HARDEN_BRANCH?
> TBD: Would be nice to avoid the additions in .init.text, but a query to
>      the binutils folks regarding the ability to identify the section
>      stuff is in (by Peter Zijlstra over a year ago:
>      https://sourceware.org/pipermail/binutils/2019-July/107528.html)
>      has been left without helpful replies.
> ---
> v2: Fix build with newer clang. Use int3 mnemonic. Also override retq.
>
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -145,7 +145,15 @@ t2 = $(call as-insn,$(CC) -I$(BASEDIR)/i
>  # https://bugs.llvm.org/show_bug.cgi?id=36110
>  t3 = $(call as-insn,$(CC),".macro FOO;.endm"$(close); asm volatile $(open)".macro FOO;.endm",-no-integrated-as)
>  
> -CLANG_FLAGS += $(call or,$(t1),$(t2),$(t3))
> +# Check whether \(text) escaping in macro bodies is supported.
> +t4 = $(call as-insn,$(CC),".macro m ret:req; \\(ret) $$\\ret; .endm; m 8",,-no-integrated-as)
> +
> +# Check whether macros can override insn mnemonics in inline assembly.
> +t5 = $(call as-insn,$(CC),".macro ret; .error; .endm; .macro retq; .error; .endm",-no-integrated-as)
> +
> +acc1 := $(call or,$(t1),$(t2),$(t3),$(t4))
> +
> +CLANG_FLAGS += $(call or,$(acc1),$(t5))

I'm not happy taking this until there is toolchain support visible in
the future.

We *cannot* rule out the use of IAS forever more, because there are
features far more important than ret speculation which depend on it.

>  endif
>  
>  CLANG_FLAGS += -Werror=unknown-warning-option
> --- a/xen/include/asm-x86/asm-defns.h
> +++ b/xen/include/asm-x86/asm-defns.h
> @@ -50,3 +50,22 @@
>  .macro INDIRECT_JMP arg:req
>      INDIRECT_BRANCH jmp \arg
>  .endm
> +
> +/*
> + * To guard against speculation past RET, insert a breakpoint insn
> + * immediately after them.
> + */
> +.macro ret operand:vararg
> +    ret$ \operand
> +.endm
> +.macro retq operand:vararg
> +    ret$ \operand
> +.endm
> +.macro ret$ operand:vararg
> +    .purgem ret
> +    ret \operand

You're substituting retq for ret, which defeats the purpose of unwrapping.

I will repeat my previous feedback.  Do away with this
wrapping/unwrapping and just use a raw .byte.  Its simpler and faster.

~Andrew
Jan Beulich Oct. 13, 2020, 1:34 p.m. UTC | #4
On 13.10.2020 14:00, Andrew Cooper wrote:
> On 28/09/2020 13:31, Jan Beulich wrote:
>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -145,7 +145,15 @@ t2 = $(call as-insn,$(CC) -I$(BASEDIR)/i
>>  # https://bugs.llvm.org/show_bug.cgi?id=36110
>>  t3 = $(call as-insn,$(CC),".macro FOO;.endm"$(close); asm volatile $(open)".macro FOO;.endm",-no-integrated-as)
>>  
>> -CLANG_FLAGS += $(call or,$(t1),$(t2),$(t3))
>> +# Check whether \(text) escaping in macro bodies is supported.
>> +t4 = $(call as-insn,$(CC),".macro m ret:req; \\(ret) $$\\ret; .endm; m 8",,-no-integrated-as)
>> +
>> +# Check whether macros can override insn mnemonics in inline assembly.
>> +t5 = $(call as-insn,$(CC),".macro ret; .error; .endm; .macro retq; .error; .endm",-no-integrated-as)
>> +
>> +acc1 := $(call or,$(t1),$(t2),$(t3),$(t4))
>> +
>> +CLANG_FLAGS += $(call or,$(acc1),$(t5))
> 
> I'm not happy taking this until there is toolchain support visible in
> the future.
> 
> We *cannot* rule out the use of IAS forever more, because there are
> features far more important than ret speculation which depend on it.

So what do you suggest? We can't have both, afaics, so we need to
pick either being able to use the integrated assembler or being
able to guard RET.

>> --- a/xen/include/asm-x86/asm-defns.h
>> +++ b/xen/include/asm-x86/asm-defns.h
>> @@ -50,3 +50,22 @@
>>  .macro INDIRECT_JMP arg:req
>>      INDIRECT_BRANCH jmp \arg
>>  .endm
>> +
>> +/*
>> + * To guard against speculation past RET, insert a breakpoint insn
>> + * immediately after them.
>> + */
>> +.macro ret operand:vararg
>> +    ret$ \operand
>> +.endm
>> +.macro retq operand:vararg
>> +    ret$ \operand
>> +.endm
>> +.macro ret$ operand:vararg
>> +    .purgem ret
>> +    ret \operand
> 
> You're substituting retq for ret, which defeats the purpose of unwrapping.

I'm afraid I don't understand the "defeats" aspect.

> I will repeat my previous feedback.  Do away with this
> wrapping/unwrapping and just use a raw .byte.  Its simpler and faster.

Well, I could now also repeat my prior response to your prior
feedback, but since iirc you didn't reply back then I don't
expect you would now. Instead I'll - once again - give in despite
my believe that this is the cleaner approach, and that in cases
like this one - when there are pros and cons to either approach -
it should be the author's choice rather than the reviewer's.

Jan
diff mbox series

Patch

--- a/xen/Makefile
+++ b/xen/Makefile
@@ -145,7 +145,15 @@  t2 = $(call as-insn,$(CC) -I$(BASEDIR)/i
 # https://bugs.llvm.org/show_bug.cgi?id=36110
 t3 = $(call as-insn,$(CC),".macro FOO;.endm"$(close); asm volatile $(open)".macro FOO;.endm",-no-integrated-as)
 
-CLANG_FLAGS += $(call or,$(t1),$(t2),$(t3))
+# Check whether \(text) escaping in macro bodies is supported.
+t4 = $(call as-insn,$(CC),".macro m ret:req; \\(ret) $$\\ret; .endm; m 8",,-no-integrated-as)
+
+# Check whether macros can override insn mnemonics in inline assembly.
+t5 = $(call as-insn,$(CC),".macro ret; .error; .endm; .macro retq; .error; .endm",-no-integrated-as)
+
+acc1 := $(call or,$(t1),$(t2),$(t3),$(t4))
+
+CLANG_FLAGS += $(call or,$(acc1),$(t5))
 endif
 
 CLANG_FLAGS += -Werror=unknown-warning-option
--- a/xen/include/asm-x86/asm-defns.h
+++ b/xen/include/asm-x86/asm-defns.h
@@ -50,3 +50,22 @@ 
 .macro INDIRECT_JMP arg:req
     INDIRECT_BRANCH jmp \arg
 .endm
+
+/*
+ * To guard against speculation past RET, insert a breakpoint insn
+ * immediately after them.
+ */
+.macro ret operand:vararg
+    ret$ \operand
+.endm
+.macro retq operand:vararg
+    ret$ \operand
+.endm
+.macro ret$ operand:vararg
+    .purgem ret
+    ret \operand
+    int3
+    .macro ret operand:vararg
+        ret$ \\(operand)
+    .endm
+.endm