Re: [PATCHv4 bpf 1/2] bpf: Fix prog_array_map_poke_run map poke update

[Lee Jones <lee@kernel.org>]

Dear Stable,

> 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
> 
> Cc: Lee Jones <lee@kernel.org>
> Cc: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Fixes: ebf7d1f508a7 ("bpf, x64: rework pro/epilogue and tailcall handling in JIT")
> Reported-by: syzbot+97a4fe20470e9bc30810@syzkaller.appspotmail.com
> Acked-by: Yonghong Song <yonghong.song@linux.dev>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  arch/x86/net/bpf_jit_comp.c | 46 +++++++++++++++++++++++++++++
>  include/linux/bpf.h         |  3 ++
>  kernel/bpf/arraymap.c       | 58 +++++++------------------------------
>  3 files changed, 59 insertions(+), 48 deletions(-)

Please could we have this backported?

Guided by the Fixes: tag.

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 8c10d9abc239..e89e415aa743 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -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);
> +	}
> +}
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 6762dac3ef76..cff5bb08820e 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -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);
>  
> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index 2058e89b5ddd..c85ff9162a5c 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -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);
>  		}
>  	}
>  }
> -- 
> 2.43.0
> 

-- 
Lee Jones [李琼斯]