Re: [PATCH 5/9] x86/alternative: Handle Jcc __x86_indirect_thunk_\reg

[Nick Desaulniers <ndesaulniers@google.com>]

On Wed, Oct 13, 2021 at 5:41 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> Handle the rare cases where the compiler (clang) does an indirect
> conditional tail-call using:
>
>   Jcc __x86_indirect_thunk_\reg

`Jcc.d32 __x86_indirect_thunk_\reg` might be clearer; otherwise
putting that in an assembler and assembling/disassembling produces the
2B instructions, which makes the below patch confusing. Ah, it is
stated in the comment added below.

>
> For the !RETPOLINE case this can be rewritten to fit the original (6
> byte) instruction like:
>
>   Jncc.d8       1f
>   JMP           *%\reg
>   NOP
> 1:
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/kernel/alternative.c |   38 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 34 insertions(+), 4 deletions(-)
>
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -378,7 +378,8 @@ static int emit_indirect(int op, int reg
>  static int patch_retpoline(void *addr, struct insn *insn, u8 *bytes)
>  {
>         void (*target)(void);
> -       int reg, i = 0;
> +       int reg, ret, i = 0;
> +       u8 op, cc;
>
>         if (cpu_feature_enabled(X86_FEATURE_RETPOLINE))
>                 return -1;
> @@ -390,9 +391,34 @@ static int patch_retpoline(void *addr, s
>         if (WARN_ON_ONCE(reg & ~0xf))
>                 return -1;
>
> -       i = emit_indirect(insn->opcode.bytes[0], reg, bytes);
> -       if (i < 0)
> -               return i;
> +       op = insn->opcode.bytes[0];
> +
> +       /*
> +        * Convert:
> +        *
> +        *   Jcc.d32 __x86_indirect_thunk_\reg
> +        *
> +        * into:
> +        *
> +        *   Jncc.d8 1f
> +        *   jmp *%\reg
> +        *   nop
> +        * 1:
> +        */
> +       if (op == 0x0f && (insn->opcode.bytes[1] & 0xf0) == 0x80) {
> +               cc = insn->opcode.bytes[1] & 0xf;
> +               cc ^= 1; /* invert condition */
> +
> +               bytes[i++] = 0x70 + cc; /* Jcc.d8 */
> +               bytes[i++] = insn->length - 2;

Isn't `insn->length - 2` always 4 (in this case)? We could avoid
computing that at runtime I suspect if we just hardcoded it.

Either way, I've looked at the disassembly enough that this LGTM.
Thanks for the patch.

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> +
> +               op = JMP32_INSN_OPCODE;
> +       }
> +
> +       ret = emit_indirect(op, reg, bytes + i);
> +       if (ret < 0)
> +               return ret;
> +       i += ret;
>
>         for (; i < insn->length;)
>                 bytes[i++] = BYTES_NOP1;
> @@ -423,6 +449,10 @@ void __init_or_module noinline apply_ret
>                 case JMP32_INSN_OPCODE:
>                         break;
>
> +               case 0x0f: /* escape */
> +                       if (op2 >= 0x80 && op2 <= 0x8f)
> +                               break;
> +                       fallthrough;
>                 default:
>                         WARN_ON_ONCE(1);
>                         continue;
>
>


-- 
Thanks,
~Nick Desaulniers