Re: [PATCH bpf v1 5/8] bpf: Mark NULL kptr stores precise

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete vulnerability fix: missing precision tracking in `bpf_register_is_null()` checks across multiple helper and kfunc argument validation paths.
--

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

bpf: Mark NULL kptr stores precise

check_map_kptr_access() permits a scalar store into an untrusted kptr
field only when the register is known to contain zero. Unlike other
verifier checks whose outcome depends on a scalar value, it does not mark
that register precise.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d7dd0befbd10e..ba9753bf529af 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4694,8 +4694,15 @@ static int check_map_kptr_access(struct bpf_verifier_env *env,
>  			return ret;
>  	} else if (class == BPF_STX) {
>  		val_reg = reg_state(env, value_regno);
> -		if (!bpf_register_is_null(val_reg) &&
> -		    map_kptr_match_type(env, kptr_field, val_reg, value_regno))
> +		if (bpf_register_is_null(val_reg)) {
> +			/*
> +			 * This store is valid only because the scalar is known to be
> +			 * zero. Mark it precise so another scalar cannot be pruned
> +			 * against this state.
> +			 */
> +			return mark_chain_precision(env, value_regno);
> +		}

[Severity: High]
This is a pre-existing issue, and was not introduced by the patch under 
review, but while this commit successfully fixes the precision-tracking 
bug in check_map_kptr_access(), does the exact same vulnerability still 
exist in several other locations where bpf_register_is_null() is used 
without calling mark_chain_precision()?

For example, in check_func_arg():

	if (bpf_register_is_null(reg) && type_may_be_null(arg_type))
		/* A NULL register has a SCALAR_VALUE type, so skip
		 * type checking.
		 */
		goto skip_type_check;

And also later in check_func_arg() for map values:

	case ARG_PTR_TO_MAP_VALUE:
		if (type_may_be_null(arg_type) && bpf_register_is_null(reg))
			return 0;

Similarly, in btf_check_func_arg_match():

			if (bpf_register_is_null(reg) && type_may_be_null(arg->arg_type))
				continue;

And in release_reg():

	if (bpf_register_is_null(reg))
		return 0;

And in check_kfunc_args() for nullable args:

		if (bpf_register_is_null(reg) && type_may_be_null(kf_arg_type))
			continue;

And in check_kfunc_args() for memory buffer lengths:

			if (bpf_register_is_null(buff_reg))
				break;

And in check_mem_reg():

	if (bpf_register_is_null(reg))
		return 0;

And in check_helper_call() for get_local_storage():

		if (!bpf_register_is_null(&regs[BPF_REG_2])) {
			verbose(env, "get_local_storage() doesn't support non-zero flags\n");
			return -EINVAL;
		}
		break;

Since mark_chain_precision() is omitted in these bypass paths, the scalar 0 
remains imprecise. Could an attacker pass an attacker-controlled arbitrary 
scalar on a second pruned path, which would then be incorrectly accepted by 
the verifier when substituted at runtime?

> +		if (map_kptr_match_type(env, kptr_field, val_reg, value_regno))
>  			return -EACCES;
>  	} else if (class == BPF_ST) {
>  		if (insn->imm) {

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