core: make RootDirectory= and ProtectKernelModules= work

Instead of having two fields inside BindMount struct where one is stack
based and the other one is heap, use one field to store the full path
and updated it when we chase symlinks. This way we avoid dealing with
both at the same time.

This makes RootDirectory= work with ProtectHome= and ProtectKernelModules=yes

Fixes: https://github.com/systemd/systemd/issues/4567

1 files changed, 86 insertions, 56 deletions

diff --git a/src/core/namespace.c b/src/core/namespace.c
index db9a7aa5e7..2dae579332 100644
--- a/src/core/namespace.c
+++ b/src/core/namespace.c
@@ -58,8 +58,7 @@ typedef enum MountMode {
 } MountMode;
 
 typedef struct BindMount {
-        const char *path; /* stack memory, doesn't need to be freed explicitly */
-        char *chased; /* malloc()ed memory, needs to be freed */
+        char *path;
         MountMode mode;
         bool ignore; /* Ignore if path does not exist */
 } BindMount;
@@ -155,12 +154,27 @@ static const TargetMount protect_system_strict_table[] = {
         { "/root",      READWRITE,      true  },      /* ProtectHome= */
 };
 
-static void set_bind_mount(BindMount **p, const char *path, MountMode mode, bool ignore) {
+static void set_bind_mount(BindMount **p, char *path, MountMode mode, bool ignore) {
         (*p)->path = path;
         (*p)->mode = mode;
         (*p)->ignore = ignore;
 }
 
+static int append_one_mount(BindMount **p, const char *root_directory,
+                            const char *path, MountMode mode, bool ignore) {
+        char *lpath;
+        assert(p);
+
+        lpath = prefix_root(root_directory, path);
+        if (!lpath)
+                return -ENOMEM;
+
+        set_bind_mount(p, lpath, mode, ignore);
+        (*p)++;
+
+        return 0;
+}
+
 static int append_mounts(BindMount **p, char **strv, MountMode mode) {
         char **i;
 
@@ -168,6 +182,7 @@ static int append_mounts(BindMount **p, char **strv, MountMode mode) {
 
         STRV_FOREACH(i, strv) {
                 bool ignore = false;
+                char *path;
 
                 if (IN_SET(mode, INACCESSIBLE, READONLY, READWRITE) && startswith(*i, "-")) {
                         (*i)++;
@@ -177,7 +192,11 @@ static int append_mounts(BindMount **p, char **strv, MountMode mode) {
                 if (!path_is_absolute(*i))
                         return -EINVAL;
 
-                set_bind_mount(p, *i, mode, ignore);
+                path = strdup(*i);
+                if (!path)
+                        return -ENOMEM;
+
+                set_bind_mount(p, path, mode, ignore);
                 (*p)++;
         }
 
@@ -196,7 +215,11 @@ static int append_target_mounts(BindMount **p, const char *root_directory, const
                  * declaration we do not support "-" at the beginning.
                  */
                 const TargetMount *m = &mounts[i];
-                const char *path = prefix_roota(root_directory, m->path);
+                char *path;
+
+                path = prefix_root(root_directory, m->path);
+                if (!path)
+                        return -ENOMEM;
 
                 if (!path_is_absolute(path))
                         return -EINVAL;
@@ -309,6 +332,7 @@ static void drop_duplicates(BindMount *m, unsigned *n) {
                  * above. */
                 if (previous && path_equal(f->path, previous->path)) {
                         log_debug("%s is duplicate.", f->path);
+                        f->path = mfree(f->path);
                         continue;
                 }
 
@@ -336,6 +360,7 @@ static void drop_inaccessible(BindMount *m, unsigned *n) {
                  * it, as inaccessible paths really should drop the entire subtree. */
                 if (clear && path_startswith(f->path, clear)) {
                         log_debug("%s is masked by %s.", f->path, clear);
+                        f->path = mfree(f->path);
                         continue;
                 }
 
@@ -375,6 +400,7 @@ static void drop_nop(BindMount *m, unsigned *n) {
                         /* We found it, let's see if it's the same mode, if so, we can drop this entry */
                         if (found && p->mode == f->mode) {
                                 log_debug("%s is redundant by %s", f->path, p->path);
+                                f->path = mfree(f->path);
                                 continue;
                         }
                 }
@@ -401,6 +427,7 @@ static void drop_outside_root(const char *root_directory, BindMount *m, unsigned
 
                 if (!path_startswith(f->path, root_directory)) {
                         log_debug("%s is outside of root directory.", f->path);
+                        f->path = mfree(f->path);
                         continue;
                 }
 
@@ -652,18 +679,21 @@ static int chase_all_symlinks(const char *root_directory, BindMount *m, unsigned
          * can't resolve the path, and which have been marked for such removal. */
 
         for (f = m, t = m; f < m+*n; f++) {
-
-                r = chase_symlinks(f->path, root_directory, &f->chased);
-                if (r == -ENOENT && f->ignore) /* Doesn't exist? Then remove it! */
+                _cleanup_free_ char *chased = NULL;
+                r = chase_symlinks(f->path, root_directory, &chased);
+                if (r == -ENOENT && f->ignore) {
+                        /* Doesn't exist? Then remove it! */
+                        f->path = mfree(f->path);
                         continue;
+                }
                 if (r < 0)
                         return log_debug_errno(r, "Failed to chase symlinks for %s: %m", f->path);
 
-                if (path_equal(f->path, f->chased))
-                        f->chased = mfree(f->chased);
-                else {
-                        log_debug("Chased %s → %s", f->path, f->chased);
-                        f->path = f->chased;
+                if (!path_equal(f->path, chased)) {
+                        log_debug("Chased %s → %s", f->path, chased);
+                        r = free_and_strdup(&f->path, chased);
+                        if (r < 0)
+                                return r;
                 }
 
                 *t = *f;
@@ -724,96 +754,96 @@ int setup_namespace(
 
         BindMount *m, *mounts = NULL;
         bool make_slave = false;
-        unsigned n;
+        unsigned n_mounts;
         int r = 0;
 
         if (mount_flags == 0)
                 mount_flags = MS_SHARED;
 
-        n = namespace_calculate_mounts(ns_info,
-                                       read_write_paths,
-                                       read_only_paths,
-                                       inaccessible_paths,
-                                       tmp_dir, var_tmp_dir,
-                                       protect_home, protect_system);
+        n_mounts = namespace_calculate_mounts(ns_info,
+                                              read_write_paths,
+                                              read_only_paths,
+                                              inaccessible_paths,
+                                              tmp_dir, var_tmp_dir,
+                                              protect_home, protect_system);
 
         /* Set mount slave mode */
-        if (root_directory || n > 0)
+        if (root_directory || n_mounts > 0)
                 make_slave = true;
 
-        if (n > 0) {
-                m = mounts = (BindMount *) alloca0(n * sizeof(BindMount));
+        if (n_mounts > 0) {
+                m = mounts = (BindMount *) alloca0(n_mounts * sizeof(BindMount));
                 r = append_mounts(&m, read_write_paths, READWRITE);
                 if (r < 0)
-                        return r;
+                        goto finish;
 
                 r = append_mounts(&m, read_only_paths, READONLY);
                 if (r < 0)
-                        return r;
+                        goto finish;
 
                 r = append_mounts(&m, inaccessible_paths, INACCESSIBLE);
                 if (r < 0)
-                        return r;
+                        goto finish;
 
                 if (tmp_dir) {
-                        m->path = prefix_roota(root_directory, "/tmp");
-                        m->mode = PRIVATE_TMP;
-                        m++;
+                        r = append_one_mount(&m, root_directory, "/tmp", PRIVATE_TMP, false);
+                        if (r < 0)
+                                goto finish;
                 }
 
                 if (var_tmp_dir) {
-                        m->path = prefix_roota(root_directory, "/var/tmp");
-                        m->mode = PRIVATE_VAR_TMP;
-                        m++;
+                        r = append_one_mount(&m, root_directory, "/var/tmp", PRIVATE_VAR_TMP, false);
+                        if (r < 0)
+                                goto finish;
                 }
 
                 if (ns_info->private_dev) {
-                        m->path = prefix_roota(root_directory, "/dev");
-                        m->mode = PRIVATE_DEV;
-                        m++;
+                        r = append_one_mount(&m, root_directory, "/dev", PRIVATE_DEV, false);
+                        if (r < 0)
+                                goto finish;
                 }
 
                 if (ns_info->protect_kernel_tunables) {
                         r = append_protect_kernel_tunables(&m, root_directory);
                         if (r < 0)
-                                return r;
+                                goto finish;
                 }
 
                 if (ns_info->protect_kernel_modules) {
                         r = append_protect_kernel_modules(&m, root_directory);
                         if (r < 0)
-                                return r;
+                                goto finish;
                 }
 
                 if (ns_info->protect_control_groups) {
-                        m->path = prefix_roota(root_directory, "/sys/fs/cgroup");
-                        m->mode = READONLY;
-                        m++;
+                        r = append_one_mount(&m, root_directory, "/sys/fs/cgroup", READONLY, false);
+                        if (r < 0)
+                                goto finish;
                 }
 
                 r = append_protect_home(&m, root_directory, protect_home);
                 if (r < 0)
-                        return r;
+                        goto finish;
 
                 r = append_protect_system(&m, root_directory, protect_system);
                 if (r < 0)
-                        return r;
+                        goto finish;
 
-                assert(mounts + n == m);
+                assert(mounts + n_mounts == m);
 
                 /* Resolve symlinks manually first, as mount() will always follow them relative to the host's
                  * root. Moreover we want to suppress duplicates based on the resolved paths. This of course is a bit
                  * racy. */
-                r = chase_all_symlinks(root_directory, mounts, &n);
+                r = chase_all_symlinks(root_directory, mounts, &n_mounts);
                 if (r < 0)
                         goto finish;
 
-                qsort(mounts, n, sizeof(BindMount), mount_path_compare);
+                qsort(mounts, n_mounts, sizeof(BindMount), mount_path_compare);
 
-                drop_duplicates(mounts, &n);
-                drop_outside_root(root_directory, mounts, &n);
-                drop_inaccessible(mounts, &n);
-                drop_nop(mounts, &n);
+                drop_duplicates(mounts, &n_mounts);
+                drop_outside_root(root_directory, mounts, &n_mounts);
+                drop_inaccessible(mounts, &n_mounts);
+                drop_nop(mounts, &n_mounts);
         }
 
         if (unshare(CLONE_NEWNS) < 0) {
@@ -843,25 +873,25 @@ int setup_namespace(
                 }
         }
 
-        if (n > 0) {
+        if (n_mounts > 0) {
                 char **blacklist;
                 unsigned j;
 
                 /* First round, add in all special mounts we need */
-                for (m = mounts; m < mounts + n; ++m) {
+                for (m = mounts; m < mounts + n_mounts; ++m) {
                         r = apply_mount(m, tmp_dir, var_tmp_dir);
                         if (r < 0)
                                 goto finish;
                 }
 
                 /* Create a blacklist we can pass to bind_mount_recursive() */
-                blacklist = newa(char*, n+1);
-                for (j = 0; j < n; j++)
+                blacklist = newa(char*, n_mounts+1);
+                for (j = 0; j < n_mounts; j++)
                         blacklist[j] = (char*) mounts[j].path;
                 blacklist[j] = NULL;
 
                 /* Second round, flip the ro bits if necessary. */
-                for (m = mounts; m < mounts + n; ++m) {
+                for (m = mounts; m < mounts + n_mounts; ++m) {
                         r = make_read_only(m, blacklist);
                         if (r < 0)
                                 goto finish;
@@ -886,8 +916,8 @@ int setup_namespace(
         r = 0;
 
 finish:
-        for (m = mounts; m < mounts + n; m++)
-                free(m->chased);
+        for (m = mounts; m < mounts + n_mounts; m++)
+                free(m->path);
 
         return r;
 }