| Message ID | Yxm+QkFPOhrVSH6q@hirez.programming.kicks-ass.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | x86,retpoline: Be sure to emit INT3 after JMP *%\reg | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-16 | success | Logs for test_verifier on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-13 | success | Logs for test_progs_no_alu32 on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-14 | success | Logs for test_progs_no_alu32 on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-17 | success | Logs for test_verifier on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-7 | success | Logs for test_maps on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-10 | success | Logs for test_progs on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-11 | success | Logs for test_progs on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-8 | success | Logs for test_maps on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-9 | success | Logs for test_progs on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-12 | success | Logs for test_progs_no_alu32 on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-15 | success | Logs for test_verifier on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-6 | success | Logs for test_maps on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for build for x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for set-matrix |
On Thu, Sep 8, 2022 at 3:07 AM Peter Zijlstra <peterz@infradead.org> wrote: > > On Thu, Sep 08, 2022 at 11:30:41AM +0200, Peter Zijlstra wrote: > > Let me go do a patch. > > --- > Subject: x86,retpoline: Be sure to emit INT3 after JMP *%\reg > > Both AMD and Intel recommend using INT3 after an indirect JMP. Make sure > to emit one when rewriting the retpoline JMP irrespective of compiler > SLS options or even CONFIG_SLS. > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > arch/x86/kernel/alternative.c | 9 +++++++++ > arch/x86/net/bpf_jit_comp.c | 3 ++- > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c > index 62f6b8b7c4a5..68d84cf8e001 100644 > --- a/arch/x86/kernel/alternative.c > +++ b/arch/x86/kernel/alternative.c > @@ -453,6 +453,15 @@ static int patch_retpoline(void *addr, struct insn *insn, u8 *bytes) > return ret; > i += ret; > > + /* > + * The compiler is supposed to EMIT an INT3 after every unconditional > + * JMP instruction due to AMD BTC. However, if the compiler is too old > + * or SLS isn't enabled, we still need an INT3 after indirect JMPs > + * even on Intel. > + */ > + if (op == JMP32_INSN_OPCODE && i < insn->length) > + bytes[i++] = INT3_INSN_OPCODE; > + > for (; i < insn->length;) > bytes[i++] = BYTES_NOP1; > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > index c1f6c1c51d99..37f821dee68f 100644 > --- a/arch/x86/net/bpf_jit_comp.c > +++ b/arch/x86/net/bpf_jit_comp.c > @@ -419,7 +419,8 @@ static void emit_indirect_jump(u8 **pprog, int reg, u8 *ip) > OPTIMIZER_HIDE_VAR(reg); > emit_jump(&prog, &__x86_indirect_thunk_array[reg], ip); > } else { > - EMIT2(0xFF, 0xE0 + reg); > + EMIT2(0xFF, 0xE0 + reg); /* jmp *%\reg */ > + EMIT1(0xCC); /* int3 */ Hmm. Why is this unconditional? Shouldn't it be guarded with CONFIG_xx or cpu_feature_enabled ? People that don't care about hw speculation vulnerabilities shouldn't pay the price of increased code size.
On Thu, Sep 08, 2022 at 07:01:12AM -0700, Alexei Starovoitov wrote: > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > > index c1f6c1c51d99..37f821dee68f 100644 > > --- a/arch/x86/net/bpf_jit_comp.c > > +++ b/arch/x86/net/bpf_jit_comp.c > > @@ -419,7 +419,8 @@ static void emit_indirect_jump(u8 **pprog, int reg, u8 *ip) > > OPTIMIZER_HIDE_VAR(reg); > > emit_jump(&prog, &__x86_indirect_thunk_array[reg], ip); > > } else { > > - EMIT2(0xFF, 0xE0 + reg); > > + EMIT2(0xFF, 0xE0 + reg); /* jmp *%\reg */ > > + EMIT1(0xCC); /* int3 */ > > Hmm. Why is this unconditional? > Shouldn't it be guarded with CONFIG_xx or cpu_feature_enabled ? > People that don't care about hw speculation vulnerabilities > shouldn't pay the price of increased code size. Sure, like so then? --- Subject: x86,retpoline: Be sure to emit INT3 after JMP *%\reg From: Peter Zijlstra <peterz@infradead.org> Date: Thu, 8 Sep 2022 12:04:50 +0200 Both AMD and Intel recommend using INT3 after an indirect JMP. Make sure to emit one when rewriting the retpoline JMP irrespective of compiler SLS options or even CONFIG_SLS. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- arch/x86/kernel/alternative.c | 9 +++++++++ arch/x86/net/bpf_jit_comp.c | 4 +++- 2 files changed, 12 insertions(+), 1 deletion(-) --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -453,6 +453,15 @@ static int patch_retpoline(void *addr, s return ret; i += ret; + /* + * The compiler is supposed to EMIT an INT3 after every unconditional + * JMP instruction due to AMD BTC. However, if the compiler is too old + * or SLS isn't enabled, we still need an INT3 after indirect JMPs + * even on Intel. + */ + if (op == JMP32_INSN_OPCODE && i < insn->length) + bytes[i++] = INT3_INSN_OPCODE; + for (; i < insn->length;) bytes[i++] = BYTES_NOP1; --- a/arch/x86/net/bpf_jit_comp.c +++ b/arch/x86/net/bpf_jit_comp.c @@ -419,7 +419,9 @@ static void emit_indirect_jump(u8 **ppro OPTIMIZER_HIDE_VAR(reg); emit_jump(&prog, &__x86_indirect_thunk_array[reg], ip); } else { - EMIT2(0xFF, 0xE0 + reg); + EMIT2(0xFF, 0xE0 + reg); /* jmp *%\reg */ + if (IS_ENABLED(CONFIG_RETPOLINE) || IS_ENABLED(CONFIG_SLS)) + EMIT1(0xCC); /* int3 */ } *pprog = prog;
On Fri, Sep 9, 2022 at 1:16 AM Peter Zijlstra <peterz@infradead.org> wrote: > > On Thu, Sep 08, 2022 at 07:01:12AM -0700, Alexei Starovoitov wrote: > > > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > > > index c1f6c1c51d99..37f821dee68f 100644 > > > --- a/arch/x86/net/bpf_jit_comp.c > > > +++ b/arch/x86/net/bpf_jit_comp.c > > > @@ -419,7 +419,8 @@ static void emit_indirect_jump(u8 **pprog, int reg, u8 *ip) > > > OPTIMIZER_HIDE_VAR(reg); > > > emit_jump(&prog, &__x86_indirect_thunk_array[reg], ip); > > > } else { > > > - EMIT2(0xFF, 0xE0 + reg); > > > + EMIT2(0xFF, 0xE0 + reg); /* jmp *%\reg */ > > > + EMIT1(0xCC); /* int3 */ > > > > Hmm. Why is this unconditional? > > Shouldn't it be guarded with CONFIG_xx or cpu_feature_enabled ? > > People that don't care about hw speculation vulnerabilities > > shouldn't pay the price of increased code size. > > Sure, like so then? > > --- > Subject: x86,retpoline: Be sure to emit INT3 after JMP *%\reg > From: Peter Zijlstra <peterz@infradead.org> > Date: Thu, 8 Sep 2022 12:04:50 +0200 > > Both AMD and Intel recommend using INT3 after an indirect JMP. Make sure > to emit one when rewriting the retpoline JMP irrespective of compiler > SLS options or even CONFIG_SLS. > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > > arch/x86/kernel/alternative.c | 9 +++++++++ > arch/x86/net/bpf_jit_comp.c | 4 +++- > 2 files changed, 12 insertions(+), 1 deletion(-) > > --- a/arch/x86/kernel/alternative.c > +++ b/arch/x86/kernel/alternative.c > @@ -453,6 +453,15 @@ static int patch_retpoline(void *addr, s > return ret; > i += ret; > > + /* > + * The compiler is supposed to EMIT an INT3 after every unconditional > + * JMP instruction due to AMD BTC. However, if the compiler is too old > + * or SLS isn't enabled, we still need an INT3 after indirect JMPs > + * even on Intel. > + */ > + if (op == JMP32_INSN_OPCODE && i < insn->length) > + bytes[i++] = INT3_INSN_OPCODE; > + > for (; i < insn->length;) > bytes[i++] = BYTES_NOP1; > > --- a/arch/x86/net/bpf_jit_comp.c > +++ b/arch/x86/net/bpf_jit_comp.c > @@ -419,7 +419,9 @@ static void emit_indirect_jump(u8 **ppro > OPTIMIZER_HIDE_VAR(reg); > emit_jump(&prog, &__x86_indirect_thunk_array[reg], ip); > } else { > - EMIT2(0xFF, 0xE0 + reg); > + EMIT2(0xFF, 0xE0 + reg); /* jmp *%\reg */ > + if (IS_ENABLED(CONFIG_RETPOLINE) || IS_ENABLED(CONFIG_SLS)) > + EMIT1(0xCC); /* int3 */ Looks better. Ack.
On Fri, Sep 09, 2022 at 10:16:13AM +0200, Peter Zijlstra wrote: > +++ b/arch/x86/net/bpf_jit_comp.c > @@ -419,7 +419,9 @@ static void emit_indirect_jump(u8 **ppro > OPTIMIZER_HIDE_VAR(reg); > emit_jump(&prog, &__x86_indirect_thunk_array[reg], ip); > } else { > - EMIT2(0xFF, 0xE0 + reg); > + EMIT2(0xFF, 0xE0 + reg); /* jmp *%\reg */ > + if (IS_ENABLED(CONFIG_RETPOLINE) || IS_ENABLED(CONFIG_SLS)) > + EMIT1(0xCC); /* int3 */ > } Hm, if you have retpolines disabled at runtime, why would you want this.
On Fri, Sep 09, 2022 at 09:48:09AM -0700, Josh Poimboeuf wrote: > On Fri, Sep 09, 2022 at 10:16:13AM +0200, Peter Zijlstra wrote: > > +++ b/arch/x86/net/bpf_jit_comp.c > > @@ -419,7 +419,9 @@ static void emit_indirect_jump(u8 **ppro > > OPTIMIZER_HIDE_VAR(reg); > > emit_jump(&prog, &__x86_indirect_thunk_array[reg], ip); > > } else { > > - EMIT2(0xFF, 0xE0 + reg); > > + EMIT2(0xFF, 0xE0 + reg); /* jmp *%\reg */ > > + if (IS_ENABLED(CONFIG_RETPOLINE) || IS_ENABLED(CONFIG_SLS)) > > + EMIT1(0xCC); /* int3 */ > > } > > Hm, if you have retpolines disabled at runtime, why would you want this. Because I don't think eIBRS guarantees it will not SLS.
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index 62f6b8b7c4a5..68d84cf8e001 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -453,6 +453,15 @@ static int patch_retpoline(void *addr, struct insn *insn, u8 *bytes) return ret; i += ret; + /* + * The compiler is supposed to EMIT an INT3 after every unconditional + * JMP instruction due to AMD BTC. However, if the compiler is too old + * or SLS isn't enabled, we still need an INT3 after indirect JMPs + * even on Intel. + */ + if (op == JMP32_INSN_OPCODE && i < insn->length) + bytes[i++] = INT3_INSN_OPCODE; + for (; i < insn->length;) bytes[i++] = BYTES_NOP1; diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c index c1f6c1c51d99..37f821dee68f 100644 --- a/arch/x86/net/bpf_jit_comp.c +++ b/arch/x86/net/bpf_jit_comp.c @@ -419,7 +419,8 @@ static void emit_indirect_jump(u8 **pprog, int reg, u8 *ip) OPTIMIZER_HIDE_VAR(reg); emit_jump(&prog, &__x86_indirect_thunk_array[reg], ip); } else { - EMIT2(0xFF, 0xE0 + reg); + EMIT2(0xFF, 0xE0 + reg); /* jmp *%\reg */ + EMIT1(0xCC); /* int3 */ } *pprog = prog;