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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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)
        ^

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.

Third, there is a huge quantity of complexity for a form of the
instruction we don't use.  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.

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.

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.

~Andrew