Re: [REPORT] BPF: Reproducible triggering of BUG() from userspace PoC

From: Jiri Olsa
Date: Thu Nov 09 2023 - 16:39:38 EST

On Wed, Nov 08, 2023 at 03:46:26PM +0000, Lee Jones wrote:
> Good afternoon,
>
> After coming across a recent Syzkaller report [0] I thought I'd take
> some time to firstly reproduce the issue, then see if there was a
> trivial way to mitigate it. The report suggests that a BUG() in
> prog_array_map_poke_run() [1] can be trivially and reliably triggered
> from userspace using the PoC provided [2].
>
> ret = bpf_arch_text_poke(poke->tailcall_bypass,
> BPF_MOD_JUMP,
> old_bypass_addr,
> poke->bypass_addr);
> BUG_ON(ret < 0 && ret != -EINVAL);
>
> Indeed the PoC does seem to be able to consistently trigger the BUG(),
> not only on the reported kernel (v6.1), but also on linux-next. I went
> to the trouble of checking LORE, but failed to find any patches which
> may be attempting to fix this.
>
> kernel BUG at kernel/bpf/arraymap.c:1094!
> invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> CPU: 5 PID: 45 Comm: kworker/5:0 Not tainted 6.6.0-rc3-next-20230929-dirty #74
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
> Workqueue: events prog_array_map_clear_deferred
> RIP: 0010:prog_array_map_poke_run+0x6b4/0x6d0
> Code: ff 0f 0b e8 1e 27 e1 ff 48 c7 c7 60 80 93 85 48 c7 c6 00 7f 93 85 48 c7 c2 bb c2 39 86 b9 45 04 00 00 45 89 f8 e8 9c 890
> RSP: 0018:ffffc9000036fb50 EFLAGS: 00010246
> RAX: 0000000000000044 RBX: ffff88811f337490 RCX: 63af48a1314f9900
> RDX: 0000000000000000 RSI: 0000000080000000 RDI: 0000000000000000
> RBP: ffffc9000036fbe8 R08: ffffffff815c23c5 R09: 1ffff11084c14eba
> R10: dfffe91084c14ebc R11: ffffed1084c14ebb R12: ffff888116517800
> R13: dffffc0000000000 R14: ffff888125a1a400 R15: 00000000fffffff0
> FS: 0000000000000000(0000) GS:ffff888426080000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00000000004ab678 CR3: 0000000122ac4000 CR4: 0000000000350eb0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> ? __die_body+0x92/0xf0
> ? die+0xa2/0xe0
> ? do_trap+0x12f/0x370
> ? handle_invalid_op+0xa6/0x140
> ? handle_invalid_op+0xdf/0x140
> ? prog_array_map_poke_run+0x6b4/0x6d0
> ? prog_array_map_poke_run+0x6b4/0x6d0
> ? exc_invalid_op+0x32/0x50
> ? asm_exc_invalid_op+0x1b/0x20
> ? __wake_up_klogd+0xd5/0x110
> ? prog_array_map_poke_run+0x6b4/0x6d0
> ? bpf_prog_6781ebc2dae4bad9+0xb/0x53
> fd_array_map_delete_elem+0x152/0x250
> prog_array_map_clear_deferred+0xf6/0x210
> ? __bpf_array_map_seq_show+0xa40/0xa40
> ? kick_pool+0x164/0x350
> ? process_one_work+0x57a/0xd00
> process_one_work+0x5e4/0xd00
> worker_thread+0x9cf/0xea0
> kthread+0x2b4/0x350
> ? pr_cont_work+0x580/0x580
> ? kthread_blkcg+0xd0/0xd0
> ret_from_fork+0x4a/0x80
> ? kthread_blkcg+0xd0/0xd0
> ret_from_fork_asm+0x11/0x20
> </TASK>
> Modules linked in:
> ---[ end trace 0000000000000000 ]---
>
> However, with my very limited BPF subsystem knowledge I was unable to
> trivially fix the issue. Hopefully some knowledgable person would be
> kind enough to provide me with some pointers.
>
> bpf_arch_text_poke() seems to be returning -EBUSY due to a negative
> memcmp() result from [3].
>
> ret = -EBUSY;
> mutex_lock(&text_mutex);
> if (memcmp(ip, old_insn, X86_PATCH_SIZE)) {
> goto out;
> [...]
>
> When spitting out the memory at those locations, this is the result:
>
> ip: e9 06 00 00 00
> old_insn: 0f 1f 44 00 00
> nop_insn: 0f 1f 44 00 00
>
> As you can see, the information stored in 'ip' does not match that of
> the data stored in 'old_insn', causing bpf_arch_text_poke() to return
> early with the error -EBUSY, suggesting that the data pointed to by
> 'old_insn', and by extension 'prog' should have been changed when
> emit_call()ing, to the value of 'ip', but wasn't.

hi,
thanks for the report.. I can reproduce that easily with [2]

AFAICS it looks like previous update fails because we use bpf_arch_text_poke,
which can't find poke->tailcall_bypass value as bpf program symbol and fails
with -EINVAL

then the following update fails to find expected jmp/nop because it was never
updated.. I think we should use __bpf_arch_text_poke like we do in
bpf_tail_call_direct_fixup and skip the bpf symbol check

with the patch below I can't reproduce the issue anymore, I'll do some more
checking though

jirka


---
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 8c10d9abc239..35c2988caf29 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -391,8 +391,8 @@ static int emit_jump(u8 **pprog, void *func, void *ip)
return emit_patch(pprog, func, ip, 0xE9);
}

-static int __bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
- void *old_addr, void *new_addr)
+int __bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
+ void *old_addr, void *new_addr)
{
const u8 *nop_insn = x86_nops[5];
u8 old_insn[X86_PATCH_SIZE];
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index eb84caf133df..0d7b8311fada 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -3172,6 +3172,8 @@ enum bpf_text_poke_type {

int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
void *addr1, void *addr2);
+int __bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
+ void *old_addr, void *new_addr);

void *bpf_arch_text_copy(void *dst, void *src, size_t len);
int bpf_arch_text_invalidate(void *dst, size_t len);
diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 2058e89b5ddd..4ab5864746ce 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -1073,33 +1073,33 @@ static void prog_array_map_poke_run(struct bpf_map *map, u32 key,
new_addr = new ? (u8 *)new->bpf_func + poke->adj_off : NULL;

if (new) {
- ret = bpf_arch_text_poke(poke->tailcall_target,
+ ret = __bpf_arch_text_poke(poke->tailcall_target,
BPF_MOD_JUMP,
old_addr, new_addr);
BUG_ON(ret < 0 && ret != -EINVAL);
if (!old) {
- ret = bpf_arch_text_poke(poke->tailcall_bypass,
+ ret = __bpf_arch_text_poke(poke->tailcall_bypass,
BPF_MOD_JUMP,
poke->bypass_addr,
NULL);
- BUG_ON(ret < 0 && ret != -EINVAL);
+ BUG_ON(ret < 0);
}
} else {
- ret = bpf_arch_text_poke(poke->tailcall_bypass,
+ ret = __bpf_arch_text_poke(poke->tailcall_bypass,
BPF_MOD_JUMP,
old_bypass_addr,
poke->bypass_addr);
- BUG_ON(ret < 0 && ret != -EINVAL);
+ BUG_ON(ret < 0);
/* let other CPUs finish the execution of program
* so that it will not possible to expose them
* to invalid nop, stack unwind, nop state
*/
if (!ret)
synchronize_rcu();
- ret = bpf_arch_text_poke(poke->tailcall_target,
+ ret = __bpf_arch_text_poke(poke->tailcall_target,
BPF_MOD_JUMP,
old_addr, NULL);
- BUG_ON(ret < 0 && ret != -EINVAL);
+ BUG_ON(ret < 0);
}
}
}