Re: [PATCH bpf-next v4 05/16] bpf: Track verifier register diagnostic events

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing diagnostic event tracking for BPF_LD_IMM64 and BPF_ATOMIC (with BPF_FETCH) operations.
--

--- Patch [5]: [PATCH bpf-next v4 05/16] bpf: Track verifier register diagnostic events ---
commit 7db736a554444e8ded5ae93ab09a9f7f17a9c87b
Author: Kumar Kartikeya Dwivedi <[email protected]>

    bpf: Track verifier register diagnostic events

This commit introduces verifier register diagnostic events to track how a
value reached its current state. It records register modifications, stack
spills, reference invalidations, and other events in the diagnostic history.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2e915292265b0..ed8a239a964d9 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -6391,15 +6421,19 @@ static int check_load_mem(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  
>  	src_reg_type = regs[insn->src_reg].type;
>  
> -	/* Check if (src_reg + off) is readable. The state of dst_reg will be
> -	 * updated by this call.
> +	/*
> +	 * check_stack_read_fixed_off() may refine the modification's origin to
> +	 * the source stack slot.
>  	 */
> +	bpf_diag_mod_begin(env, &regs[insn->dst_reg], NULL, BPF_DIAG_MOD_WRITE);
>  	err = check_mem_access(env, env->insn_idx, regs + insn->src_reg, argno_from_reg(insn->src_reg), insn->off,
>  			       BPF_SIZE(insn->code), BPF_READ, insn->dst_reg,
>  			       strict_alignment_once, is_ldsx);
>  	err = err ?: save_aux_ptr_type(env, src_reg_type,
>  				       allow_trust_mismatch);
>  	err = err ?: reg_bounds_sanity_check(env, &regs[insn->dst_reg], ctx);
> +	if (!err)
> +		bpf_diag_mod_end(env);
>  
>  	return err;
>  }

[Severity: Medium]
Are we missing similar diagnostic event tracking for BPF_LD_IMM64 and
BPF_ATOMIC (with BPF_FETCH) operations?

For example, in check_ld_imm(), we directly modify the destination register
without calling bpf_diag_mod_begin() or bpf_diag_mod_end(), which prevents
the origin of the value from being recorded:

	dst_reg = &regs[insn->dst_reg];
	if (insn->src_reg == 0) {
		u64 imm = ((u64)(insn + 1)->imm << 32) | (u32)insn->imm;

		dst_reg->type = SCALAR_VALUE;
		__mark_reg_known(&regs[insn->dst_reg], imm);
		return 0;
	}

Additionally, in check_atomic_rmw(), we modify load_reg but lack diagnostic
event tracing. If stack memory is fetched, check_stack_read_fixed_off() calls
bpf_diag_mod_begin(), but bpf_diag_mod_end() is never called afterwards. 

	load_reg = bpf_atomic_load_reg(insn);
	if (load_reg >= 0) {
		/* check and record load of old value */
		err = check_reg_arg(env, load_reg, DST_OP);
		if (err)
			return err;
	}

	dst_reg = cur_regs(env) + insn->dst_reg;

	/* Check whether we can read the memory, with second call for fetch
	 * case to simulate the register fill.
	 */
	err = check_mem_access(env, env->insn_idx, dst_reg, argno_from_reg(insn->dst_reg), insn->off,
			       BPF_SIZE(insn->code), BPF_READ, -1, true, false);
	if (!err && load_reg >= 0)
		err = check_mem_access(env, env->insn_idx, dst_reg, argno_from_reg(insn->dst_reg),
				       insn->off, BPF_SIZE(insn->code),
				       BPF_READ, load_reg, true, false);

Does this leak the active modification state and cause the stack fetch event
to be permanently lost? Should these paths also be instrumented to track
value origins correctly?

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