Re: [PATCH bpf-next v3 11/51] bpf: compute parameters liveness in bpf_compute_live_registers()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: state pruning bypasses NOT_INIT check for static subprog args
--

commit 9fa2cdb3328d1fdeb0246efe76e84a8ecaca85e6
Author: Eduard Zingerman <eddyz87@gmail.com>

bpf: compute parameters liveness in bpf_compute_live_registers()

This commit changes the verifier to compute exact liveness for the
arguments of calls to subprograms instead of assuming r1..r5 are read.
It adds a separate pass to prepare BTF function arguments early so that
liveness can be accurately determined.

> diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c
> index a359e457e6c0c..8d8428ccc5052 100644
> --- a/kernel/bpf/liveness.c
> +++ b/kernel/bpf/liveness.c

[ ... ]

> @@ -2613,8 +2620,18 @@ static void compute_insn_live_regs(struct bpf_verifier_env *env,
>  		case BPF_CALL:
>  			def = ALL_CALLER_SAVED_REGS;
>  			use = def & ~BIT(BPF_REG_0);
> -			if (bpf_get_call_summary(env, insn, &cs))
> +			if (bpf_get_call_summary(env, insn, &cs)) {
>  				use = GENMASK(min_t(u8, cs.arg_slot_cnt, MAX_BPF_FUNC_REG_ARGS), 1);
> +			} else if (bpf_pseudo_call(insn)) {
> +				/* a static callee's reads are added in bpf_compute_live_registers() */
> +				subprog = callee_subprog(env, insn);
> +				if (bpf_subprog_is_global(env, subprog)) {
> +					reg_args = min_t(u8, env->subprog_info[subprog].arg_slot_cnt, MAX_BPF_FUNC_REG_ARGS);
> +					use = GENMASK(reg_args, 1);
> +				} else {
> +					use = 0;
> +				}
> +			}

[Severity: High]
Does marking these unused arguments as not live for static subprograms
interact poorly with verifier state pruning? 

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index a5456a6e26946..50cb564be9f74 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -10939,6 +10940,15 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog, s
>  	meta.fn = fn;
>  	meta.func_proto = func_proto;
>  
> +	/*
> +	 * Check if all register arguments declared in BTF are initialized.
> +	 * If they aren't the BTF is unreliable.
> +	 */
> +	if (!bpf_subprog_is_global(env, subprog)) {
> +		for (i = 0; i < min_t(u32, sub->arg_slot_cnt, MAX_BPF_FUNC_REG_ARGS); i++)
> +			if (caller->regs[BPF_REG_1 + i].type == NOT_INIT)
> +				return -EINVAL;
> +	}
>  	return check_func_args(env, &meta, env->insn_idx);
>  }

[Severity: High]
Because unused arguments are no longer marked as live due to the change
in compute_insn_live_regs(), can this NOT_INIT check be completely
bypassed by regsafe()?

If the verifier first explores a path where the argument is initialized,
the subprogram is not marked unreliable. If it subsequently explores
another path where the argument is left NOT_INIT, this second path
will be pruned before evaluating the BPF_CALL instruction and hitting
btf_check_func_arg_match().

Could this result in the subprogram remaining "reliable", allowing
fentry/freplace extension programs to attach to it and potentially
leak uninitialized kernel memory via the BTF context?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-scev-minimal-rebase-v3-0-bcba9bc53ac9@gmail.com?part=11