[PATCHv2,bpf,2/2] bpf: Fix prog_array_map_poke_run map poke update

Commit Message

Jiri Olsa Nov. 28, 2023, 9:28 a.m. UTC

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 imballance 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 calling bpf_arch_text_poke with 'checkip=false' to skip the
bpf symbol check like we do in bpf_tail_call_direct_fixup. This way the
prog_array_map_poke_run does not depend on bpf program having the kallsym
symbol in place.

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

Cc: Lee Jones <[e-mail]>
Cc: Maciej Fijalkowski <[e-mail]>
Fixes: ebf7d1f508a7 ("bpf, x64: rework pro/epilogue and tailcall handling in JIT")
Reported-by: [e-mail]
Signed-off-by: Jiri Olsa <[e-mail]>
---
 kernel/bpf/arraymap.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

Patch

diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 7ba389f7212f..b194282eacbb 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -1044,20 +1044,11 @@  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()
+			 *    buffer is freed.
+			 * 4) Any error happening below from bpf_arch_text_poke()
 			 *    is a unexpected bug.
 			 */
 			if (!READ_ONCE(poke->tailcall_target_stable))
@@ -1075,21 +1066,21 @@  static void prog_array_map_poke_run(struct bpf_map *map, u32 key,
 			if (new) {
 				ret = bpf_arch_text_poke(poke->tailcall_target,
 							 BPF_MOD_JUMP,
-							 old_addr, new_addr, true);
-				BUG_ON(ret < 0 && ret != -EINVAL);
+							 old_addr, new_addr, false);
+				BUG_ON(ret < 0);
 				if (!old) {
 					ret = bpf_arch_text_poke(poke->tailcall_bypass,
 								 BPF_MOD_JUMP,
 								 poke->bypass_addr,
-								 NULL, true);
-					BUG_ON(ret < 0 && ret != -EINVAL);
+								 NULL, false);
+					BUG_ON(ret < 0);
 				}
 			} else {
 				ret = bpf_arch_text_poke(poke->tailcall_bypass,
 							 BPF_MOD_JUMP,
 							 old_bypass_addr,
-							 poke->bypass_addr, true);
-				BUG_ON(ret < 0 && ret != -EINVAL);
+							 poke->bypass_addr, false);
+				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
@@ -1098,8 +1089,8 @@  static void prog_array_map_poke_run(struct bpf_map *map, u32 key,
 					synchronize_rcu();
 				ret = bpf_arch_text_poke(poke->tailcall_target,
 							 BPF_MOD_JUMP,
-							 old_addr, NULL, true);
-				BUG_ON(ret < 0 && ret != -EINVAL);
+							 old_addr, NULL, false);
+				BUG_ON(ret < 0);
 			}
 		}
 	}