Re: [PATCH bpf 1/2] bpf: Add return value check for BPF_LSM_CGROUP

[sashiko-bot@kernel.org]

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

New issues:
- [High] The bounds validation for bpf_set_retval() can be bypassed by passing a pointer instead of a scalar, leading to the exact kernel panic the patch attempts to fix.

Pre-existing issues:
- [High] Bounds enforcement is strictly restricted to BPF_PROG_TYPE_LSM, completely neglecting other vulnerable cgroup program types like BPF_PROG_TYPE_CGROUP_DEVICE.
--

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

bpf: Add return value check for BPF_LSM_CGROUP

This commit adds validation for the return value set by the bpf_set_retval()
helper in BPF_LSM_CGROUP programs. It verifies that the provided argument
falls within the valid return value range for the target hook to prevent
kernel panics.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 7fb88e1cd7c4d..fe60a695de558 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -10462,6 +10462,9 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  	case BPF_FUNC_set_retval:
>  		if (prog_type == BPF_PROG_TYPE_LSM &&
>  		    env->prog->expected_attach_type == BPF_LSM_CGROUP) {

[Severity: High]
This is a pre-existing issue, but does restricting this check to
BPF_PROG_TYPE_LSM leave other cgroup program types vulnerable?

Programs like BPF_PROG_TYPE_CGROUP_DEVICE can also call bpf_set_retval()
with arbitrary positive values. If such a program returns 1 (allow)
after setting a positive return value like 1234, the value propagates
through __cgroup_bpf_check_dev_permission() and bdev_permission(), causing
bdev_file_open_by_dev() to return ERR_PTR(1234).

Since 1234 is not a valid negative error code, IS_ERR() would return false,
potentially leading to a kernel panic when the caller attempts to
dereference 1234 as a file pointer.

Should we extend this validation to all cgroup program types?

> +			struct bpf_retval_range range;
> +			struct bpf_reg_state *r1 = &regs[BPF_REG_1];
> +
>  			if (!env->prog->aux->attach_func_proto->type) {
>  				/* Make sure programs that attach to void
>  				 * hooks don't try to modify return value.
> @@ -10469,6 +10472,13 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  				verbose(env, "BPF_LSM_CGROUP that attach to void LSM hooks can't modify return value!\n");
>  				return -EINVAL;
>  			}
> +
> +			bpf_lsm_get_retval_range(env->prog, &range);
> +			range.return_32bit = true;
> +			if (!retval_range_within(range, r1)) {

[Severity: High]
Can this bounds check be bypassed if a pointer is passed instead of a scalar?

Because bpf_set_retval_proto uses ARG_ANYTHING, check_func_arg() allows
pointer registers when env->allow_ptr_leaks is true (which is the case
with CAP_BPF).

If a pointer like PTR_TO_STACK is passed without a variable offset,
its s32_min_value and s32_max_value are both 0. Since 0 falls within the
valid [-MAX_ERRNO, 0] range, retval_range_within() will allow it.

At runtime, the 64-bit pointer address would be truncated to a 32-bit integer,
potentially yielding an arbitrary positive return value and causing the exact
kernel panic this patch intends to fix.

Should the verifier explicitly ensure that R1 is a SCALAR_VALUE before
validating its range?

> +				verbose_invalid_scalar(env, r1, range, "At bpf_set_retval", "R1");
> +				return -EINVAL;
> +			}
>  		}
>  		break;
>  	case BPF_FUNC_dynptr_data:

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