Re: [PATCH bpf-next v6 2/6] bpf: Propagate async callback instructions to scheduling subprograms

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Unbounded `async_stats_subprog_cnt` causes OOB write in `push_async_cb` and OOB read/write in `account_current_path`.
--

--- Patch [2]: [PATCH bpf-next v6 2/6] bpf: Propagate async callback instructions to scheduling subprograms ---
commit e22fc6d02fda1f775515a1bbc9834113d0ba7767
Author: Kumar Kartikeya Dwivedi <[email protected]>

bpf: Propagate async callback instructions to scheduling subprograms

This commit propagates the instruction budget from asynchronous callback
executions back to the subprograms that originally scheduled them. It tracks
active subprogram IDs when a callback is queued and adds the inclusive
subtotal of the async frame-zero path to the saved scheduling subprograms.

> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -447,6 +447,8 @@ static_assert(MAX_BPF_STACK / 8 <= (1 << 6));
>  struct bpf_verifier_state {
>  	/* call stack tracking */
>  	struct bpf_func_state *frame[MAX_CALL_FRAMES];
> +	u32 async_stats_subprog_ids[MAX_CALL_FRAMES];
> +	u32 async_stats_subprog_cnt;

[Severity: Critical]
Is it possible for the accumulated call chains of all ancestral scheduling
events to exceed MAX_CALL_FRAMES? If a BPF program chains async callbacks
(like a timer callback scheduling another timer callback), the count might
grow indefinitely.

>  	struct bpf_verifier_state *parent;
>  	/* Acquired reference states */
>  	struct bpf_reference_state *refs;

[ ... ]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -2293,6 +2298,12 @@ static struct bpf_verifier_state *push_async_cb(struct bpf_verifier_env *env,
>  			0 /* frameno within this callchain */,
>  			subprog /* subprog number within this prog */);
>  	elem->st.frame[0] = frame;
> +	cnt = env->cur_state->async_stats_subprog_cnt;
> +	memcpy(elem->st.async_stats_subprog_ids, env->cur_state->async_stats_subprog_ids,
> +	       cnt * sizeof(elem->st.async_stats_subprog_ids[0]));
> +	for (i = 0; i <= env->cur_state->curframe; i++)
> +		elem->st.async_stats_subprog_ids[cnt++] = env->cur_state->frame[i]->subprogno;
> +	elem->st.async_stats_subprog_cnt = cnt;

[Severity: Critical]
Can this loop overflow the async_stats_subprog_ids array? Since cnt is
incremented without checking against MAX_CALL_FRAMES, it appears this could
write past the end of the array, corrupting adjacent fields in struct
bpf_verifier_state like the parent pointer.

>  	return &elem->st;
>  }

[ ... ]

> @@ -9828,16 +9839,24 @@ static void account_processed_insns(struct bpf_verifier_env *env,
>  	if (caller)
>  		caller->insns_subtotal += insns;
>  	callee->insns_subtotal = 0;
> +	return insns;
>  }
>  
>  static void account_current_path(struct bpf_verifier_env *env)
>  {
>  	struct bpf_verifier_state *state = env->cur_state;
> -	int frame;
> +	u32 insns;
> +	int frame, i;
>  
>  	for (frame = state->curframe; frame >= 0; frame--)
> -		account_processed_insns(env, state->frame[frame],
> -					frame ? state->frame[frame - 1] : NULL);
> +		insns = account_processed_insns(env, state->frame[frame],
> +						frame ? state->frame[frame - 1] : NULL);
> +
> +	if (!state->async_stats_subprog_cnt)
> +		return;
> +
> +	for (i = 0; i < state->async_stats_subprog_cnt; i++)
> +		env->subprog_info[state->async_stats_subprog_ids[i]].insns_total += insns;

[Severity: Critical]
If async_stats_subprog_cnt was corrupted and grew beyond MAX_CALL_FRAMES, does
this result in an out-of-bounds read from async_stats_subprog_ids? This could
read from the corrupted parent pointer and cause an arbitrary kernel memory
write into env->subprog_info.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2