From 5954c07433b134694256b9989f2ad3f85a643976 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Apr 2013 19:15:30 -0300 Subject: cgroup: do not allow manipulating the cgroup path of units within the systemd:/system subtree --- TODO | 7 ++++--- src/core/unit.c | 35 ++++++++++++++++++++--------------- src/shared/cgroup-util.c | 18 ++++++++++++++---- src/shared/cgroup-util.h | 15 +++++++++++++++ 4 files changed, 53 insertions(+), 22 deletions(-) diff --git a/TODO b/TODO index bb1802813f..1a35703d1a 100644 --- a/TODO +++ b/TODO @@ -26,6 +26,10 @@ Fedora 19: Features: +* handle named vs controller hierarchies correctly in cg_pid_get_path() + +* add nspawn@.service + * investigate endianess issues of UUID vs. GUID * see if we can fix https://bugs.freedesktop.org/show_bug.cgi?id=63672 @@ -57,9 +61,6 @@ Features: * make sure cg_pid_get_path() works properly for co-mounted controllers -* explicitly disallow changing the cgroup path of units in the - name=systemd hierarchy, unless it is outside of /system - * test/: - add 'set -e' to scripts in test/ - make stuff in test/ work with separate output dir diff --git a/src/core/unit.c b/src/core/unit.c index 282852fed3..c0f156c928 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -1977,10 +1977,16 @@ static int unit_add_cgroup(Unit *u, CGroupBonding *b) { } char *unit_default_cgroup_path(Unit *u) { + _cleanup_free_ char *escaped_instance = NULL; + assert(u); + escaped_instance = cg_escape(u->id); + if (!escaped_instance) + return NULL; + if (u->instance) { - _cleanup_free_ char *t = NULL, *escaped_template = NULL, *escaped_instance = NULL; + _cleanup_free_ char *t = NULL, *escaped_template = NULL; t = unit_name_template(u->id); if (!t) @@ -1990,20 +1996,9 @@ char *unit_default_cgroup_path(Unit *u) { if (!escaped_template) return NULL; - escaped_instance = cg_escape(u->id); - if (!escaped_instance) - return NULL; - return strjoin(u->manager->cgroup_hierarchy, "/", escaped_template, "/", escaped_instance, NULL); - } else { - _cleanup_free_ char *escaped = NULL; - - escaped = cg_escape(u->id); - if (!escaped) - return NULL; - - return strjoin(u->manager->cgroup_hierarchy, "/", escaped, NULL); - } + } else + return strjoin(u->manager->cgroup_hierarchy, "/", escaped_instance, NULL); } int unit_add_cgroup_from_text(Unit *u, const char *name, bool overwrite, CGroupBonding **ret) { @@ -2025,7 +2020,7 @@ int unit_add_cgroup_from_text(Unit *u, const char *name, bool overwrite, CGroupB } if (!controller) { - controller = strdup(SYSTEMD_CGROUP_CONTROLLER); + controller = strdup("systemd"); ours = true; } @@ -2035,6 +2030,16 @@ int unit_add_cgroup_from_text(Unit *u, const char *name, bool overwrite, CGroupB return log_oom(); } + if (streq(controller, "systemd")) { + /* Within the systemd unit hierarchy we do not allow changes. */ + if (path_startswith(path, "/system")) { + log_warning_unit(u->id, "Manipulating the systemd:/system cgroup hierarchy is not permitted."); + free(path); + free(controller); + return -EPERM; + } + } + b = cgroup_bonding_find_list(u->cgroup_bondings, controller); if (b) { if (streq(path, b->path)) { diff --git a/src/shared/cgroup-util.c b/src/shared/cgroup-util.c index 46a8128eb4..016080f65b 100644 --- a/src/shared/cgroup-util.c +++ b/src/shared/cgroup-util.c @@ -916,6 +916,7 @@ int cg_is_empty_recursive(const char *controller, const char *path, bool ignore_ int cg_split_spec(const char *spec, char **controller, char **path) { const char *e; char *t = NULL, *u = NULL; + _cleanup_free_ char *v = NULL; assert(spec); @@ -928,6 +929,7 @@ int cg_split_spec(const char *spec, char **controller, char **path) { if (!t) return -ENOMEM; + path_kill_slashes(t); *path = t; } @@ -943,7 +945,7 @@ int cg_split_spec(const char *spec, char **controller, char **path) { return -EINVAL; if (controller) { - t = strdup(spec); + t = strdup(normalize_controller(spec)); if (!t) return -ENOMEM; @@ -956,7 +958,10 @@ int cg_split_spec(const char *spec, char **controller, char **path) { return 0; } - t = strndup(spec, e-spec); + v = strndup(spec, e-spec); + if (!v) + return -ENOMEM; + t = strdup(normalize_controller(v)); if (!t) return -ENOMEM; if (!cg_controller_is_valid(t, true)) { @@ -969,12 +974,15 @@ int cg_split_spec(const char *spec, char **controller, char **path) { free(t); return -ENOMEM; } - if (!path_is_safe(u)) { + if (!path_is_safe(u) || + !path_is_absolute(u)) { free(t); free(u); return -EINVAL; } + path_kill_slashes(u); + if (controller) *controller = t; else @@ -993,7 +1001,6 @@ int cg_join_spec(const char *controller, const char *path, char **spec) { assert(path); - if (!controller) controller = "systemd"; else { @@ -1010,6 +1017,8 @@ int cg_join_spec(const char *controller, const char *path, char **spec) { if (!s) return -ENOMEM; + path_kill_slashes(s + strlen(controller) + 1); + *spec = s; return 0; } @@ -1029,6 +1038,7 @@ int cg_mangle_path(const char *path, char **result) { if (!t) return -ENOMEM; + path_kill_slashes(t); *result = t; return 0; } diff --git a/src/shared/cgroup-util.h b/src/shared/cgroup-util.h index a2ee72d67f..7bd02c1008 100644 --- a/src/shared/cgroup-util.h +++ b/src/shared/cgroup-util.h @@ -28,6 +28,21 @@ #include "set.h" #include "def.h" +/* + * General rules: + * + * We accept named hierarchies in the syntax "foo" and "name=foo". + * + * We expect that named hierarchies do not conflict in name with a + * kernel hierarchy, modulo the "name=" prefix. + * + * We always generate "normalized" controller names, i.e. without the + * "name=" prefix. + * + * We require absolute cgroup paths. When returning, we will always + * generate paths with multiple adjacent / removed. + */ + int cg_enumerate_processes(const char *controller, const char *path, FILE **_f); int cg_enumerate_tasks(const char *controller, const char *path, FILE **_f); int cg_read_pid(FILE *f, pid_t *_pid); -- cgit v1.2.3-54-g00ecf