Re: [PATCH bpf-next v5 2/3] bpf: Add validation for bpf_set_retval argument

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The verifier incorrectly refines the bounds of `bpf_get_retval()` for boolean LSM hooks to [0, 1], but at runtime the returned value can be -EPERM (-1), leading to a register bounds violation and arbitrary out-of-bounds memory access.
--

commit 2c0b94558327f28e9ce1f11e3518238475fc93d6
Author: Xu Kuohai <xukuohai@huawei.com>

bpf: Add validation for bpf_set_retval argument

This commit adds argument validation for the bpf_set_retval() helper to
prevent positive return values in cgroup hooks that expect negative error
codes. It also refines the return value bounds of bpf_get_retval() so it
can be passed directly to bpf_set_retval() without extra manual bounds
checking.

Link: https://lore.kernel.org/all/567d3206-74a5-44e5-99c6-779c425f399e@std.uestc.edu.cn

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 8ed484cb1a8a4..a50280bb0d453 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -9790,6 +9792,29 @@ static int do_refine_retval_range(struct bpf_verifier_env *env,
>  		reg_set_urange32(ret_reg, 0, nr_cpu_ids - 1);
>  		reg_bounds_sync(ret_reg);
>  		break;
> +	case BPF_FUNC_get_retval:
> +		/*
> +		 * bpf_get_retval may see arbitrary value passed by bpf_prog_run_array_cg for
> +		 * CGROUP_GETSOCKOPT type.
> +		 */
> +		if (prog_type == BPF_PROG_TYPE_CGROUP_SOCKOPT &&
> +		    env->prog->expected_attach_type == BPF_CGROUP_GETSOCKOPT)
> +			break;
> +
> +		if (prog_type == BPF_PROG_TYPE_LSM &&
> +		    env->prog->expected_attach_type == BPF_LSM_CGROUP) {
> +			if (!env->prog->aux->attach_func_proto->type)
> +				break;
> +			bpf_lsm_get_retval_range(env->prog, &range);

[Severity: Critical]
Does this cause a verifier bounds violation when multiple programs are
attached to a boolean LSM hook?

When do_refine_retval_range() calls bpf_lsm_get_retval_range() for a boolean
LSM hook, it strictly refines the register bounds to [0, 1].

However, if multiple programs are attached to the same cgroup hook via
BPF_F_ALLOW_MULTI, they run sequentially in bpf_prog_run_array_cg().
If the first program returns 0 (which is within the [0, 1] bounds),
bpf_prog_run_array_cg() translates this into an error code:

kernel/bpf/cgroup.c:bpf_prog_run_array_cg() {
    ...
    if (!func_ret && !IS_ERR_VALUE((long)run_ctx.retval))
        run_ctx.retval = -EPERM;
    ...
}

When the next BPF program in the array executes and calls bpf_get_retval(),
it receives the current run_ctx.retval, which is now -EPERM (-1).

Since the verifier statically assumes bpf_get_retval() will only return
0 or 1, will receiving -1 at runtime allow an attacker to bypass verifier
checks and perform out-of-bounds memory accesses?

> +		} else {
> +			range.minval = -MAX_ERRNO;
> +			range.maxval = 0;
> +		}
> +
> +		reg_set_srange64(ret_reg, range.minval, range.maxval);
> +		reg_set_srange32(ret_reg, range.minval, range.maxval);
> +		reg_bounds_sync(ret_reg);
> +		break;
>  	}
>  
>  	return reg_bounds_sanity_check(env, ret_reg, "retval");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605140243.664590-1-xukuohai@huaweicloud.com?part=2