Make sure we don't generate direct JMP/CALL instructions to an ENDBR
instruction (which might be poison).
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
arch/x86/include/asm/text-patching.h | 6 ++++++
1 file changed, 6 insertions(+)
--- a/arch/x86/include/asm/text-patching.h
+++ b/arch/x86/include/asm/text-patching.h
@@ -5,6 +5,7 @@
#include <linux/types.h>
#include <linux/stddef.h>
#include <asm/ptrace.h>
+#include <asm/ibt.h>
struct paravirt_patch_site;
#ifdef CONFIG_PARAVIRT
@@ -101,6 +102,11 @@ void *text_gen_insn(u8 opcode, const voi
static union text_poke_insn insn; /* per instance */
int size = text_opcode_size(opcode);
+#ifdef CONFIG_X86_IBT
+ if (is_endbr(dest))
+ dest += 4;
+#endif
+
insn.opcode = opcode;
if (size > 1) {
> +#ifdef CONFIG_X86_IBT
> + if (is_endbr(dest))
> + dest += 4;
> +#endif
Hi, FWIIW I saw this snippet trigger a bug in the jump_label infra where
the target displacement would not fit in a JMP8 operand. The behavior
was seen because clang, for whatever reason (probably a bug?) inlined an
ENDBR function along with a function, thus the JMP8 target was
incremented. I compared the faulty kernel to one compiled with GCC and
the latter wont emit/inline the ENDBR.
The displacement I'm using in my experimentation is a few bytes more
than just 4, because I'm also adding extra instrumentation that should
be skipped when not reached indirectly. Of course this is more prone to
triggering the bug, but I don't think it is impossible to happen in the
current implementation.
For these cases perhaps we can verify if the displacement fits the
operand and, if not, simply ignore and lose the decode cycle which may
not be a huge problem and remains semantically correct. Seems more
sensible than padding jump tables with nops. In the meantime I'll
investigate clang's behavior and if it is really a bug, I'll work on a
patch.
On Wed, Feb 23, 2022 at 05:18:04PM -0800, Joao Moreira wrote:
> > +#ifdef CONFIG_X86_IBT
> > + if (is_endbr(dest))
> > + dest += 4;
> > +#endif
>
> Hi, FWIIW I saw this snippet trigger a bug in the jump_label infra where the
> target displacement would not fit in a JMP8 operand.
Bah, I was afraid of seening that :/
> For these cases perhaps we can verify if the displacement fits the operand
> and, if not, simply ignore and lose the decode cycle which may not be a huge
> problem and remains semantically correct. Seems more sensible than padding
> jump tables with nops. In the meantime I'll investigate clang's behavior and
> if it is really a bug, I'll work on a patch.
Urgh, trouble is, we're going to be re-writing a bunch of ENDBR to be
UD1 0x0(%eax),%eax, and you really don't want to try and execute those.