Re: [PATCH bpf-next 1/2] bpf: Fix setting retval to -EPERM for cgroup hooks not returning errno

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch breaks cgroup BPF boolean LSM hooks by ignoring a BPF program's denial (exit code 0). It leaves `run_ctx.retval` unmodified for boolean hooks, which fails to enforce denials, ignores overrides from previous programs in the cgroup array, and causes the trampoline to incorrectly fall back to the original kernel function.
--

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

bpf: Fix setting retval to -EPERM for cgroup hooks not returning errno

This commit modifies bpf_prog_run_array_cg() to avoid setting the hook return
value to -EPERM when a cgroup BPF program exits with 0 for non-errno hooks
(such as boolean or void LSM hooks).

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 35d1f1428ef39..83ce66296ac19 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c

[ ... ]

> @@ -83,7 +105,8 @@ bpf_prog_run_array_cg(const struct cgroup_bpf *cgrp,
>  			*(ret_flags) |= (func_ret >> 1);
>  			func_ret &= 1;
>  		}
> -		if (!func_ret && !IS_ERR_VALUE((long)run_ctx.retval))
> +		if (!func_ret && cgroup_bpf_hook_returns_errno(atype) &&
> +		    !IS_ERR_VALUE((long)run_ctx.retval))
>  			run_ctx.retval = -EPERM;

[Severity: High]
Could this change cause BPF program denials to be ignored for boolean hooks?

In cgroup BPF, a program returning 0 typically signifies a denial. By
requiring cgroup_bpf_hook_returns_errno() to be true here, a func_ret of 0
is ignored for boolean hooks, leaving run_ctx.retval unmodified.

If a prior program in the cgroup array called bpf_set_retval(1), and the
current program returns 0 (denial), run_ctx.retval remains 1. Doesn't this
break cgroup hierarchy semantics where a denial must override an allow?

Additionally, if run_ctx.retval remains 0, the fmod_ret trampoline interprets
0 as an instruction to call the original kernel function rather than skipping
it. If the underlying kernel function returns 1, the operation ends up being
allowed. Does this unintentionally bypass the BPF program's intent to deny
the operation?

>  		item++;
>  	}

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