Re: [PATCH] x86: guard against straight-line speculation past RET

[Jan Beulich <jbeulich@suse.com>]

On 04.09.2020 20:18, Andrew Cooper wrote:
> On 24/08/2020 13:50, Jan Beulich wrote:
>> --- a/xen/include/asm-x86/asm-defns.h
>> +++ b/xen/include/asm-x86/asm-defns.h
>> @@ -50,3 +50,19 @@
>>  .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 ret$ operand:vararg
>> +    .purgem ret
>> +    ret \operand
>> +    int $3
>> +    .macro ret operand:vararg
>> +        ret$ \\(operand)
>> +    .endm
>> +.endm
> 
> Several observations.  First, clang chokes on this:
> 
> <instantiation>:2:9: error: unknown token in expression
>     ret \\(operand)
>         ^

Must be clang more recent than the 5.x one I've tested with; likely
because there we end up using -mno-integrated-as.

> Second, you mean int3 rather than int $3.  By convention, they are
> synonymous, but the second one really ought to be the two byte encoding,
> rather than the single byte encoding, and we really want the single byte
> version for code volume reasons.

Well, no, I didn't mean "int3", but I've switched nevertheless, just
for consistency with the earlier change of yours referenced in the
description. To me "int3" has only ever been a kludge. Assemblers
I've grown up with don't know such a mnemonic. Nor did Intel
originally document it, and AMD still doesn't.

> Third, there is a huge quantity of complexity for a form of the
> instruction we don't use.

The complexity isn't with handling the possible immediate operand,
but with the need to override the "ret" insn, and then to transiently
cancel this override.

>  Instead:
> 
> .macro ret operand:vararg
>     .ifnb \operand
>         .error "TODO - speculation safety for 'ret $imm'"
>     .endif
> 
>     .byte 0xc3
>     int3
> .endm
> 
> is much simpler, and compatible with both GCC and Clang.

I really wish to avoid .byte for code emission whenever possible.
It subverts the assembler applying sanity checks. This may not be
overly relevant here, but then we would still better avoid setting
precedents. However, if clang can't be made work without going
this route, so be it.

> Almost...
> 
> Clang doesn't actually expand the macro for ret instructions, so a Clang
> build of Xen only ends up getting protected in the assembly files.
> 
> The following experiment demonstrates the issue:
> 
> $ cat ret.c
> asm (".macro ret\n\t"
>      ".error \"foo\"\n\t"
>      ".endm\n\t");
> void foo(void) {}
> 
> $ gcc -O3 -c ret.c -o ret.o && objdump -d ret.o
> /tmp/ccf8hkyN.s: Assembler messages:
> /tmp/ccf8hkyN.s:16: Error: foo
> 
> $ clang-10 -O3 -c ret.c -o ret.o && objdump -d ret.o
> 
> ret.o:     file format elf64-x86-64
> 
> 
> Disassembly of section .text:
> 
> 0000000000000000 <foo>:
>    0:    c3                       retq
> 
> 
> Worse, -no-integrated-as doesn't immediately help, even though it
> invokes $(AS).
> 
> I tracked that down to the difference between ret and retq, which
> highlights an assumption about GCC which may not remain true in the future.
> 
> Adding a second macro covering retq fixes the scenario in combination
> with -no-integrated-as.

Ah, yes, I should of course have thought of retq. Albeit as per
above - generated code looks fine here when using clang 5.

> So overall I think we can make a safe binary with a clang build. 
> However, it is at the expense of the integrated assembler, which I
> believe is now mandatory for LTO, and control-flow integrity, neither of
> which we want to lose in the long term.

Why at this expense? Are you saying that even when going the .byte
route and even with very new clang one has to force
-mno-integrated-as?

Jan