linux-kernel - [PATCH bpf/for-next] cgroup: Make current_cgns_cgroup_dfl() safe to call after exit_task

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 14 Mar 2023 11:59:49 -1000
From:   Tejun Heo <tj@...nel.org>
To:     Alexei Starovoitov <ast@...nel.org>
Cc:     bpf@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel-team@...a.com
Subject: [PATCH bpf/for-next] cgroup: Make current_cgns_cgroup_dfl() safe to
 call after exit_task_namespace()

332ea1f697be ("bpf: Add bpf_cgroup_from_id() kfunc") added
bpf_cgroup_from_id() which calls current_cgns_cgroup_dfl() through
cgroup_get_from_id(). However, BPF programs may be attached to a point where
current->nsproxy has already been cleared to NULL by exit_task_namespace()
and calling bpf_cgroup_from_id() would cause an oops.

Just return the system-wide root if nsproxy has been cleared. This allows
all cgroups to be looked up after the task passed through
exit_task_namespace(), which semantically makes sense. Given that the only
way to get this behavior is through BPF programs, it seems safe but let's
see what others think.

Signed-off-by: Tejun Heo <tj@...nel.org>
Cc: Alexei Starovoitov <ast@...nel.org>
Fixes: 332ea1f697be ("bpf: Add bpf_cgroup_from_id() kfunc")
---
Hello,

Given that this is only exposed through a commit in bpf/for-next, I think
it'd be best to route it together. If this looks okay, please apply to
bpf/for-next.

Thanks.

 kernel/cgroup/cgroup.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 8bfb0e2a87ee..c0da5cb9f193 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -1465,8 +1465,20 @@ static struct cgroup *current_cgns_cgroup_dfl(void)
 {
 	struct css_set *cset;

-	cset = current->nsproxy->cgroup_ns->root_cset;
-	return __cset_cgroup_from_root(cset, &cgrp_dfl_root);
+	if (current->nsproxy) {
+		cset = current->nsproxy->cgroup_ns->root_cset;
+		return __cset_cgroup_from_root(cset, &cgrp_dfl_root);
+	} else {
+		/*
+		 * NOTE: This function may be called from bpf_cgroup_from_id()
+		 * on a task which has already passed exit_task_namespace() and
+		 * NULL nsproxy. Fall back to cgrp_dfl_root which will make all
+		 * cgroups visible for lookups. Given that BPF progs are
+		 * privileged, this shouldn't create security concerns but there
+		 * may be a better way to handle this.
+		 */
+		return &cgrp_dfl_root.cgrp;
+	}
 }

 /* look up cgroup associated with given css_set on the specified hierarchy */
-- 
2.39.2