Re: [PATCH bpf-next v1 05/17] bpf: Track verifier register diagnostic history

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Excessive stack allocation in diagnostic formatting functions can cause kernel stack overflows.
- [Medium] Uninitialized stack memory read in check_stack_arg_write() due to uninitialized old_arg.
- [Medium] Diagnostic history records incorrect frame numbers for registers invalidated in caller frames.
--

commit 9a17934339f89b801d41d6242f00c9032267ab0f
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Track verifier register diagnostic history

Record material register and outgoing stack argument changes so diagnostics can
explain how a value reached its current type, bounds, or unreadable state.
Store old and new register types, scalar ranges, tnum value and mask, map and
BTF type identity, and basic operand metadata.

> diff --git a/kernel/bpf/diagnostics.c b/kernel/bpf/diagnostics.c
> index 50cc50a394cae..1bdc7e69fbced 100644
> --- a/kernel/bpf/diagnostics.c
> +++ b/kernel/bpf/diagnostics.c
[ ... ]
> +static void bpf_diag_record_reg_mod_reason(struct bpf_verifier_state *state,
> +					   u32 insn_idx, u8 dst_reg,
> +					   bool src_valid, u8 src_reg,
> +					   u8 opcode,
> +					   enum bpf_diag_reg_mod_reason reason,
> +					   const struct bpf_reg_state *old_reg,
> +					   const struct bpf_reg_state *new_reg)
> +{
> +	struct bpf_diag_history_event event = {
> +		.insn_idx = insn_idx,
> +		.kind = BPF_DIAG_HISTORY_REG_MOD,
> +		.reg.dst_reg = dst_reg,
> +		.reg.src_reg = src_reg,
> +		.reg.opcode = opcode,
> +		.reg.src_valid = src_valid,
> +	};
> +
> +	if (state && state->frame[state->curframe])
> +		event.reg.frameno = state->frame[state->curframe]->frameno;

[Severity: Medium]
Will this record the wrong frame number for registers invalidated in caller
frames?

Functions like clear_all_pkt_pointers() and release_reference() iterate over
all active frames to invalidate registers, but this assignment hardcodes the
current executing frame number via state->curframe.

Should bpf_diag_record_reg_mod_reason() accept the actual frame number of the
register from the caller instead?

[ ... ]
> +static void bpf_diag_format_reg_snapshot(struct bpf_verifier_env *env, char *buf,
> +					 size_t size,
> +					 const struct bpf_diag_reg_snapshot *snapshot)
> +{
> +	const char *type_name = reg_type_str(env, snapshot->type);
> +	char offset_desc[BPF_DIAG_REG_DESC_LEN];
> +	char btf_type[BPF_DIAG_REG_DESC_LEN];

[Severity: High]
Could this cause a kernel stack overflow?

BPF_DIAG_REG_DESC_LEN is 512 bytes. This function allocates 1KB on the stack,
and is called by bpf_diag_print_reg_mod() which also allocates 1KB.

Additionally, this function calls bpf_diag_format_var_offset() which
allocates another 512 bytes. This >3KB stack usage chain might trigger
CONFIG_FRAME_WARN build failures or cause panics on architectures with 8KB
stacks.

[ ... ]
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d60e1309957a7..188201d48108c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4033,6 +4033,8 @@ static int check_stack_arg_write(struct bpf_verifier_env *env, struct bpf_func_s
>  	struct bpf_subprog_info *subprog = &env->subprog_info[state->subprogno];
>  	int spi = -off / BPF_REG_SIZE - 1;
>  	struct bpf_reg_state *arg;
> +	struct bpf_reg_state old_arg;
> +	bool slot_exists;
>  	int err;
>  
>  	if (spi >= max_stack_arg_regs) {
> @@ -4041,6 +4043,7 @@ static int check_stack_arg_write(struct bpf_verifier_env *env, struct bpf_func_s
>  		return -EINVAL;
>  	}
>  
> +	slot_exists = spi < state->out_stack_arg_cnt;
>  	err = grow_stack_arg_slots(env, state, spi + 1);
>  	if (err)
>  		return err;
> @@ -4049,13 +4052,27 @@ static int check_stack_arg_write(struct bpf_verifier_env *env, struct bpf_func_s
>  	if (spi + 1 > subprog->max_out_stack_arg_cnt)
>  		subprog->max_out_stack_arg_cnt = spi + 1;
>  
> +	arg = &state->stack_arg_regs[spi];
> +	if (slot_exists)
> +		old_arg = *arg;
> +	else
> +		bpf_mark_reg_not_init(env, &old_arg);

[Severity: Medium]
Does this pass uninitialized stack memory to bpf_mark_reg_not_init()?

When slot_exists is false, old_arg is declared without initialization.
bpf_mark_reg_not_init() explicitly reads reg->subreg_def into a local
variable before clearing the struct, which could trigger a KMSAN
use-of-uninitialized-value warning.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605063412.974640-1-memxor@gmail.com?part=5