With IBT enabled builds we need ENDBR instructions at indirect jump
target sites, since we start execution of the JIT'ed code through an
indirect jump, the very first instruction needs to be ENDBR.
Similarly, since eBPF tail-calls use indirect branches, their landing
site needs to be an ENDBR too.
The trampolines need similar adjustment.
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
arch/x86/net/bpf_jit_comp.c | 37 +++++++++++++++++++++++++++++++------
kernel/bpf/trampoline.c | 20 ++++----------------
2 files changed, 35 insertions(+), 22 deletions(-)
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -46,6 +46,12 @@ static u8 *emit_code(u8 *ptr, u32 bytes,
#define EMIT4_off32(b1, b2, b3, b4, off) \
do { EMIT4(b1, b2, b3, b4); EMIT(off, 4); } while (0)
+#ifdef CONFIG_X86_KERNEL_IBT
+#define EMIT_ENDBR() EMIT(gen_endbr(), 4)
+#else
+#define EMIT_ENDBR()
+#endif
+
static bool is_imm8(int value)
{
return value <= 127 && value >= -128;
@@ -241,7 +247,7 @@ struct jit_context {
/* Number of bytes emit_patch() needs to generate instructions */
#define X86_PATCH_SIZE 5
/* Number of bytes that will be skipped on tailcall */
-#define X86_TAIL_CALL_OFFSET 11
+#define X86_TAIL_CALL_OFFSET (11 + 4*HAS_KERNEL_IBT)
static void push_callee_regs(u8 **pprog, bool *callee_regs_used)
{
@@ -286,16 +292,21 @@ static void emit_prologue(u8 **pprog, u3
/* BPF trampoline can be made to work without these nops,
* but let's waste 5 bytes for now and optimize later
*/
- memcpy(prog, x86_nops[5], X86_PATCH_SIZE);
+ EMIT_ENDBR();
+ memcpy(prog, x86_nops[5], X86_PATCH_SIZE); /* = 5 */
prog += X86_PATCH_SIZE;
if (!ebpf_from_cbpf) {
if (tail_call_reachable && !is_subprog)
- EMIT2(0x31, 0xC0); /* xor eax, eax */
+ EMIT2(0x31, 0xC0); /* xor eax, eax */ /* +2 = 7 */
else
EMIT2(0x66, 0x90); /* nop2 */
}
- EMIT1(0x55); /* push rbp */
- EMIT3(0x48, 0x89, 0xE5); /* mov rbp, rsp */
+ EMIT1(0x55); /* push rbp */ /* +1 = 8 */
+ EMIT3(0x48, 0x89, 0xE5); /* mov rbp, rsp */ /* +3 = 11 */
+
+ /* X86_TAIL_CALL_OFFSET is here */
+ EMIT_ENDBR();
+
/* sub rsp, rounded_stack_depth */
if (stack_depth)
EMIT3_off32(0x48, 0x81, 0xEC, round_up(stack_depth, 8));
@@ -339,9 +350,18 @@ static int __bpf_arch_text_poke(void *ip
u8 *prog;
int ret;
+#ifdef CONFIG_X86_KERNEL_IBT
+ if (is_endbr(*(u32 *)ip))
+ ip += 4;
+#endif
+
memcpy(old_insn, nop_insn, X86_PATCH_SIZE);
if (old_addr) {
prog = old_insn;
+#ifdef CONFIG_X86_KERNEL_IBT
+ if (is_endbr(*(u32 *)old_addr))
+ old_addr += 4;
+#endif
ret = t == BPF_MOD_CALL ?
emit_call(&prog, old_addr, ip) :
emit_jump(&prog, old_addr, ip);
@@ -352,6 +372,10 @@ static int __bpf_arch_text_poke(void *ip
memcpy(new_insn, nop_insn, X86_PATCH_SIZE);
if (new_addr) {
prog = new_insn;
+#ifdef CONFIG_X86_KERNEL_IBT
+ if (is_endbr(*(u32 *)new_addr))
+ new_addr += 4;
+#endif
ret = t == BPF_MOD_CALL ?
emit_call(&prog, new_addr, ip) :
emit_jump(&prog, new_addr, ip);
@@ -2028,10 +2052,11 @@ int arch_prepare_bpf_trampoline(struct b
/* skip patched call instruction and point orig_call to actual
* body of the kernel function.
*/
- orig_call += X86_PATCH_SIZE;
+ orig_call += X86_PATCH_SIZE + 4*HAS_KERNEL_IBT;
prog = image;
+ EMIT_ENDBR();
EMIT1(0x55); /* push rbp */
EMIT3(0x48, 0x89, 0xE5); /* mov rbp, rsp */
EMIT4(0x48, 0x83, 0xEC, stack_size); /* sub rsp, stack_size */
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -117,18 +117,6 @@ static void bpf_trampoline_module_put(st
tr->mod = NULL;
}
-static int is_ftrace_location(void *ip)
-{
- long addr;
-
- addr = ftrace_location((long)ip);
- if (!addr)
- return 0;
- if (WARN_ON_ONCE(addr != (long)ip))
- return -EFAULT;
- return 1;
-}
-
static int unregister_fentry(struct bpf_trampoline *tr, void *old_addr)
{
void *ip = tr->func.addr;
@@ -160,12 +148,12 @@ static int modify_fentry(struct bpf_tram
static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
{
void *ip = tr->func.addr;
+ unsigned long faddr;
int ret;
- ret = is_ftrace_location(ip);
- if (ret < 0)
- return ret;
- tr->func.ftrace_managed = ret;
+ faddr = ftrace_location((unsigned long)ip);
+ if (faddr)
+ tr->func.ftrace_managed = true;
if (bpf_trampoline_module_get(tr))
return -ENOENT;
On Thu, Feb 24, 2022 at 03:51:54PM +0100, Peter Zijlstra wrote:
> @@ -339,9 +350,18 @@ static int __bpf_arch_text_poke(void *ip
> u8 *prog;
> int ret;
>
> +#ifdef CONFIG_X86_KERNEL_IBT
> + if (is_endbr(*(u32 *)ip))
> + ip += 4;
> +#endif
> +
> memcpy(old_insn, nop_insn, X86_PATCH_SIZE);
> if (old_addr) {
> prog = old_insn;
> +#ifdef CONFIG_X86_KERNEL_IBT
> + if (is_endbr(*(u32 *)old_addr))
> + old_addr += 4;
> +#endif
> ret = t == BPF_MOD_CALL ?
> emit_call(&prog, old_addr, ip) :
> emit_jump(&prog, old_addr, ip);
> @@ -352,6 +372,10 @@ static int __bpf_arch_text_poke(void *ip
> memcpy(new_insn, nop_insn, X86_PATCH_SIZE);
> if (new_addr) {
> prog = new_insn;
> +#ifdef CONFIG_X86_KERNEL_IBT
> + if (is_endbr(*(u32 *)new_addr))
> + new_addr += 4;
> +#endif
All the above ifdef-itis should be able to be removed since is_endbr()
returns false for !IBT.
> ret = t == BPF_MOD_CALL ?
> emit_call(&prog, new_addr, ip) :
> emit_jump(&prog, new_addr, ip);
> @@ -2028,10 +2052,11 @@ int arch_prepare_bpf_trampoline(struct b
> /* skip patched call instruction and point orig_call to actual
> * body of the kernel function.
> */
> - orig_call += X86_PATCH_SIZE;
> + orig_call += X86_PATCH_SIZE + 4*HAS_KERNEL_IBT;
All the "4*HAS_KERNEL_IBT" everywhere is cute, but you might as well
just have IBT_ENDBR_SIZE (here and in other patches).
--
Josh
On Thu, Feb 24, 2022 at 03:37:31PM -0800, Josh Poimboeuf wrote:
> On Thu, Feb 24, 2022 at 03:51:54PM +0100, Peter Zijlstra wrote:
> > @@ -339,9 +350,18 @@ static int __bpf_arch_text_poke(void *ip
> > u8 *prog;
> > int ret;
> >
> > +#ifdef CONFIG_X86_KERNEL_IBT
> > + if (is_endbr(*(u32 *)ip))
> > + ip += 4;
> > +#endif
> > +
> > memcpy(old_insn, nop_insn, X86_PATCH_SIZE);
> > if (old_addr) {
> > prog = old_insn;
> > +#ifdef CONFIG_X86_KERNEL_IBT
> > + if (is_endbr(*(u32 *)old_addr))
> > + old_addr += 4;
> > +#endif
> > ret = t == BPF_MOD_CALL ?
> > emit_call(&prog, old_addr, ip) :
> > emit_jump(&prog, old_addr, ip);
> > @@ -352,6 +372,10 @@ static int __bpf_arch_text_poke(void *ip
> > memcpy(new_insn, nop_insn, X86_PATCH_SIZE);
> > if (new_addr) {
> > prog = new_insn;
> > +#ifdef CONFIG_X86_KERNEL_IBT
> > + if (is_endbr(*(u32 *)new_addr))
> > + new_addr += 4;
> > +#endif
>
> All the above ifdef-itis should be able to be removed since is_endbr()
> returns false for !IBT.
So I've been pondering a skip_endbr() function for all these sites.
I just couldn't decide making it a 'function' and having a signature
like:
ip = skip_endbr(ip);
or making it macro magic and it reading:
skip_endbr(ip);
Which is why I've not cleaned it up yet. This being C(ish) I suppose
the former is less confusing, so let me go do that.
On Thu, Feb 24, 2022 at 03:37:31PM -0800, Josh Poimboeuf wrote:
> > @@ -2028,10 +2052,11 @@ int arch_prepare_bpf_trampoline(struct b
> > /* skip patched call instruction and point orig_call to actual
> > * body of the kernel function.
> > */
> > - orig_call += X86_PATCH_SIZE;
> > + orig_call += X86_PATCH_SIZE + 4*HAS_KERNEL_IBT;
>
> All the "4*HAS_KERNEL_IBT" everywhere is cute, but you might as well
> just have IBT_ENDBR_SIZE (here and in other patches).
So there's two forms of this, only one has the 4 included:
(x * (1 + HAS_KERNEL_IBT))
(x + 4*HAS_KERNEL_IBT)
If I include the 4, then the first form would become something like:
(x * (1 + !!IBT_ENDBR_SIZE))
that ok?
On Thu, Feb 24, 2022 at 03:37:31PM -0800, Josh Poimboeuf wrote:
> All the "4*HAS_KERNEL_IBT" everywhere is cute, but you might as well
> just have IBT_ENDBR_SIZE (here and in other patches).
Oops, I should have read ahead. Yeah, I like folding the multiplication
into the macro...
--
Kees Cook
On Fri, Feb 25, 2022 at 01:24:58PM +0100, Peter Zijlstra wrote:
> On Thu, Feb 24, 2022 at 03:37:31PM -0800, Josh Poimboeuf wrote:
>
> > > @@ -2028,10 +2052,11 @@ int arch_prepare_bpf_trampoline(struct b
> > > /* skip patched call instruction and point orig_call to actual
> > > * body of the kernel function.
> > > */
> > > - orig_call += X86_PATCH_SIZE;
> > > + orig_call += X86_PATCH_SIZE + 4*HAS_KERNEL_IBT;
> >
> > All the "4*HAS_KERNEL_IBT" everywhere is cute, but you might as well
> > just have IBT_ENDBR_SIZE (here and in other patches).
>
> So there's two forms of this, only one has the 4 included:
>
> (x * (1 + HAS_KERNEL_IBT))
> (x + 4*HAS_KERNEL_IBT)
>
> If I include the 4, then the first form would become something like:
>
> (x * (1 + !!IBT_ENDBR_SIZE))
>
> that ok?
I don't have a strong preference, but there's no harming in having both
IBT_ENDBR_SIZE and HAS_KERNEL_IBT if it would help readability.
--
Josh