From bde2607563678de28d6cf16e14e7d40bf3fb895a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 16 Mar 2022 14:29:49 +0100 Subject: [PATCH 1/4] cgroup-show: split out delegation xattr check into its own function Just some refactoring. --- src/shared/cgroup-show.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/src/shared/cgroup-show.c b/src/shared/cgroup-show.c index f18420c1b6..d2fb17ff5d 100644 --- a/src/shared/cgroup-show.c +++ b/src/shared/cgroup-show.c @@ -128,6 +128,27 @@ static int show_cgroup_one_by_path( return 0; } +static int is_delegated(int cgfd, const char *path) { + _cleanup_free_ char *b = NULL; + int r; + + assert(cgfd >= 0 || path); + + r = getxattr_malloc(cgfd < 0 ? path : FORMAT_PROC_FD_PATH(cgfd), "trusted.delegate", &b); + if (r < 0) { + if (r == -ENODATA) + return false; + + return log_debug_errno(r, "Failed to read trusted.delegate extended attribute, 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 r; +} + static int show_cgroup_name( const char *path, const char *prefix, @@ -137,7 +158,7 @@ static int show_cgroup_name( uint64_t cgroupid = UINT64_MAX; _cleanup_free_ char *b = NULL; _cleanup_close_ int fd = -1; - bool delegate = false; + bool delegate; int r; if (FLAGS_SET(flags, OUTPUT_CGROUP_XATTRS) || FLAGS_SET(flags, OUTPUT_CGROUP_ID)) { @@ -146,19 +167,7 @@ static int show_cgroup_name( log_debug_errno(errno, "Failed to open cgroup '%s', ignoring: %m", path); } - r = getxattr_malloc(fd < 0 ? path : FORMAT_PROC_FD_PATH(fd), "trusted.delegate", &b); - if (r < 0) { - if (r != -ENODATA) - log_debug_errno(r, "Failed to read trusted.delegate extended attribute, ignoring: %m"); - } else { - r = parse_boolean(b); - if (r < 0) - log_debug_errno(r, "Failed to parse trusted.delegate extended attribute boolean value, ignoring: %m"); - else - delegate = r > 0; - - b = mfree(b); - } + delegate = is_delegated(fd, path) > 0; if (FLAGS_SET(flags, OUTPUT_CGROUP_ID)) { cg_file_handle fh = CG_FILE_HANDLE_INIT; From d9bc1c36141e247d5f78eaeefeab92e9302449fc Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 16 Mar 2022 14:58:57 +0100 Subject: [PATCH 2/4] cgroup: also indicate cgroup delegation state in user-accessible xattr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/core/cgroup.c | 32 +++++++++++++++++++++----------- src/shared/cgroup-show.c | 12 ++++++++---- 2 files changed, 29 insertions(+), 15 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index f2044c2ffc..1780d7ca27 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 d2fb17ff5d..fc1e631464 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; } From 200aa3583f0e29f30057f6b3e4c423323ad915a6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 16 Mar 2022 16:31:24 +0100 Subject: [PATCH 3/4] docs: document the user.delegate xattr --- docs/CGROUP_DELEGATION.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/CGROUP_DELEGATION.md b/docs/CGROUP_DELEGATION.md index 926d16eef6..8b14acf4eb 100644 --- a/docs/CGROUP_DELEGATION.md +++ b/docs/CGROUP_DELEGATION.md @@ -253,6 +253,13 @@ So, if you want to do your own raw cgroups kernel level access, then allocate a scope unit, or a service unit (or just use the service unit you already have for your service code), and turn on delegation for it. +The service manager sets the `user.delegate` extended attribute (readable via +`getxattr(2)` and related calls) to the character `1` on cgroup directories +where delegation is enabled (and removes it on those cgroups where it is +not). This may be used by service programs to determine whether a cgroup tree +was delegated to them. Note that this is only supported on kernels 5.6 and +newer in combination with systemd 251 and newer. + (OK, here's one caveat: if you turn on delegation for a service, and that service has `ExecStartPost=`, `ExecReload=`, `ExecStop=` or `ExecStopPost=` set, then these commands will be executed within the `.control/` sub-cgroup of From 1fa3b6c2479979fcb495bb9a208a63010d8d205e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 16 Mar 2022 16:47:07 +0100 Subject: [PATCH 4/4] cgroup: also set user.invocation_id in addition to trusted.invocation_id Similar thinking as the preceeding commit. (While we are at it, let's unify some code we use over and over again in two helper functions) --- src/core/cgroup.c | 92 +++++++++++++++++++++++++++-------------------- 1 file changed, 54 insertions(+), 38 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 1780d7ca27..98bf5e8db7 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -736,9 +736,44 @@ int cgroup_add_bpf_foreign_program(CGroupContext *c, uint32_t attach_type, const UNIT_DEFINE_ANCESTOR_MEMORY_LOOKUP(memory_low); UNIT_DEFINE_ANCESTOR_MEMORY_LOOKUP(memory_min); +static void unit_set_xattr_graceful(Unit *u, const char *cgroup_path, const char *name, const void *data, size_t size) { + int r; + + assert(u); + assert(name); + + if (!cgroup_path) { + if (!u->cgroup_path) + return; + + cgroup_path = u->cgroup_path; + } + + r = cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, cgroup_path, name, data, size, 0); + if (r < 0) + log_unit_debug_errno(u, r, "Failed to set '%s' xattr on control group %s, ignoring: %m", name, empty_to_root(cgroup_path)); +} + +static void unit_remove_xattr_graceful(Unit *u, const char *cgroup_path, const char *name) { + int r; + + assert(u); + assert(name); + + if (!cgroup_path) { + if (!u->cgroup_path) + return; + + cgroup_path = u->cgroup_path; + } + + r = cg_remove_xattr(SYSTEMD_CGROUP_CONTROLLER, cgroup_path, name); + if (r < 0 && r != -ENODATA) + log_unit_debug_errno(u, r, "Failed to remove '%s' xattr flag on control group %s, ignoring: %m", name, empty_to_root(cgroup_path)); +} + void cgroup_oomd_xattr_apply(Unit *u, const char *cgroup_path) { CGroupContext *c; - int r; assert(u); @@ -746,48 +781,34 @@ void cgroup_oomd_xattr_apply(Unit *u, const char *cgroup_path) { if (!c) return; - if (c->moom_preference == MANAGED_OOM_PREFERENCE_OMIT) { - r = cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, cgroup_path, "user.oomd_omit", "1", 1, 0); - if (r < 0) - log_unit_debug_errno(u, r, "Failed to set oomd_omit flag on control group %s, ignoring: %m", empty_to_root(cgroup_path)); - } + if (c->moom_preference == MANAGED_OOM_PREFERENCE_OMIT) + unit_set_xattr_graceful(u, cgroup_path, "user.oomd_omit", "1", 1); - if (c->moom_preference == MANAGED_OOM_PREFERENCE_AVOID) { - r = cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, cgroup_path, "user.oomd_avoid", "1", 1, 0); - if (r < 0) - log_unit_debug_errno(u, r, "Failed to set oomd_avoid flag on control group %s, ignoring: %m", empty_to_root(cgroup_path)); - } + if (c->moom_preference == MANAGED_OOM_PREFERENCE_AVOID) + unit_set_xattr_graceful(u, cgroup_path, "user.oomd_avoid", "1", 1); - if (c->moom_preference != MANAGED_OOM_PREFERENCE_AVOID) { - r = cg_remove_xattr(SYSTEMD_CGROUP_CONTROLLER, cgroup_path, "user.oomd_avoid"); - if (r < 0 && r != -ENODATA) - log_unit_debug_errno(u, r, "Failed to remove oomd_avoid flag on control group %s, ignoring: %m", empty_to_root(cgroup_path)); - } + if (c->moom_preference != MANAGED_OOM_PREFERENCE_AVOID) + unit_remove_xattr_graceful(u, cgroup_path, "user.oomd_avoid"); - if (c->moom_preference != MANAGED_OOM_PREFERENCE_OMIT) { - r = cg_remove_xattr(SYSTEMD_CGROUP_CONTROLLER, cgroup_path, "user.oomd_omit"); - if (r < 0 && r != -ENODATA) - log_unit_debug_errno(u, r, "Failed to remove oomd_omit flag on control group %s, ignoring: %m", empty_to_root(cgroup_path)); - } + if (c->moom_preference != MANAGED_OOM_PREFERENCE_OMIT) + unit_remove_xattr_graceful(u, cgroup_path, "user.oomd_omit"); } static void cgroup_xattr_apply(Unit *u) { const char *xn; bool b; - int r; assert(u); if (!MANAGER_IS_SYSTEM(u->manager)) return; - if (!sd_id128_is_null(u->invocation_id)) { - r = cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, - "trusted.invocation_id", - SD_ID128_TO_STRING(u->invocation_id), 32, - 0); - if (r < 0) - log_unit_debug_errno(u, r, "Failed to set invocation ID on control group %s, ignoring: %m", empty_to_root(u->cgroup_path)); + b = !sd_id128_is_null(u->invocation_id); + FOREACH_STRING(xn, "trusted.invocation_id", "user.invocation_id") { + if (b) + unit_set_xattr_graceful(u, NULL, xn, SD_ID128_TO_STRING(u->invocation_id), 32); + else + unit_remove_xattr_graceful(u, NULL, xn); } /* Indicate on the cgroup whether delegation is on, via an xattr. This is best-effort, as old kernels @@ -800,15 +821,10 @@ static void cgroup_xattr_apply(Unit *u) { * 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)); - } + if (b) + unit_set_xattr_graceful(u, NULL, xn, "1", 1); + else + unit_remove_xattr_graceful(u, NULL, xn); } cgroup_oomd_xattr_apply(u, u->cgroup_path);