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

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

[sashiko-bot]

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

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

[Xu Kuohai]

On 6/10/2026 8:45 PM, sashiko-bot@kernel.org wrote:
> 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?
>

The key problem is that, boolean hooks do not have deny/allow semantics.

There are 3 boolean hooks, each has its own meaning of the return value:
1. inode_xattr_skipcap:  1 is "skip capability check", 0 is "not skip"
2. audit_rule_known:  1 is "rule is known", 0 is "rule not known"
3. xfrm_state_pol_flow_match: 1 is "match", 0 is "not match"

None of them has deny/allow semantics.

So for a boolean hook, we do not know whether 0 or 1 is the correct value
to translate the return value to when bpf prog exits with 0. I prefer to
not translate it at all, just leave the return value untouched.

>>   		item++;
>>   	}
> 

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

[Xu Kuohai]

This series fixes the issue reported by sashiko in [1]. The issue is that,
when a cgroup BPF program exits with 0, bpf_prog_run_array_cg() sets
the hook return value to -EPERM if it is not a valid errno. This is
correct for errno-based hooks, which return 0 on success and negative
errno on failure, but wrong for void and boolean LSM hooks. Boolean
LSM hooks should only return true or false, and void LSM hooks have
no return value at all.

Fix it by skipping setting -EPERM for hooks not returning errno.

[1] https://lore.kernel.org/bpf/20260605144232.95A141F00893@smtp.kernel.org/

Xu Kuohai (2):
  bpf: Fix setting retval to -EPERM for cgroup hooks not returning errno
  selftests/bpf: Add retval test for bool and errno LSM cgroup hooks

 include/linux/bpf_lsm.h                       |  6 ++
 kernel/bpf/bpf_lsm.c                          | 20 +++++
 kernel/bpf/cgroup.c                           | 47 ++++++++---
 .../selftests/bpf/prog_tests/lsm_cgroup.c     | 79 +++++++++++++++++++
 .../testing/selftests/bpf/progs/lsm_cgroup.c  | 30 +++++++
 5 files changed, 169 insertions(+), 13 deletions(-)

-- 
2.43.0

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

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

When a cgroup BPF program exits with 0, bpf_prog_run_array_cg() sets
the hook return value to -EPERM if it is not a valid errno. This is
correct for errno-based hooks, which return 0 on success and negative
errno on failure, but wrong for boolean and void LSM hooks. Boolean
LSM hooks should only return true or false, and void LSM hooks have
no return value at all.

Fix it by skipping setting -EPERM for hooks not returning errno.

Fixes: 69fd337a975c ("bpf: per-cgroup lsm flavor")
Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 include/linux/bpf_lsm.h |  6 ++++++
 kernel/bpf/bpf_lsm.c    | 20 ++++++++++++++++++
 kernel/bpf/cgroup.c     | 47 +++++++++++++++++++++++++++++------------
 3 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
index 643809cc78c3..143775a27a2a 100644
--- a/include/linux/bpf_lsm.h
+++ b/include/linux/bpf_lsm.h
@@ -52,6 +52,7 @@ int bpf_set_dentry_xattr_locked(struct dentry *dentry, const char *name__str,
 				const struct bpf_dynptr *value_p, int flags);
 int bpf_remove_dentry_xattr_locked(struct dentry *dentry, const char *name__str);
 bool bpf_lsm_has_d_inode_locked(const struct bpf_prog *prog);
+bool bpf_lsm_hook_returns_errno(u32 btf_id);
 
 #else /* !CONFIG_BPF_LSM */
 
@@ -104,6 +105,11 @@ static inline bool bpf_lsm_has_d_inode_locked(const struct bpf_prog *prog)
 {
 	return false;
 }
+
+static inline bool bpf_lsm_hook_returns_errno(u32 btf_id)
+{
+	return true;
+}
 #endif /* CONFIG_BPF_LSM */
 
 #endif /* _LINUX_BPF_LSM_H */
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index c5c925f00202..564071a92d7d 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -427,6 +427,26 @@ BTF_ID(func, bpf_lsm_audit_rule_known)
 BTF_ID(func, bpf_lsm_inode_xattr_skipcap)
 BTF_SET_END(bool_lsm_hooks)
 
+/* hooks returning void */
+#define LSM_HOOK_void(DEFAULT, NAME, ...) BTF_ID(func, bpf_lsm_##NAME)
+#define LSM_HOOK_int(DEFAULT, NAME, ...)  /* nothing */
+#define LSM_HOOK(RET, DEFAULT, NAME, ...) LSM_HOOK_##RET(DEFAULT, NAME, __VA_ARGS__)
+BTF_SET_START(void_lsm_hooks)
+#include <linux/lsm_hook_defs.h>
+#undef LSM_HOOK
+#undef LSM_HOOK_void
+#undef LSM_HOOK_int
+BTF_SET_END(void_lsm_hooks)
+
+bool bpf_lsm_hook_returns_errno(u32 btf_id)
+{
+	if (btf_id_set_contains(&bool_lsm_hooks, btf_id))
+		return false;
+	if (btf_id_set_contains(&void_lsm_hooks, btf_id))
+		return false;
+	return true;
+}
+
 int bpf_lsm_get_retval_range(const struct bpf_prog *prog,
 			     struct bpf_retval_range *retval_range)
 {
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 35d1f1428ef3..83ce66296ac1 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -55,6 +55,28 @@ void __init cgroup_bpf_lifetime_notifier_init(void)
 						&cgroup_bpf_lifetime_nb));
 }
 
+#ifdef CONFIG_BPF_LSM
+struct cgroup_lsm_atype {
+	u32 attach_btf_id;
+	int refcnt;
+	bool returns_errno;
+};
+
+static struct cgroup_lsm_atype cgroup_lsm_atype[CGROUP_LSM_NUM];
+
+static bool cgroup_bpf_hook_returns_errno(enum cgroup_bpf_attach_type atype)
+{
+	if (atype >= CGROUP_LSM_START && atype <= CGROUP_LSM_END)
+		return READ_ONCE(cgroup_lsm_atype[atype - CGROUP_LSM_START].returns_errno);
+	return true;
+}
+#else
+static bool cgroup_bpf_hook_returns_errno(enum cgroup_bpf_attach_type atype)
+{
+	return true;
+}
+#endif
+
 /* __always_inline is necessary to prevent indirect call through run_prog
  * function pointer.
  */
@@ -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;
 		item++;
 	}
@@ -156,13 +179,6 @@ unsigned int __cgroup_bpf_run_lsm_current(const void *ctx,
 }
 
 #ifdef CONFIG_BPF_LSM
-struct cgroup_lsm_atype {
-	u32 attach_btf_id;
-	int refcnt;
-};
-
-static struct cgroup_lsm_atype cgroup_lsm_atype[CGROUP_LSM_NUM];
-
 static enum cgroup_bpf_attach_type
 bpf_cgroup_atype_find(enum bpf_attach_type attach_type, u32 attach_btf_id)
 {
@@ -191,10 +207,13 @@ void bpf_cgroup_atype_get(u32 attach_btf_id, int cgroup_atype)
 
 	lockdep_assert_held(&cgroup_mutex);
 
-	WARN_ON_ONCE(cgroup_lsm_atype[i].attach_btf_id &&
-		     cgroup_lsm_atype[i].attach_btf_id != attach_btf_id);
-
-	cgroup_lsm_atype[i].attach_btf_id = attach_btf_id;
+	if (!cgroup_lsm_atype[i].attach_btf_id) {
+		cgroup_lsm_atype[i].attach_btf_id = attach_btf_id;
+		WRITE_ONCE(cgroup_lsm_atype[i].returns_errno,
+			   bpf_lsm_hook_returns_errno(attach_btf_id));
+	} else {
+		WARN_ON_ONCE(cgroup_lsm_atype[i].attach_btf_id != attach_btf_id);
+	}
 	cgroup_lsm_atype[i].refcnt++;
 }
 
@@ -203,8 +222,10 @@ void bpf_cgroup_atype_put(int cgroup_atype)
 	int i = cgroup_atype - CGROUP_LSM_START;
 
 	cgroup_lock();
-	if (--cgroup_lsm_atype[i].refcnt <= 0)
+	if (--cgroup_lsm_atype[i].refcnt <= 0) {
+		WRITE_ONCE(cgroup_lsm_atype[i].returns_errno, true);
 		cgroup_lsm_atype[i].attach_btf_id = 0;
+	}
 	WARN_ON_ONCE(cgroup_lsm_atype[i].refcnt < 0);
 	cgroup_unlock();
 }
-- 
2.43.0

[PATCH bpf-next 2/2] selftests/bpf: Add retval test for bool and errno LSM cgroup hooks

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

Add test to check the return value when a BPF program exits with 0 for
a boolean and an errno LSM hook.

For each hook, two BPF programs are attached. The first program returns
0 without calling bpf_set_retval() to exercise the return value translation
logic, while the second program reads the retval via bpf_get_retval().

Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 .../selftests/bpf/prog_tests/lsm_cgroup.c     | 79 +++++++++++++++++++
 .../testing/selftests/bpf/progs/lsm_cgroup.c  | 30 +++++++
 2 files changed, 109 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
index 6df25de8f080..41e867467f6c 100644
--- a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
+++ b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
@@ -2,6 +2,7 @@
 
 #include <sys/types.h>
 #include <sys/socket.h>
+#include <sys/xattr.h>
 #include <test_progs.h>
 #include <bpf/btf.h>
 
@@ -309,11 +310,89 @@ static void test_lsm_cgroup_nonvoid(void)
 	lsm_cgroup_nonvoid__destroy(skel);
 }
 
+static void test_lsm_cgroup_retval(void)
+{
+	struct lsm_cgroup *skel = NULL;
+	int skipcap_prog_fd1, skipcap_prog_fd2, socket_prog_fd1, socket_prog_fd2;
+	int cgroup_fd = -1;
+	int err, fd;
+	char tmpfile[] = "/tmp/test_lsm_cgroup_retval.XXXXXX";
+
+	fd = mkstemp(tmpfile);
+	if (!ASSERT_OK_FD(fd, "mkstemp"))
+		return;
+	close(fd);
+
+	cgroup_fd = test__join_cgroup("/default_retval");
+	if (!ASSERT_OK_FD(cgroup_fd, "join_cgroup"))
+		goto cleanup_tmpfile;
+
+	skel = lsm_cgroup__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open_and_load"))
+		goto cleanup_cgroup;
+
+	skipcap_prog_fd1 = bpf_program__fd(skel->progs.skipcap_first);
+	skipcap_prog_fd2 = bpf_program__fd(skel->progs.skipcap_second);
+	socket_prog_fd1 = bpf_program__fd(skel->progs.socket_first);
+	socket_prog_fd2 = bpf_program__fd(skel->progs.socket_second);
+
+	err = bpf_prog_attach(skipcap_prog_fd1, cgroup_fd, BPF_LSM_CGROUP, BPF_F_ALLOW_MULTI);
+	if (err == -ENOTSUPP) {
+		test__skip();
+		goto cleanup_skeleton;
+	}
+	if (!ASSERT_OK(err, "attach first skipcap prog"))
+		goto cleanup_skeleton;
+
+	err = bpf_prog_attach(skipcap_prog_fd2, cgroup_fd, BPF_LSM_CGROUP, BPF_F_ALLOW_MULTI);
+	if (!ASSERT_OK(err, "attach second skipcap prog"))
+		goto cleanup_skipcap1;
+
+	err = bpf_prog_attach(socket_prog_fd1, cgroup_fd, BPF_LSM_CGROUP, BPF_F_ALLOW_MULTI);
+	if (!ASSERT_OK(err, "attach first sock_create prog"))
+		goto cleanup_skipcap2;
+
+	err = bpf_prog_attach(socket_prog_fd2, cgroup_fd, BPF_LSM_CGROUP, BPF_F_ALLOW_MULTI);
+	if (!ASSERT_OK(err, "attach second sock_create prog"))
+		goto cleanup_sock_create1;
+
+	/* trigger the bool hook by setxattr */
+	err = setxattr(tmpfile, "user.test", "value", 5, 0);
+	if (!ASSERT_OK(err, "setxattr"))
+		goto cleanup_sock_create2;
+
+	/* trigger the errno hook by creating a socket */
+	fd = socket(AF_INET, SOCK_STREAM, 0);
+	if (!ASSERT_OK_FD(fd, "socket"))
+		goto cleanup_sock_create2;
+	close(fd);
+
+	ASSERT_EQ(skel->data->skipcap_retval, 0, "bool_hook_retval_should_be_0");
+	ASSERT_EQ(skel->data->socket_retval, -EPERM, "errno_hook_retval_should_be_EPERM");
+
+cleanup_sock_create2:
+	bpf_prog_detach2(socket_prog_fd2, cgroup_fd, BPF_LSM_CGROUP);
+cleanup_sock_create1:
+	bpf_prog_detach2(socket_prog_fd1, cgroup_fd, BPF_LSM_CGROUP);
+cleanup_skipcap2:
+	bpf_prog_detach2(skipcap_prog_fd2, cgroup_fd, BPF_LSM_CGROUP);
+cleanup_skipcap1:
+	bpf_prog_detach2(skipcap_prog_fd1, cgroup_fd, BPF_LSM_CGROUP);
+cleanup_skeleton:
+	lsm_cgroup__destroy(skel);
+cleanup_cgroup:
+	close(cgroup_fd);
+cleanup_tmpfile:
+	unlink(tmpfile);
+}
+
 void test_lsm_cgroup(void)
 {
 	if (test__start_subtest("functional"))
 		test_lsm_cgroup_functional();
 	if (test__start_subtest("nonvoid"))
 		test_lsm_cgroup_nonvoid();
+	if (test__start_subtest("retval"))
+		test_lsm_cgroup_retval();
 	btf__free(btf);
 }
diff --git a/tools/testing/selftests/bpf/progs/lsm_cgroup.c b/tools/testing/selftests/bpf/progs/lsm_cgroup.c
index d7598538aa2d..3bfa479104be 100644
--- a/tools/testing/selftests/bpf/progs/lsm_cgroup.c
+++ b/tools/testing/selftests/bpf/progs/lsm_cgroup.c
@@ -35,6 +35,8 @@ int called_socket_bind;
 int called_socket_bind2;
 int called_socket_alloc;
 int called_socket_clone;
+int skipcap_retval = -4095;
+int socket_retval = -4095;
 
 static __always_inline int test_local_storage(void)
 {
@@ -190,3 +192,31 @@ int BPF_PROG(socket_clone, struct sock *newsk, const struct request_sock *req)
 
 	return 1;
 }
+
+SEC("lsm_cgroup/inode_xattr_skipcap")
+int BPF_PROG(skipcap_first, const char *name)
+{
+	return 0;
+}
+
+SEC("lsm_cgroup/inode_xattr_skipcap")
+int BPF_PROG(skipcap_second, const char *name)
+{
+	skipcap_retval = bpf_get_retval();
+	bpf_set_retval(0);
+	return 1;
+}
+
+SEC("lsm_cgroup/socket_create")
+int BPF_PROG(socket_first, int family, int type, int protocol, int kern)
+{
+	return 0;
+}
+
+SEC("lsm_cgroup/socket_create")
+int BPF_PROG(socket_second, int family, int type, int protocol, int kern)
+{
+	socket_retval = bpf_get_retval();
+	bpf_set_retval(0);
+	return 1;
+}
-- 
2.43.0