[systemd-devel] [PATCH] [PATCH v3] core: Private*/Protect* options with RootDirectory
Lennart Poettering
lennart at poettering.net
Wed May 13 08:48:15 PDT 2015
On Tue, 12.05.15 15:36, Alban Crequy (alban.crequy at gmail.com) wrote:
> diff --git a/src/core/execute.c b/src/core/execute.c
> index 1a297ba..d4ccac6 100644
> --- a/src/core/execute.c
> +++ b/src/core/execute.c
> @@ -1277,6 +1277,7 @@ static int exec_child(
> uid_t uid = UID_INVALID;
> gid_t gid = GID_INVALID;
> int i, r;
> + bool new_mnt_namespace;
I don't like unnecessary abbreviations I must say. Just call this
new_mount_namespace or needs_mount_namespace or so... There's really
no need in sparing two characters, it certainly doesn't help
readability...
> - if (!strv_isempty(context->read_write_dirs) ||
> + new_mnt_namespace = !strv_isempty(context->read_write_dirs) ||
> !strv_isempty(context->read_only_dirs) ||
> !strv_isempty(context->inaccessible_dirs) ||
> context->mount_flags != 0 ||
> @@ -1563,8 +1564,9 @@ static int exec_child(
> params->bus_endpoint_path ||
> context->private_devices ||
> context->protect_system != PROTECT_SYSTEM_NO ||
> - context->protect_home != PROTECT_HOME_NO) {
> + context->protect_home != PROTECT_HOME_NO;
I think we should take this one step further even, and move this whole
humgous if check into its own function.
Done so now in git.
>
> +static int mount_move_root(const char *path) {
> + if (chdir(path) < 0) {
> + return -errno;
> + }
> +
> + if (mount(path, "/", NULL, MS_MOVE, NULL) < 0) {
> + return -errno;
> + }
> +
> + if (chroot(".") < 0) {
> + return -errno;
> + }
> +
> + if (chdir("/") < 0) {
> + return -errno;
> + }
> +
> + return 0;
> +}
Please see CODING_STYLE, we do not add braces around single-line if
blocks. This is not PHP...
> +
> static int mount_dev(BindMount *m) {
> static const char devnodes[] =
> "/dev/null\0"
> @@ -225,7 +246,13 @@ static int mount_dev(BindMount *m) {
>
> dev_setup(temporary_mount);
>
> - if (mount(dev, "/dev/", NULL, MS_MOVE, NULL) < 0) {
> + /* Create the /dev directory if missing. It is more likely to be
> + * missing when the service is started with RootDirectory. This is
> + * consistent with mount units creating the mount points when missing.
> + */
> + mkdir_p_label (m->path, 0755);
Please see CODING_STYLE, no space between function name and opening "("
please...
Also, we nowadays try to explicitly mark calls where we knowingly
ignore the exit code by casting its result to (void). This is helpful
to indicate to static checkers like coverity, whether a return value
may be safely ignored, or not.
> +
> + if (mount(dev, m->path, NULL, MS_MOVE, NULL) < 0) {
> r = -errno;
> goto fail;
> }
> @@ -404,6 +431,7 @@ static int make_read_only(BindMount *m) {
> }
>
> int setup_namespace(
> + const char* chroot,
Hmm, I'd avoid naming a variable like a function needlessly...
> char** read_write_dirs,
> char** read_only_dirs,
> char** inaccessible_dirs,
> @@ -449,37 +477,48 @@ int setup_namespace(
> return r;
>
> if (tmp_dir) {
> - m->path = "/tmp";
> + m->path = strjoina(chroot?:"", "/tmp");
Hmm, this is very common expression that is used all over the place
now, I figure it's time to introduce a better expression for this,
that unifies this logic, and properly avoids creating duplicate
slashes in the resulting path.
I have thus added two calls now to path-util.c: prefix_root() and
prefix_roota(). The former allocates from the heap, the latter from
the stack using alloca(). In this case use prefix_roota() please.
> m->mode = PRIVATE_TMP;
> m++;
> }
>
> if (var_tmp_dir) {
> - m->path = "/var/tmp";
> + m->path = strjoina(chroot?:"", "/var/tmp");
Same here...
> m->mode = PRIVATE_VAR_TMP;
> m++;
> }
>
> if (private_dev) {
> - m->path = "/dev";
> + m->path = strjoina(chroot?:"", "/dev");
Same here...
> m->mode = PRIVATE_DEV;
> m++;
> }
>
> if (bus_endpoint_path) {
> - m->path = bus_endpoint_path;
> + m->path = strjoina(chroot?:"",
> bus_endpoint_path);
same here...
> m->mode = PRIVATE_BUS_ENDPOINT;
> m++;
> }
>
> if (protect_home != PROTECT_HOME_NO) {
> - r = append_mounts(&m, STRV_MAKE("-/home", "-/run/user", "-/root"), protect_home == PROTECT_HOME_READ_ONLY ? READONLY : INACCESSIBLE);
> + r = append_mounts(&m, STRV_MAKE(
> + strjoina("-", chroot?:"", "/home"),
> + strjoina("-", chroot?:"", "/run/user"),
> + strjoina("-", chroot?:"", "/root")),
> + protect_home ==
> PROTECT_HOME_READ_ONLY ? READONLY : INACCESSIBLE);
This is not OK. You may not use alloca() within function invocations,
this is not supported. See the alloca() man page, the part about BUGS.
Effectively this means the strv needs to be built explicitly, before
it is passed to append_mounts().
> if (r < 0)
> return r;
> }
>
> if (protect_system != PROTECT_SYSTEM_NO) {
> - r = append_mounts(&m, protect_system == PROTECT_SYSTEM_FULL ? STRV_MAKE("/usr", "-/boot", "/etc") : STRV_MAKE("/usr", "-/boot"), READONLY);
> + r = append_mounts(&m, protect_system == PROTECT_SYSTEM_FULL
> + ? STRV_MAKE(
> + strjoina(chroot?:"", "/usr"),
> + strjoina("-", chroot?:"", "/boot"),
> + strjoina(chroot?:"", "/etc")
> + ) : STRV_MAKE(
> + strjoina(chroot?:"", "/usr"),
> + strjoina("-", chroot?:"", "/boot")), READONLY);
> if (r < 0)
Same here...
> return r;
> }
> @@ -490,12 +529,21 @@ int setup_namespace(
> drop_duplicates(mounts, &n);
> }
>
> - if (n > 0) {
> + if (n > 0 || chroot) {
> /* Remount / as SLAVE so that nothing now mounted in the namespace
> shows up in the parent */
> if (mount(NULL, "/", NULL, MS_SLAVE|MS_REC, NULL) < 0)
> return -errno;
> + }
> +
> + if (chroot) {
> + /* Turn directory into bind mount */
> + if (mount(chroot, chroot, NULL, MS_BIND|MS_REC, NULL) < 0) {
> + return -errno;
> + }
Too many {}...
> diff --git a/src/test/test-ns.c b/src/test/test-ns.c
> index 76b131c..b412e98 100644
> --- a/src/test/test-ns.c
> +++ b/src/test/test-ns.c
> @@ -38,10 +38,12 @@ int main(int argc, char *argv[]) {
> NULL
> };
>
> - const char * const inaccessible[] = {
> + const char *inaccessible[] = {
> "/home/lennart/projects",
> NULL
> };
> + char *chroot = getenv("TEST_NS_CHROOT");
> + char *projects = getenv("TEST_NS_PROJECTS");
Please see CODING_STYLE, we try to avoid mixing declarations of
variables and invoking functions in the same lines...
> int r;
> char tmp_dir[] = "/tmp/systemd-private-XXXXXX",
> @@ -50,7 +52,11 @@ int main(int argc, char *argv[]) {
> assert_se(mkdtemp(tmp_dir));
> assert_se(mkdtemp(var_tmp_dir));
>
> - r = setup_namespace((char **) writable,
> + if (projects)
> + inaccessible[0] = projects;
> +
> + r = setup_namespace(chroot,
> + (char **) writable,
> (char **) readonly,
> (char **) inaccessible,
> tmp_dir,
> --
> 2.1.4
>
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Lennart
--
Lennart Poettering, Red Hat
More information about the systemd-devel
mailing list