Commit 4b7de801 authored by Jiri Olsa's avatar Jiri Olsa Committed by Daniel Borkmann
Browse files

bpf: Fix prog_array_map_poke_run map poke update 

Lee pointed out issue found by syscaller [0] hitting BUG in prog array
map poke update in prog_array_map_poke_run function due to error value
returned from bpf_arch_text_poke function.

There's race window where bpf_arch_text_poke can fail due to missing
bpf program kallsym symbols, which is accounted for with check for
-EINVAL in that BUG_ON call.

The problem is that in such case we won't update the tail call jump
and cause imbalance for the next tail call update check which will
fail with -EBUSY in bpf_arch_text_poke.

I'm hitting following race during the program load:

  CPU 0                             CPU 1

  bpf_prog_load
    bpf_check
      do_misc_fixups
        prog_array_map_poke_track

                                    map_update_elem
                                      bpf_fd_array_map_update_elem
                                        prog_array_map_poke_run

                                          bpf_arch_text_poke returns -EINVAL

    bpf_prog_kallsyms_add

After bpf_arch_text_poke (CPU 1) fails to update the tail call jump, the next
poke update fails on expected jump instruction check in bpf_arch_text_poke
with -EBUSY and triggers the BUG_ON in prog_array_map_poke_run.

Similar race exists on the program unload.

Fixing this by moving the update to bpf_arch_poke_desc_update function which
makes sure we call __bpf_arch_text_poke that skips the bpf address check.

Each architecture has slightly different approach wrt looking up bpf address
in bpf_arch_text_poke, so instead of splitting the function or adding new
'checkip' argument in previous version, it seems best to move the whole
map_poke_run update as arch specific code.

  [0] https://syzkaller.appspot.com/bug?extid=97a4fe20470e9bc30810



Fixes: ebf7d1f5 ("bpf, x64: rework pro/epilogue and tailcall handling in JIT")
Reported-by: default avatar <syzbot+97a4fe20470e9bc30810@syzkaller.appspotmail.com>
Signed-off-by: default avatarJiri Olsa <jolsa@kernel.org>
Signed-off-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
Acked-by: default avatarYonghong Song <yonghong.song@linux.dev>
Cc: Lee Jones <lee@kernel.org>
Cc: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Link: https://lore.kernel.org/bpf/20231206083041.1306660-2-jolsa@kernel.org
parent e4d008d4

arch/x86/net/bpf_jit_comp.c

+46 −0
Original line number Diff line number Diff line
@@ -3025,3 +3025,49 @@ void arch_bpf_stack_walk(bool (*consume_fn)(void *cookie, u64 ip, u64 sp, u64 bp 
#endif

	WARN(1, "verification of programs using bpf_throw should have failed\n");

}



void bpf_arch_poke_desc_update(struct bpf_jit_poke_descriptor *poke,

			       struct bpf_prog *new, struct bpf_prog *old)

{

	u8 *old_addr, *new_addr, *old_bypass_addr;

	int ret;



	old_bypass_addr = old ? NULL : poke->bypass_addr;

	old_addr = old ? (u8 *)old->bpf_func + poke->adj_off : NULL;

	new_addr = new ? (u8 *)new->bpf_func + poke->adj_off : NULL;



	/*

	 * On program loading or teardown, the program's kallsym entry

	 * might not be in place, so we use __bpf_arch_text_poke to skip

	 * the kallsyms check.

	 */

	if (new) {

		ret = __bpf_arch_text_poke(poke->tailcall_target,

					   BPF_MOD_JUMP,

					   old_addr, new_addr);

		BUG_ON(ret < 0);

		if (!old) {

			ret = __bpf_arch_text_poke(poke->tailcall_bypass,

						   BPF_MOD_JUMP,

						   poke->bypass_addr,

						   NULL);

			BUG_ON(ret < 0);

		}

	} else {

		ret = __bpf_arch_text_poke(poke->tailcall_bypass,

					   BPF_MOD_JUMP,

					   old_bypass_addr,

					   poke->bypass_addr);

		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,

					   BPF_MOD_JUMP,

					   old_addr, NULL);

		BUG_ON(ret < 0);

	}

}

include/linux/bpf.h

+3 −0
Original line number Diff line number Diff line
@@ -3175,6 +3175,9 @@ enum bpf_text_poke_type { 
int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,

		       void *addr1, void *addr2);



void bpf_arch_poke_desc_update(struct bpf_jit_poke_descriptor *poke,

			       struct bpf_prog *new, struct bpf_prog *old);



void *bpf_arch_text_copy(void *dst, void *src, size_t len);

int bpf_arch_text_invalidate(void *dst, size_t len);

kernel/bpf/arraymap.c

+10 −48
Original line number Diff line number Diff line
@@ -1012,11 +1012,16 @@ static void prog_array_map_poke_untrack(struct bpf_map *map, 
	mutex_unlock(&aux->poke_mutex);

}



void __weak bpf_arch_poke_desc_update(struct bpf_jit_poke_descriptor *poke,

				      struct bpf_prog *new, struct bpf_prog *old)

{

	WARN_ON_ONCE(1);

}



static void prog_array_map_poke_run(struct bpf_map *map, u32 key,

				    struct bpf_prog *old,

				    struct bpf_prog *new)

{

	u8 *old_addr, *new_addr, *old_bypass_addr;

	struct prog_poke_elem *elem;

	struct bpf_array_aux *aux;
@@ -1025,7 +1030,7 @@ static void prog_array_map_poke_run(struct bpf_map *map, u32 key, 


	list_for_each_entry(elem, &aux->poke_progs, list) {

		struct bpf_jit_poke_descriptor *poke;

		int i, ret;

		int i;



		for (i = 0; i < elem->aux->size_poke_tab; i++) {

			poke = &elem->aux->poke_tab[i];
@@ -1044,21 +1049,10 @@ static void prog_array_map_poke_run(struct bpf_map *map, u32 key, 
			 *    activated, so tail call updates can arrive from here

			 *    while JIT is still finishing its final fixup for

			 *    non-activated poke entries.

			 * 3) On program teardown, the program's kallsym entry gets

			 *    removed out of RCU callback, but we can only untrack

			 *    from sleepable context, therefore bpf_arch_text_poke()

			 *    might not see that this is in BPF text section and

			 *    bails out with -EINVAL. As these are unreachable since

			 *    RCU grace period already passed, we simply skip them.

			 * 4) Also programs reaching refcount of zero while patching

			 * 3) Also programs reaching refcount of zero while patching

			 *    is in progress is okay since we're protected under

			 *    poke_mutex and untrack the programs before the JIT

			 *    buffer is freed. When we're still in the middle of

			 *    patching and suddenly kallsyms entry of the program

			 *    gets evicted, we just skip the rest which is fine due

			 *    to point 3).

			 * 5) Any other error happening below from bpf_arch_text_poke()

			 *    is a unexpected bug.

			 *    buffer is freed.

			 */

			if (!READ_ONCE(poke->tailcall_target_stable))

				continue;
@@ -1068,39 +1062,7 @@ static void prog_array_map_poke_run(struct bpf_map *map, u32 key, 
			    poke->tail_call.key != key)

				continue;



			old_bypass_addr = old ? NULL : poke->bypass_addr;

			old_addr = old ? (u8 *)old->bpf_func + poke->adj_off : NULL;

			new_addr = new ? (u8 *)new->bpf_func + poke->adj_off : NULL;



			if (new) {

				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,

								 BPF_MOD_JUMP,

								 poke->bypass_addr,

								 NULL);

					BUG_ON(ret < 0 && ret != -EINVAL);

				}

			} else {

				ret = bpf_arch_text_poke(poke->tailcall_bypass,

							 BPF_MOD_JUMP,

							 old_bypass_addr,

							 poke->bypass_addr);

				BUG_ON(ret < 0 && ret != -EINVAL);

				/* 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,

							 BPF_MOD_JUMP,

							 old_addr, NULL);

				BUG_ON(ret < 0 && ret != -EINVAL);

			}

			bpf_arch_poke_desc_update(poke, new, old);

		}

	}

}