Re: [PATCH bpf-next v5 05/14] bpf: Track verifier register diagnostic events

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The old state of BPF_REG_0 is incorrectly recorded as NOT_INIT for helper calls, losing its lineage.
--

--- Patch [5]: [PATCH bpf-next v5 05/14] bpf: Track verifier register diagnostic events ---
commit 3cbb3352506d462a8a06bc8434f061a5dc31d13d
Author: Kumar Kartikeya Dwivedi <[email protected]>
Subject: bpf: Track verifier register diagnostic events

This commit introduces verification diagnostic events to track material
register and outgoing stack argument changes. This allows diagnostics to
explain how a value reached its current state, type, or bounds.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index db644690ac4bf..a5929e40f18d1 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -10518,12 +10594,14 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  		return err;
>  
>  	/* reset caller saved regs */
> +	bpf_diag_record_caller_saved(env, regs);
>  	for (i = 0; i < CALLER_SAVED_REGS; i++) {
>  		bpf_mark_reg_not_init(env, &regs[caller_saved[i]]);
>  		check_reg_arg(env, caller_saved[i], DST_OP_NO_MARK);
>  	}
>  	invalidate_outgoing_stack_args(env, cur_func(env));
>  
> +	bpf_diag_mod_begin(env, &regs[BPF_REG_0], NULL, BPF_DIAG_MOD_WRITE);
[Severity: Medium]
Does this record the old state of BPF_REG_0 as NOT_INIT?

The loop just above this line in check_helper_call() scrubs all caller-saved
registers (which includes BPF_REG_0) by calling bpf_mark_reg_not_init(). 
Because the bpf_diag_mod_begin() snapshot is taken after this loop finishes,
it seems we might capture the already-scrubbed state instead of what R0
actually was before the helper call.

Could this break the diagnostic lineage tracking for the return register?
Perhaps the bpf_diag_mod_begin() snapshot should be taken before the
caller-saved registers are cleared?

>  	/* update return register (already marked as written above) */
>  	ret_type = fn->ret_type;
>  	ret_flag = type_flag(ret_type);

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