From: Lennart Poettering <lennart@poettering.net>
Date: Wed, 16 Mar 2022 13:58:57 +0000 (+0100)
Subject: cgroup: also indicate cgroup delegation state in user-accessible xattr
X-Git-Tag: v251-rc1~135^2~2
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=d9bc1c36141e247d5f78eaeefeab92e9302449fc;p=thirdparty%2Fsystemd.git

cgroup: also indicate cgroup delegation state in user-accessible xattr

So far we set the "trusted.delegate" xattr on cgroups where delegation
is on. This duplicates this behaviour with the "user.delegate" xattr.
This has two benefits:

1. unprivileged clients can *read* the xattr. "systemd-cgls" can thus
   show delegated cgroups as such properly, even when invoked without
   privs

2. unprivileged systemd instances can set the xattr, i.e. when systemd
   --user delegates a cgroup to further payloads.

This weakens security a tiny bit, given that code that got a cgroup
delegated can manipulate the xattr, but I think that's OK, given they
have a higher trust level regarding cgroups anyway, if they got a
subtree delegated, and access controls on the cgroup itself are still
enforced. Moreover PID 1 as the cgroup manager only sets these xattrs,
never reads them — the xattr is primarily a way to tell payloads about
the delegation, and it's strictly this one way.
---

diff --git a/src/core/cgroup.c b/src/core/cgroup.c
index f2044c2ffc0..1780d7ca279 100644
--- a/src/core/cgroup.c
+++ b/src/core/cgroup.c
@@ -772,6 +772,8 @@ void cgroup_oomd_xattr_apply(Unit *u, const char *cgroup_path) {
 }
 
 static void cgroup_xattr_apply(Unit *u) {
+        const char *xn;
+        bool b;
         int r;
 
         assert(u);
@@ -788,17 +790,25 @@ static void cgroup_xattr_apply(Unit *u) {
                         log_unit_debug_errno(u, r, "Failed to set invocation ID on control group %s, ignoring: %m", empty_to_root(u->cgroup_path));
         }
 
-        if (unit_cgroup_delegate(u)) {
-                r = cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path,
-                                 "trusted.delegate",
-                                 "1", 1,
-                                 0);
-                if (r < 0)
-                        log_unit_debug_errno(u, r, "Failed to set delegate flag on control group %s, ignoring: %m", empty_to_root(u->cgroup_path));
-        } else {
-                r = cg_remove_xattr(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, "trusted.delegate");
-                if (r < 0 && r != -ENODATA)
-                        log_unit_debug_errno(u, r, "Failed to remove delegate flag on control group %s, ignoring: %m", empty_to_root(u->cgroup_path));
+        /* Indicate on the cgroup whether delegation is on, via an xattr. This is best-effort, as old kernels
+         * didn't support xattrs on cgroups at all. Later they got support for setting 'trusted.*' xattrs,
+         * and even later 'user.*' xattrs. We started setting this field when 'trusted.*' was added, and
+         * given this is now pretty much API, let's continue to support that. But also set 'user.*' as well,
+         * since it is readable by any user, not just CAP_SYS_ADMIN. This hence comes with slightly weaker
+         * security (as users who got delegated cgroups could turn it off if they like), but this shouldn't
+         * be a big problem given this communicates delegation state to clients, but the manager never reads
+         * it. */
+        b = unit_cgroup_delegate(u);
+        FOREACH_STRING(xn, "trusted.delegate", "user.delegate") {
+                if (b) {
+                        r = cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, xn, "1", 1, 0);
+                        if (r < 0)
+                                log_unit_debug_errno(u, r, "Failed to set '%s' xattr on control group %s, ignoring: %m", xn, empty_to_root(u->cgroup_path));
+                } else {
+                        r = cg_remove_xattr(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, xn);
+                        if (r < 0 && r != -ENODATA)
+                                log_unit_debug_errno(u, r, "Failed to remove '%s' xattr flag on control group %s, ignoring: %m", xn, empty_to_root(u->cgroup_path));
+                }
         }
 
         cgroup_oomd_xattr_apply(u, u->cgroup_path);
diff --git a/src/shared/cgroup-show.c b/src/shared/cgroup-show.c
index d2fb17ff5d3..fc1e6314649 100644
--- a/src/shared/cgroup-show.c
+++ b/src/shared/cgroup-show.c
@@ -135,16 +135,20 @@ static int is_delegated(int cgfd, const char *path) {
         assert(cgfd >= 0 || path);
 
         r = getxattr_malloc(cgfd < 0 ? path : FORMAT_PROC_FD_PATH(cgfd), "trusted.delegate", &b);
-        if (r < 0) {
+        if (r == -ENODATA) {
+                /* If the trusted xattr isn't set (preferred), then check the untrusted one. Under the
+                 * assumption that whoever is trusted enough to own the cgroup, is also trusted enough to
+                 * decide if it is delegated or not this should be safe. */
+                r = getxattr_malloc(cgfd < 0 ? path : FORMAT_PROC_FD_PATH(cgfd), "user.delegate", &b);
                 if (r == -ENODATA)
                         return false;
-
-                return log_debug_errno(r, "Failed to read trusted.delegate extended attribute, ignoring: %m");
         }
+        if (r < 0)
+                return log_debug_errno(r, "Failed to read delegate xattr, ignoring: %m");
 
         r = parse_boolean(b);
         if (r < 0)
-                return log_debug_errno(r, "Failed to parse trusted.delegate extended attribute boolean value, ignoring: %m");
+                return log_debug_errno(r, "Failed to parse delegate xattr boolean value, ignoring: %m");
 
         return r;
 }