core: make RootDirectory= and ProtectKernelModules= work #4594

tixxdz · 2016-11-06T13:07:52Z

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=, ProtectControlGroups= and
ProtectKernelModules=yes

Fixes: #4567

evverx

@tixxdz , thanks! I found two small issues (see comments). I'll run systemd under valgrind and under AddressSanitizer tomorrow.

evverx · 2016-11-06T16:40:47Z

src/core/namespace.c

+                if (!path)
+                        return -ENOMEM;
+
+                if (!path_is_absolute(path))


There is a memleak here. I think should be something like:

_cleanup_free_ char *path = NULL; ... path = strdup(*i); ... set_bind_mount(p, path, mode, ignore); path = NULL;

Hey @evverx could you please check again, since I reworked the whole logic and those paths are cleaned https://github.com/systemd/systemd/pull/4594/files#diff-f315b72505d5f0a92ec2f4d068d4f916R913 here and in different places where we chase symlinks, drop duplicate paths etc...

Oh, I see. So
https://github.com/systemd/systemd/pull/4594/files#diff-f315b72505d5f0a92ec2f4d068d4f916L746

r = append_mounts(&m, read_write_paths, READWRITE); if (r < 0) return r;

should be

r = append_mounts(&m, read_write_paths, READWRITE); if (r < 0) goto finish;

Right?

It seems that strdup should be done below the !path_is_absolute check. This would avoid the memleak.

evverx · 2016-11-06T16:43:59Z

src/core/namespace.c

@@ -196,7 +200,7 @@ 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 = prefix_root(root_directory, m->path);

                if (!path_is_absolute(path))


There is a memleak. Similar to my previous comment

Hmm ? I'm a bit lost here, could you please confirm ? thanks ;-)

evverx · 2016-11-06T18:08:34Z

src/core/namespace.c

@@ -756,64 +776,67 @@ int setup_namespace(
                        return r;

                if (tmp_dir) {
-                        m->path = prefix_roota(root_directory, "/tmp");
+                        m->path = prefix_root(root_directory, "/tmp");


Shouldn't we check the result of the prefix_root?

evverx · 2016-11-06T18:42:28Z

Also, I'd prefer to not mix two commits in this PR. I think we should fix the memory issue first. @tixxdz , what do you think?

tixxdz · 2016-11-06T19:05:04Z

@evverx ok, all fixed, the other patch is here #4596

Thanks!

evverx · 2016-11-06T19:48:27Z

@tixxdz , thanks!

I'll run systemd under valgrind/AddressSanitizer tomorrow

BTW TestPathsStat passed:

=== RUN   TestPathsStat
--- PASS: TestPathsStat (27.81s)
    rkt_tests.go:117: Running command: /home/vagrant/rkt/build-rkt-1.18.0+git/tmp/functional/rkt --dir=/tmp/datadir-374862404 --local-config=/tmp/localdir-303126739 --system-config=/tmp/systemdir-176106006 --user-config=/tmp/userdir-554123901 --insecure-options=image prepare /home/vagrant/rkt/build-rkt-1.18.0+git/tmp/functional/test-tmp/rkt-inspect-stat-procfs.aci
    rkt_tests.go:117: Running command: /home/vagrant/rkt/build-rkt-1.18.0+git/tmp/functional/rkt --dir=/tmp/datadir-492000184 --local-config=/tmp/localdir-105045943 --system-config=/tmp/systemdir-241623978 --user-config=/tmp/userdir-046887169 --insecure-options=image prepare /home/vagrant/rkt/build-rkt-1.18.0+git/tmp/functional/test-tmp/rkt-inspect-stat-procfs.aci
PASS
ok      github.com/coreos/rkt/tests 27.931s

keszybz

Looks good in general, but there's still a few iffy places.

keszybz · 2016-11-06T20:11:14Z

src/core/namespace.c

+                if (!path)
+                        return -ENOMEM;
+
+                if (!path_is_absolute(path))


It seems that strdup should be done below the !path_is_absolute check. This would avoid the memleak.

keszybz · 2016-11-06T20:12:11Z

src/core/namespace.c

-                const char *path = prefix_roota(root_directory, m->path);
+                char *path = prefix_root(root_directory, m->path);
+
+                if (!path)


It'd be more idiomatic to move the initalization out of the declaration block:

char *path; path = prefix_root(...); if (!path) ...

keszybz · 2016-11-06T20:15:15Z

src/core/namespace.c

@@ -309,6 +331,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);
+                        free(f->path);


Hm, even though this free is correct, it looks dangerous. Maybe f->path = mfree(f->path) would be safer.

yes the mountcnt catches this but better safe than sorry, updated.

keszybz · 2016-11-06T20:23:12Z

src/core/namespace.c

                        continue;
-                if (r < 0)
-                        return log_debug_errno(r, "Failed to chase symlinks for %s: %m", f->path);
+                } if (r < 0)


Newline here.

keszybz · 2016-11-06T20:26:11Z

src/core/namespace.c

                else {
-                        log_debug("Chased %s → %s", f->path, f->chased);
-                        f->path = f->chased;
+                        log_debug("Chased %s → %s", f->path, chased);


This manual string freeing looks error-prone. Maybe use _cleanup_free_ for chased, and free_and_replace here.

I think you mean free_and_strdup, thanks!

No, free_and_replace. No need to duplicate the string again.

keszybz · 2016-11-06T20:26:42Z

src/core/namespace.c

@@ -724,96 +753,96 @@ int setup_namespace(

        BindMount *m, *mounts = NULL;
        bool make_slave = false;
-        unsigned n;
+        unsigned mountcnt;


n_mounts?

tixxdz · 2016-11-06T21:59:15Z

@keszybz updated, thanks!

tixxdz · 2016-11-07T07:31:14Z

@martinpitt this one failed at xeinal-i386 test where previous versions of it succeeded, and this #4596 with same commit + another one succeeded, should I re-push or maybe a valid bug ? failed at this one https://anonscm.debian.org/cgit/pkg-systemd/systemd.git/tree/debian/tests/boot-and-services#n62 ? thanks!

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: systemd#4567

martinpitt · 2016-11-07T12:11:35Z

@tixxdz , the lightdm test is indeed a bit flaky; if that happens, I usually just retry it. But it's re-running now anyway due to new commits.

tixxdz · 2016-11-07T12:12:51Z

@martinpitt I see ok, thank you for the info. I forced pushed the same patch again.

keszybz · 2016-11-07T14:01:13Z

I added two small changes on top, please re-review.

tixxdz · 2016-11-07T14:11:45Z

@keszybz ah free_and_replace() sets chased to null, didn't notice that. Lgtm thank you!

evverx · 2016-11-07T21:39:32Z

@tixxdz , sorry for the delay. I'm testing. Stay tuned :-)

evverx · 2016-11-08T00:25:25Z

Hm, another test:

=== RUN   TestVolumeSysfs
--- FAIL: TestVolumeSysfs (12.67s)
...
        [309106.757998] rkt-inspect[12]: *** Error in `(inspect)': double free or corruption (fasttop): 0x000055bbdd76c550 ***
        [FAILED] Failed to start Application=rkt-inspect Image=coreos.com/rkt-inspect.
...

…s() fails This certainly fixes a bug that was introduced by PR systemd#4594 that intended to fix systemd#4567. The fix was not complete. This patch makes sure that we count freed paths when chase_all_symlinks() fails and we do not return directly. Also go ahead and remove assert() checks against mount counts being zero. This does not make sense since we are explicitly decrementing and freeing mount paths which makes it easy to hit the assert. If users did provide a misconfigured paths then just log a debug message, ignore and continue. Note that if mount count is zero at the beginning, then we we never hit this code path. Fixes systemd#4567

This certainly fixes a bug that was introduced by PR systemd#4594 that intended to fix systemd#4567. The fix was not complete. This patch makes sure that we count and free all paths that fail inside chase_all_symlinks(). Fixes systemd#4567

…() (#4619) This certainly fixes a bug that was introduced by PR #4594 that intended to fix #4567. The fix was not complete. This patch makes sure that we count and free all paths that fail inside chase_all_symlinks(). Fixes #4567

rkt currently tries to switch on `ProtectKernelTunables` for supported systemd versions (ie. >=232), but it does not work together with `RootDirectory` due to systemd/systemd#4594 which is targeted at v233. This commit bumps the version check accordingly.

…() (#4619) This certainly fixes a bug that was introduced by PR systemd/systemd#4594 that intended to fix systemd/systemd#4567. The fix was not complete. This patch makes sure that we count and free all paths that fail inside chase_all_symlinks(). Fixes systemd/systemd#4567 (cherry picked from commit 1d54cd5)

tixxdz mentioned this pull request Nov 6, 2016

RootDirectory=, ProtectKernelTunables=yes cause "ERROR: AddressSanitizer: stack-buffer-overflow" #4567

Closed

evverx requested changes Nov 6, 2016

View reviewed changes

evverx added the pid1 label Nov 6, 2016

tixxdz force-pushed the djalal/fix-rootdir-apply-mntns branch from e9615f4 to ca5acd1 Compare November 6, 2016 17:46

tixxdz changed the title ~~core: make RootDirectory= and ProtectKernelTunables= work~~ core: make RootDirectory= and ProtectKernelModules= work Nov 6, 2016

evverx reviewed Nov 6, 2016

View reviewed changes

tixxdz force-pushed the djalal/fix-rootdir-apply-mntns branch from ca5acd1 to 10e08b9 Compare November 6, 2016 18:54

tixxdz mentioned this pull request Nov 6, 2016

core: on DynamicUser= make sure that protecting sensitive paths is enforced #4596

Closed

tixxdz mentioned this pull request Nov 6, 2016

RootDirectory fails maybe on some (or all) options that create a mount namespace #4588

Open

tixxdz force-pushed the djalal/fix-rootdir-apply-mntns branch from 10e08b9 to cd92950 Compare November 6, 2016 19:18

evverx mentioned this pull request Nov 6, 2016

./configure --with-stage1-systemd-version=master changes the rkt-behaviour due to GetFlavor == 0 rkt/rkt#3344

Closed

keszybz requested changes Nov 6, 2016

View reviewed changes

tixxdz force-pushed the djalal/fix-rootdir-apply-mntns branch from cd92950 to ec066d2 Compare November 6, 2016 21:55

tixxdz force-pushed the djalal/fix-rootdir-apply-mntns branch from ec066d2 to f0a4feb Compare November 7, 2016 11:37

tixxdz added this to the v233 milestone Nov 7, 2016

keszybz added 2 commits November 7, 2016 08:54

nspawn: avoid one strdup by using free_and_replace

49fedb4

nspawn: slight simplification

46c3230

keszybz approved these changes Nov 7, 2016

View reviewed changes

evverx merged commit 453a9c7 into systemd:master Nov 7, 2016

tixxdz mentioned this pull request Nov 8, 2016

core:namespace: make sure to count freed paths when chase_all_symlink… #4619

Merged

lucab mentioned this pull request Dec 7, 2016

1.17.0+: Container failed with error code 226. rkt/rkt#3456

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: make RootDirectory= and ProtectKernelModules= work #4594

core: make RootDirectory= and ProtectKernelModules= work #4594

tixxdz commented Nov 6, 2016 •

edited

evverx left a comment

evverx Nov 6, 2016

tixxdz Nov 6, 2016

evverx Nov 6, 2016

keszybz Nov 6, 2016

evverx Nov 6, 2016

tixxdz Nov 6, 2016

evverx Nov 6, 2016

evverx commented Nov 6, 2016

tixxdz commented Nov 6, 2016

evverx commented Nov 6, 2016

keszybz left a comment

keszybz Nov 6, 2016

keszybz Nov 6, 2016

keszybz Nov 6, 2016

tixxdz Nov 6, 2016

keszybz Nov 6, 2016

keszybz Nov 6, 2016

tixxdz Nov 6, 2016

keszybz Nov 6, 2016

keszybz Nov 6, 2016

tixxdz commented Nov 6, 2016

tixxdz commented Nov 7, 2016

martinpitt commented Nov 7, 2016

tixxdz commented Nov 7, 2016

keszybz commented Nov 7, 2016

tixxdz commented Nov 7, 2016

evverx commented Nov 7, 2016

evverx commented Nov 8, 2016

core: make RootDirectory= and ProtectKernelModules= work #4594

core: make RootDirectory= and ProtectKernelModules= work #4594

Conversation

tixxdz commented Nov 6, 2016 • edited

evverx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evverx commented Nov 6, 2016

tixxdz commented Nov 6, 2016

evverx commented Nov 6, 2016

keszybz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tixxdz commented Nov 6, 2016

tixxdz commented Nov 7, 2016

martinpitt commented Nov 7, 2016

tixxdz commented Nov 7, 2016

keszybz commented Nov 7, 2016

tixxdz commented Nov 7, 2016

evverx commented Nov 7, 2016

evverx commented Nov 8, 2016

tixxdz commented Nov 6, 2016 •

edited