From 8e0193aabf8de15a9e7d424bf774618f5853886f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 6 Feb 2025 23:01:37 +0100 Subject: [PATCH 1/7] tree-wide: pass EBADF to fd params of namespace_fork() --- src/machine/machined-core.c | 12 ++++++------ src/shared/mount-util.c | 14 ++++++++++++-- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/machine/machined-core.c b/src/machine/machined-core.c index 90fd91f134..5c6d225051 100644 --- a/src/machine/machined-core.c +++ b/src/machine/machined-core.c @@ -230,11 +230,11 @@ int machine_get_addresses(Machine *machine, struct local_address **ret_addresses /* except_fds = */ NULL, /* n_except_fds = */ 0, FORK_RESET_SIGNALS|FORK_DEATHSIG_SIGKILL, - /* pidns_fd = */ -1, - /* mntns_fd = */ -1, + /* pidns_fd = */ -EBADF, + /* mntns_fd = */ -EBADF, netns_fd, - /* userns_fd = */ -1, - /* root_fd = */ -1, + /* userns_fd = */ -EBADF, + /* root_fd = */ -EBADF, &child); if (r < 0) return log_debug_errno(r, "Failed to fork(): %m"); @@ -347,8 +347,8 @@ int machine_get_os_release(Machine *machine, char ***ret_os_release) { FORK_RESET_SIGNALS|FORK_DEATHSIG_SIGKILL, pidns_fd, mntns_fd, - /* netns_fd = */ -1, - /* userns_fd = */ -1, + /* netns_fd = */ -EBADF, + /* userns_fd = */ -EBADF, root_fd, &child); if (r < 0) diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 75a18051bc..d471825162 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -1038,8 +1038,18 @@ static int mount_in_namespace_legacy( goto finish; } - r = namespace_fork("(sd-bindmnt)", "(sd-bindmnt-inner)", NULL, 0, FORK_RESET_SIGNALS|FORK_DEATHSIG_SIGTERM, - pidns_fd, mntns_fd, -1, -1, root_fd, &child); + r = namespace_fork( + "(sd-bindmnt)", + "(sd-bindmnt-inner)", + /* except_fds= */ NULL, + /* n_except_fds= */ 0, + FORK_RESET_SIGNALS|FORK_DEATHSIG_SIGTERM, + pidns_fd, + mntns_fd, + /* netns_fd= */ -EBADF, + /* userns_fd= */ -EBADF, + root_fd, + &child); if (r < 0) goto finish; if (r == 0) { From 3075ea0bc98286412381941469244227e34dccf0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 6 Feb 2025 23:03:46 +0100 Subject: [PATCH 2/7] mount-util: refactor make_mount_point_inode_from_xyz() This replaces make_mount_point_inode_from_stat() by make_mount_point_inode_from_mode() and makes it take a single mode_t rather than a "struct stat". Moreover, at an "atfd" style directory parameter. Then port all users over to new feature, and in particular make use of the directory fd: use chase() to create and pin parent directories first where needed. --- src/shared/mount-util.c | 50 ++++++++++++++++++++------------------ src/shared/mount-util.h | 4 +-- src/sysext/sysext.c | 20 +++++++++------ src/test/test-mount-util.c | 4 +-- 4 files changed, 43 insertions(+), 35 deletions(-) diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index d471825162..3ff15a2214 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -958,10 +958,7 @@ static int mount_in_namespace_legacy( /* Second, we mount the source file or directory to a directory inside of our MS_SLAVE playground. */ mount_tmp = strjoina(mount_slave, "/mount"); - if (flags & MOUNT_IN_NAMESPACE_IS_IMAGE) - r = mkdir_p(mount_tmp, 0700); - else - r = make_mount_point_inode_from_stat(chased_src_st, mount_tmp, 0700); + r = make_mount_point_inode_from_mode(AT_FDCWD, mount_tmp, (flags & MOUNT_IN_NAMESPACE_IS_IMAGE) ? S_IFDIR : chased_src_st->st_mode, 0700); if (r < 0) { log_debug_errno(r, "Failed to create temporary mount point %s: %m", mount_tmp); goto finish; @@ -1057,12 +1054,15 @@ static int mount_in_namespace_legacy( errno_pipe_fd[0] = safe_close(errno_pipe_fd[0]); - if (flags & MOUNT_IN_NAMESPACE_MAKE_FILE_OR_DIRECTORY) { - if (!(flags & MOUNT_IN_NAMESPACE_IS_IMAGE)) { - (void) mkdir_parents(dest, 0755); - (void) make_mount_point_inode_from_stat(chased_src_st, dest, 0700); - } else - (void) mkdir_p(dest, 0755); + _cleanup_close_ int dest_fd = -EBADF; + _cleanup_free_ char *dest_fn = NULL; + r = chase(dest, /* root= */ NULL, CHASE_PARENT|CHASE_EXTRACT_FILENAME|((flags & MOUNT_IN_NAMESPACE_MAKE_FILE_OR_DIRECTORY) ? CHASE_MKDIR_0755 : 0), &dest_fn, &dest_fd); + if (r < 0) + log_debug_errno(r, "Failed to pin parent directory of mount '%s', ignoring: %m", dest); + else if (flags & MOUNT_IN_NAMESPACE_MAKE_FILE_OR_DIRECTORY) { + r = make_mount_point_inode_from_mode(dest_fd, dest_fn, (flags & MOUNT_IN_NAMESPACE_IS_IMAGE) ? S_IFDIR : chased_src_st->st_mode, 0700); + if (r < 0) + log_debug_errno(r, "Failed to make mount point inode of mount '%s', ignoring: %m", dest); } /* Fifth, move the mount to the right place inside */ @@ -1076,7 +1076,7 @@ static int mount_in_namespace_legacy( if (!mount_inside) report_errno_and_exit(errno_pipe_fd[1], log_oom_debug()); - r = mount_nofollow_verbose(LOG_DEBUG, mount_inside, dest, NULL, MS_MOVE, NULL); + r = mount_nofollow_verbose(LOG_DEBUG, mount_inside, dest_fd >= 0 ? FORMAT_PROC_FD_PATH(dest_fd) : dest, /* fstype= */ NULL, MS_MOVE, /* options= */ NULL); if (r < 0) report_errno_and_exit(errno_pipe_fd[1], r); @@ -1247,8 +1247,14 @@ static int mount_in_namespace( if (r == 0) { errno_pipe_fd[0] = safe_close(errno_pipe_fd[0]); + _cleanup_close_ int dest_fd = -EBADF; + _cleanup_free_ char *dest_fn = NULL; + r = chase(dest, /* root= */ NULL, CHASE_PARENT|CHASE_EXTRACT_FILENAME|((flags & MOUNT_IN_NAMESPACE_MAKE_FILE_OR_DIRECTORY) ? CHASE_MKDIR_0755 : 0), &dest_fn, &dest_fd); + if (r < 0) + report_errno_and_exit(errno_pipe_fd[1], r); + if (flags & MOUNT_IN_NAMESPACE_MAKE_FILE_OR_DIRECTORY) - (void) mkdir_parents(dest, 0755); + (void) make_mount_point_inode_from_mode(dest_fd, dest_fn, img ? S_IFDIR : st.st_mode, 0700); if (img) { DissectImageFlags f = @@ -1268,12 +1274,8 @@ static int mount_in_namespace( /* uid_range= */ UID_INVALID, /* userns_fd= */ -EBADF, f); - } else { - if (flags & MOUNT_IN_NAMESPACE_MAKE_FILE_OR_DIRECTORY) - (void) make_mount_point_inode_from_stat(&st, dest, 0700); - + } else r = mount_exchange_graceful(new_mount_fd, dest, /* mount_beneath= */ true); - } report_errno_and_exit(errno_pipe_fd[1], r); } @@ -1698,17 +1700,17 @@ int bind_mount_submounts( return ret; } -int make_mount_point_inode_from_stat(const struct stat *st, const char *dest, mode_t mode) { - assert(st); +int make_mount_point_inode_from_mode(int dir_fd, const char *dest, mode_t source_mode, mode_t target_mode) { + assert(dir_fd >= 0 || dir_fd == AT_FDCWD); assert(dest); - if (S_ISDIR(st->st_mode)) - return mkdir_label(dest, mode); + if (S_ISDIR(source_mode)) + return mkdirat_label(dir_fd, dest, target_mode & 07777); else - return RET_NERRNO(mknod(dest, S_IFREG|(mode & ~0111), 0)); + return RET_NERRNO(mknodat(dir_fd, dest, S_IFREG|(target_mode & 07666), 0)); /* Mask off X bit */ } -int make_mount_point_inode_from_path(const char *source, const char *dest, mode_t mode) { +int make_mount_point_inode_from_path(const char *source, const char *dest, mode_t access_mode) { struct stat st; assert(source); @@ -1717,7 +1719,7 @@ int make_mount_point_inode_from_path(const char *source, const char *dest, mode_ if (stat(source, &st) < 0) return -errno; - return make_mount_point_inode_from_stat(&st, dest, mode); + return make_mount_point_inode_from_mode(AT_FDCWD, dest, st.st_mode, access_mode); } int trigger_automount_at(int dir_fd, const char *path) { diff --git a/src/shared/mount-util.h b/src/shared/mount-util.h index 895fe2b2cd..fa55c9d32c 100644 --- a/src/shared/mount-util.h +++ b/src/shared/mount-util.h @@ -178,8 +178,8 @@ int bind_mount_submounts( const char *source, const char *target); -/* Creates a mount point (not parents) based on the source path or stat - ie, a file or a directory */ -int make_mount_point_inode_from_stat(const struct stat *st, const char *dest, mode_t mode); +/* Creates a mount point (without any parents) based on the source path or mode - i.e., a file or a directory */ +int make_mount_point_inode_from_mode(int dir_fd, const char *dest, mode_t source_mode, mode_t target_mode); int make_mount_point_inode_from_path(const char *source, const char *dest, mode_t mode); int trigger_automount_at(int dir_fd, const char *path); diff --git a/src/sysext/sysext.c b/src/sysext/sysext.c index 9b1839d43a..92c60c49ae 100644 --- a/src/sysext/sysext.c +++ b/src/sysext/sysext.c @@ -297,22 +297,28 @@ static int move_submounts(const char *src, const char *dst) { assert_se(suffix = path_startswith(m->path, src)); + if (fstat(m->mount_fd, &st) < 0) + return log_error_errno(errno, "Failed to stat %s: %m", m->path); + t = path_join(dst, suffix); if (!t) return log_oom(); - if (fstat(m->mount_fd, &st) < 0) - return log_error_errno(errno, "Failed to stat %s: %m", m->path); - - r = mkdir_parents(t, 0755); + _cleanup_free_ char *fn = NULL; + _cleanup_close_ int fd = -EBADF; + r = chase(t, /* root= */ NULL, CHASE_PARENT|CHASE_EXTRACT_FILENAME|CHASE_PROHIBIT_SYMLINKS|CHASE_MKDIR_0755, &fn, &fd); if (r < 0) - return log_error_errno(r, "Failed to create parent directories of %s: %m", t); + return log_error_errno(r, "Failed to create and pin parent directory of %s: %m", t); - r = make_mount_point_inode_from_stat(&st, t, 0755); + r = make_mount_point_inode_from_mode(fd, fn, st.st_mode, 0755); if (r < 0 && r != -EEXIST) return log_error_errno(r, "Failed to create mountpoint %s: %m", t); - r = mount_follow_verbose(LOG_ERR, m->path, t, NULL, MS_BIND|MS_REC, NULL); + _cleanup_close_ int child_fd = openat(fd, fn, O_PATH|O_CLOEXEC); + if (child_fd < 0) + return log_error_errno(errno, "Failed to pin mountpoint %s: %m", t); + + r = mount_follow_verbose(LOG_ERR, m->path, FORMAT_PROC_FD_PATH(child_fd), /* fstype= */ NULL, MS_BIND|MS_REC, /* options= */ NULL); if (r < 0) return r; diff --git a/src/test/test-mount-util.c b/src/test/test-mount-util.c index 5fb4d23f3e..33220abe71 100644 --- a/src/test/test-mount-util.c +++ b/src/test/test-mount-util.c @@ -266,9 +266,9 @@ TEST(make_mount_point_inode) { assert_se(rmdir(dst_dir) == 0); assert_se(stat(src_file, &st) == 0); - assert_se(make_mount_point_inode_from_stat(&st, dst_file, 0755) >= 0); + assert_se(make_mount_point_inode_from_mode(AT_FDCWD, dst_file, st.st_mode, 0755) >= 0); assert_se(stat(src_dir, &st) == 0); - assert_se(make_mount_point_inode_from_stat(&st, dst_dir, 0755) >= 0); + assert_se(make_mount_point_inode_from_mode(AT_FDCWD, dst_dir, st.st_mode, 0755) >= 0); assert_se(stat(dst_dir, &st) == 0); assert_se(S_ISDIR(st.st_mode)); From 38c35970b13c47de731003535efec516b75c473e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 6 Feb 2025 23:08:37 +0100 Subject: [PATCH 3/7] core: port mount unit inode creation to make_mount_point_inode_from_mode() too This also ports over things to use chase() to create/pin the underlying to mount, and in particular checks that the path does not contain any symlinks. That's crucial since we cannot allow mounts to be established with that, since it would mean we couldn't recognize the entries in /proc/self/mountinfo anymore. --- src/core/automount.c | 2 +- src/core/mount.c | 31 ++++++++++++++++++++++--------- src/core/unit.c | 35 +++++++++++++++++++++-------------- src/core/unit.h | 3 ++- 4 files changed, 46 insertions(+), 25 deletions(-) diff --git a/src/core/automount.c b/src/core/automount.c index ac81875442..5ac5dec8d9 100644 --- a/src/core/automount.c +++ b/src/core/automount.c @@ -556,7 +556,7 @@ static void automount_enter_waiting(Automount *a) { set_clear(a->tokens); - r = unit_fail_if_noncanonical(UNIT(a), a->where); + r = unit_fail_if_noncanonical_mount_path(UNIT(a), a->where); if (r < 0) goto fail; diff --git a/src/core/mount.c b/src/core/mount.c index d7557bf389..361d8e0318 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -8,11 +8,13 @@ #include "sd-messages.h" #include "alloc-util.h" +#include "chase.h" #include "dbus-mount.h" #include "dbus-unit.h" #include "device.h" #include "exec-credential.h" #include "exit-status.h" +#include "fd-util.h" #include "format-util.h" #include "fs-util.h" #include "fstab-util.h" @@ -1183,22 +1185,32 @@ static int mount_set_mount_command(Mount *m, ExecCommand *c, const MountParamete } static void mount_enter_mounting(Mount *m) { - MountParameters *p; - bool source_is_dir = true; + _cleanup_close_ int fd = -EBADF; + _cleanup_free_ char *fn = NULL; int r; assert(m); - r = unit_fail_if_noncanonical(UNIT(m), m->where); - if (r < 0) + /* Validate that the path we are overmounting does not contain any symlinks, because if it does, we + * couldn't support that reasonably: the mounts in /proc/self/mountinfo would not be recognizable to + * us anymore. */ + fd = chase_and_open_parent(m->where, /* root= */ NULL, CHASE_PROHIBIT_SYMLINKS|CHASE_MKDIR_0755, &fn); + if (fd == -EREMCHG) { + r = unit_log_noncanonical_mount_path(UNIT(m), m->where); goto fail; + } + if (fd < 0) { + log_unit_error_errno(UNIT(m), fd, "Failed to resolve parent of mount point '%s': %m", m->where); + goto fail; + } - p = get_mount_parameters_fragment(m); + MountParameters *p = get_mount_parameters_fragment(m); if (!p) { r = log_unit_warning_errno(UNIT(m), SYNTHETIC_ERRNO(ENOENT), "No mount parameters to operate on."); goto fail; } + bool source_is_dir = true; if (mount_is_bind(p)) { r = is_dir(p->what, /* follow = */ true); if (r < 0 && r != -ENOENT) @@ -1207,10 +1219,11 @@ static void mount_enter_mounting(Mount *m) { source_is_dir = false; } - if (source_is_dir) - r = mkdir_p_label(m->where, m->directory_mode); - else - r = touch_file(m->where, /* parents = */ true, USEC_INFINITY, UID_INVALID, GID_INVALID, MODE_INVALID); + r = make_mount_point_inode_from_mode( + fd, + fn, + source_is_dir ? S_IFDIR : S_IFREG, + m->directory_mode); if (r < 0 && r != -EEXIST) log_unit_warning_errno(UNIT(m), r, "Failed to create mount point '%s', ignoring: %m", m->where); diff --git a/src/core/unit.c b/src/core/unit.c index 3f4432f655..9160876635 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -5125,23 +5125,10 @@ void unit_warn_if_dir_nonempty(Unit *u, const char* where) { "WHERE=%s", where); } -int unit_fail_if_noncanonical(Unit *u, const char* where) { - _cleanup_free_ char *canonical_where = NULL; - int r; - +int unit_log_noncanonical_mount_path(Unit *u, const char *where) { assert(u); assert(where); - r = chase(where, NULL, CHASE_NONEXISTENT, &canonical_where, NULL); - if (r < 0) { - log_unit_debug_errno(u, r, "Failed to check %s for symlinks, ignoring: %m", where); - return 0; - } - - /* We will happily ignore a trailing slash (or any redundant slashes) */ - if (path_equal(where, canonical_where)) - return 0; - /* No need to mention "." or "..", they would already have been rejected by unit_name_from_path() */ log_unit_struct(u, LOG_ERR, "MESSAGE_ID=" SD_MESSAGE_OVERMOUNTING_STR, @@ -5152,6 +5139,26 @@ int unit_fail_if_noncanonical(Unit *u, const char* where) { return -ELOOP; } +int unit_fail_if_noncanonical_mount_path(Unit *u, const char* where) { + int r; + + assert(u); + assert(where); + + _cleanup_free_ char *canonical_where = NULL; + r = chase(where, /* root= */ NULL, CHASE_NONEXISTENT, &canonical_where, /* ret_fd= */ NULL); + if (r < 0) { + log_unit_debug_errno(u, r, "Failed to check %s for symlinks, ignoring: %m", where); + return 0; + } + + /* We will happily ignore a trailing slash (or any redundant slashes) */ + if (path_equal(where, canonical_where)) + return 0; + + return unit_log_noncanonical_mount_path(u, where); +} + bool unit_is_pristine(Unit *u) { assert(u); diff --git a/src/core/unit.h b/src/core/unit.h index 45b7d72b7a..4b19d8f0f7 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -974,7 +974,8 @@ static inline PidRef* unit_main_pid(Unit *u) { } void unit_warn_if_dir_nonempty(Unit *u, const char* where); -int unit_fail_if_noncanonical(Unit *u, const char* where); +int unit_log_noncanonical_mount_path(Unit *u, const char *where); +int unit_fail_if_noncanonical_mount_path(Unit *u, const char* where); int unit_test_start_limit(Unit *u); From 66b5e7dfaa85df938811a34732f50ec708a9ae11 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 7 Feb 2025 14:32:44 +0100 Subject: [PATCH 4/7] catalog: assign a proper message ID for mounts on symlinked paths For some reason we reused the non-empty catalog entry so far, which is plain wrong. Correct that. --- catalog/systemd.catalog.in | 12 ++++++++++++ src/core/unit.c | 2 +- src/systemd/sd-messages.h | 2 ++ 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/catalog/systemd.catalog.in b/catalog/systemd.catalog.in index f3ca748862..801bda0939 100644 --- a/catalog/systemd.catalog.in +++ b/catalog/systemd.catalog.in @@ -460,6 +460,18 @@ this directory become inaccessible. To see those over-mounted files, please manually mount the underlying file system to a secondary location. +-- 1edabb4eda2a49c19bc0206f24b43889 +Subject: Mount point path contains symlinks +Defined-By: systemd +Support: %SUPPORT_URL% + +The path @WHERE@ is specified as a mount point path (second field in /etc/fstab +or Where= field in systemd unit file) and is not canonical, i.e. contains one +or more symlinks as path elements. This is generally not supported and such +mount attempts are refused, because the mount table information exposed by the +kernel and the requested path would deviate once established. Please +canonicalize paths before requesting a mount to be established. + -- 24d8d4452573402496068381a6312df2 Subject: A virtual machine or container has been started Defined-By: systemd diff --git a/src/core/unit.c b/src/core/unit.c index 9160876635..b784342c08 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -5131,7 +5131,7 @@ int unit_log_noncanonical_mount_path(Unit *u, const char *where) { /* No need to mention "." or "..", they would already have been rejected by unit_name_from_path() */ log_unit_struct(u, LOG_ERR, - "MESSAGE_ID=" SD_MESSAGE_OVERMOUNTING_STR, + "MESSAGE_ID=" SD_MESSAGE_NON_CANONICAL_MOUNT_STR, LOG_UNIT_INVOCATION_ID(u), LOG_UNIT_MESSAGE(u, "Mount path %s is not canonical (contains a symlink).", where), "WHERE=%s", where); diff --git a/src/systemd/sd-messages.h b/src/systemd/sd-messages.h index 441f4e6888..cc30add200 100644 --- a/src/systemd/sd-messages.h +++ b/src/systemd/sd-messages.h @@ -190,6 +190,8 @@ _SD_BEGIN_DECLARATIONS; #define SD_MESSAGE_OVERMOUNTING SD_ID128_MAKE(1d,ee,03,69,c7,fc,47,36,b7,09,9b,38,ec,b4,6e,e7) #define SD_MESSAGE_OVERMOUNTING_STR SD_ID128_MAKE_STR(1d,ee,03,69,c7,fc,47,36,b7,09,9b,38,ec,b4,6e,e7) +#define SD_MESSAGE_NON_CANONICAL_MOUNT SD_ID128_MAKE(1e,da,bb,4e,da,2a,49,c1,9b,c0,20,6f,24,b4,38,89) +#define SD_MESSAGE_NON_CANONICAL_MOUNT_STR SD_ID128_MAKE_STR(1e,da,bb,4e,da,2a,49,c1,9b,c0,20,6f,24,b4,38,89) #define SD_MESSAGE_UNIT_OOMD_KILL SD_ID128_MAKE(d9,89,61,1b,15,e4,4c,9d,bf,31,e3,c8,12,56,e4,ed) #define SD_MESSAGE_UNIT_OOMD_KILL_STR SD_ID128_MAKE_STR(d9,89,61,1b,15,e4,4c,9d,bf,31,e3,c8,12,56,e4,ed) From 61178346e66b06a63b1c75a19dd63438aa0e0cfa Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 7 Feb 2025 13:43:30 +0100 Subject: [PATCH 5/7] mount-tool: modernize umount and make sure it works for bind mounted files So far, "systemd-umount" executed on a bind mounted file would assume it is supposed to unmount a loopback mounted file system. Let's address that by instead checking if the file is a mount. --- src/mount/mount-tool.c | 52 +++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 21 deletions(-) diff --git a/src/mount/mount-tool.c b/src/mount/mount-tool.c index d1b4773ce8..3dd34b9e73 100644 --- a/src/mount/mount-tool.c +++ b/src/mount/mount-tool.c @@ -1044,7 +1044,11 @@ static int action_umount( int argc, char **argv) { - int r, r2 = 0; + int r, ret = 0; + + assert(bus); + assert(argv); + assert(argc > optind); if (arg_transport != BUS_TRANSPORT_LOCAL) { for (int i = optind; i < argc; i++) { @@ -1054,46 +1058,52 @@ static int action_umount( if (r < 0) return r; - r = stop_mounts(bus, p); - if (r < 0) - r2 = r; + RET_GATHER(ret, stop_mounts(bus, p)); } - return r2; + return ret; } for (int i = optind; i < argc; i++) { _cleanup_free_ char *u = NULL, *p = NULL; - struct stat st; u = fstab_node_to_udev_node(argv[i]); if (!u) return log_oom(); - r = chase(u, NULL, 0, &p, NULL); + _cleanup_close_ int fd = -EBADF; + r = chase(u, /* root= */ NULL, 0, &p, &fd); if (r < 0) { - r2 = log_error_errno(r, "Failed to make path %s absolute: %m", argv[i]); + RET_GATHER(ret, log_error_errno(r, "Failed to make path %s absolute: %m", u)); continue; } - if (stat(p, &st) < 0) + struct stat st; + if (fstat(fd, &st) < 0) return log_error_errno(errno, "Can't stat %s (from %s): %m", p, argv[i]); - if (S_ISBLK(st.st_mode)) - r = umount_by_device_node(bus, p); - else if (S_ISREG(st.st_mode)) - r = umount_loop(bus, p); - else if (S_ISDIR(st.st_mode)) - r = stop_mounts(bus, p); + r = is_mount_point_at(fd, /* filename= */ NULL, /* flags= */ 0); + fd = safe_close(fd); /* before continuing make sure the dir is not keeping anything busy */ + if (r > 0) + RET_GATHER(ret, stop_mounts(bus, p)); else { - log_error("Invalid file type: %s (from %s)", p, argv[i]); - r = -EINVAL; - } + /* This can realistically fail on pre-5.8 kernels that do not tell us via statx() if + * something is a mount point, hence handle this gracefully, and go by type as we did + * in pre-v258 times. */ + if (r < 0) + log_warning_errno(r, "Failed to determine if '%s' is a mount point, ignoring: %m", u); - if (r < 0) - r2 = r; + if (S_ISBLK(st.st_mode)) + RET_GATHER(ret, umount_by_device_node(bus, p)); + else if (S_ISREG(st.st_mode)) + RET_GATHER(ret, umount_loop(bus, p)); + else if (S_ISDIR(st.st_mode)) + RET_GATHER(ret, stop_mounts(bus, p)); + else + RET_GATHER(ret, log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Invalid file type: %s (from %s)", p, argv[i])); + } } - return r2; + return ret; } static int acquire_mount_type(sd_device *d) { From 4e24796b5a64064f8b21c07356eba19e9db4bb33 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 7 Feb 2025 12:32:16 +0100 Subject: [PATCH 6/7] mount-tool: add explicitly control of path canonicalization With this the default canonicalization of paths can be turned off, giving users explicit control on what shall happen if symlinks are encountered within a path. --- man/systemd-mount.xml | 18 ++++++++++++++++++ src/mount/mount-tool.c | 39 +++++++++++++++++++++++++++------------ 2 files changed, 45 insertions(+), 12 deletions(-) diff --git a/man/systemd-mount.xml b/man/systemd-mount.xml index d0eb9678d7..9723af64d6 100644 --- a/man/systemd-mount.xml +++ b/man/systemd-mount.xml @@ -340,6 +340,24 @@ + + + + + Controls whether the specified path shall be canonicalized on the client side before + requesting the operation or not. Takes a boolean parameter, defaults to true. Note that for + non-local operation (i.e. when or -- are used) + canonicalization is implicitly turned off. + + Canonicalization of path entails resolving of symlinks, .. path elements + and LABEL=/UUID= style device node expansion. If + canonicalization is disabled and the path contains a symlink element, .., or a + LABEL=/UUID=/… expansion the operation will fail. + + + + + diff --git a/src/mount/mount-tool.c b/src/mount/mount-tool.c index 3dd34b9e73..27fd318fb0 100644 --- a/src/mount/mount-tool.c +++ b/src/mount/mount-tool.c @@ -75,6 +75,7 @@ static bool arg_fsck = true; static bool arg_aggressive_gc = false; static bool arg_tmpfs = false; static sd_json_format_flags_t arg_json_format_flags = SD_JSON_FORMAT_OFF; +static bool arg_canonicalize = true; STATIC_DESTRUCTOR_REGISTER(arg_mount_what, freep); STATIC_DESTRUCTOR_REGISTER(arg_mount_where, freep); @@ -90,14 +91,14 @@ static int parse_where(const char *input, char **ret_where) { assert(input); assert(ret_where); - if (arg_transport == BUS_TRANSPORT_LOCAL) { - r = chase(input, NULL, CHASE_NONEXISTENT, ret_where, NULL); + if (arg_transport == BUS_TRANSPORT_LOCAL && arg_canonicalize) { + r = chase(input, /* root= */ NULL, CHASE_NONEXISTENT, ret_where, /* ret_fd= */ NULL); if (r < 0) return log_error_errno(r, "Failed to make path %s absolute: %m", input); } else { if (!path_is_absolute(input)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Path must be absolute when operating remotely: %s", + "Path must be absolute when operating remotely or when canonicalization is turned off: %s", input); r = path_simplify_alloc(input, ret_where); @@ -119,8 +120,9 @@ static int help(void) { printf("systemd-mount [OPTIONS...] WHAT [WHERE]\n" "systemd-mount [OPTIONS...] --tmpfs [NAME] WHERE\n" "systemd-mount [OPTIONS...] --list\n" - "%s [OPTIONS...] %sWHAT|WHERE...\n\n" - "%sEstablish a mount or auto-mount point transiently.%s\n\n" + "%1$s [OPTIONS...] %7$sWHAT|WHERE...\n" + "\n%5$sEstablish a mount or auto-mount point transiently.%6$s\n" + "\n%3$sOptions:%4$s\n" " -h --help Show this help\n" " --version Show package version\n" " --no-block Do not wait until operation finished\n" @@ -149,12 +151,16 @@ static int help(void) { " -u --umount Unmount mount points\n" " -G --collect Unload unit after it stopped, even when failed\n" " -T --tmpfs Create a new tmpfs on the mount point\n" - "\nSee the %s for details.\n", + " --canonicalize=BOOL Controls whether to canonicalize path before\n" + " operation\n" + "\nSee the %2$s for details.\n", program_invocation_short_name, - streq(program_invocation_short_name, "systemd-umount") ? "" : "--umount ", + link, + ansi_underline(), + ansi_normal(), ansi_highlight(), ansi_normal(), - link); + streq(program_invocation_short_name, "systemd-umount") ? "" : "--umount "); return 0; } @@ -181,6 +187,7 @@ static int parse_argv(int argc, char *argv[]) { ARG_BIND_DEVICE, ARG_LIST, ARG_JSON, + ARG_CANONICALIZE, }; static const struct option options[] = { @@ -213,6 +220,7 @@ static int parse_argv(int argc, char *argv[]) { { "collect", no_argument, NULL, 'G' }, { "tmpfs", no_argument, NULL, 'T' }, { "json", required_argument, NULL, ARG_JSON }, + { "canonicalize", required_argument, NULL, ARG_CANONICALIZE }, {}, }; @@ -374,6 +382,13 @@ static int parse_argv(int argc, char *argv[]) { break; + case ARG_CANONICALIZE: + r = parse_boolean_argument("--canonicalize=", optarg, &arg_canonicalize); + if (r < 0) + return r; + + break; + case '?': return -EINVAL; @@ -441,21 +456,21 @@ static int parse_argv(int argc, char *argv[]) { if (!arg_mount_what) return log_oom(); - } else if (arg_transport == BUS_TRANSPORT_LOCAL) { + } else if (arg_transport == BUS_TRANSPORT_LOCAL && arg_canonicalize) { _cleanup_free_ char *u = NULL; u = fstab_node_to_udev_node(argv[optind]); if (!u) return log_oom(); - r = chase(u, NULL, 0, &arg_mount_what, NULL); + r = chase(u, /* root= */ NULL, /* flags= */ 0, &arg_mount_what, /* ret_fd= */ NULL); if (r < 0) return log_error_errno(r, "Failed to make path %s absolute: %m", u); } else { if (!path_is_absolute(argv[optind])) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Path must be absolute when operating remotely: %s", + "Path must be absolute when operating remotely or when canonicalization is turned off: %s", argv[optind]); r = path_simplify_alloc(argv[optind], &arg_mount_what); @@ -1050,7 +1065,7 @@ static int action_umount( assert(argv); assert(argc > optind); - if (arg_transport != BUS_TRANSPORT_LOCAL) { + if (arg_transport != BUS_TRANSPORT_LOCAL || !arg_canonicalize) { for (int i = optind; i < argc; i++) { _cleanup_free_ char *p = NULL; From a34ce4842b5deafd3ea6654e482d27684efc2f91 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 7 Feb 2025 13:46:11 +0100 Subject: [PATCH 7/7] ci: test new logic --- test/units/TEST-74-AUX-UTILS.mount.sh | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/units/TEST-74-AUX-UTILS.mount.sh b/test/units/TEST-74-AUX-UTILS.mount.sh index ecfdb0640f..85539452a0 100755 --- a/test/units/TEST-74-AUX-UTILS.mount.sh +++ b/test/units/TEST-74-AUX-UTILS.mount.sh @@ -39,3 +39,21 @@ systemd-mount --type=overlay --options="lowerdir=/etc,upperdir=$WORK_DIR/upper,w touch "$WORK_DIR/overlay/foo" test -e "$WORK_DIR/upper/foo" systemd-umount "$WORK_DIR/overlay" + +# Validate that we cannot mount through a symlink or .. +mkdir "$WORK_DIR"/flurb +ln -s flurb "$WORK_DIR"/knarb +systemd-mount --canonicalize=no --tmpfs "$WORK_DIR"/flurb/shlum +systemd-umount "$WORK_DIR/"/flurb/shlum +(! systemd-mount --canonicalize=no --tmpfs "$WORK_DIR"/knarb/shlum) +systemd-mount --canonicalize=yes --tmpfs "$WORK_DIR"/knarb/shlum +systemd-umount "$WORK_DIR/"/flurb/shlum +(! systemd-mount --canonicalize=no --tmpfs "$WORK_DIR"/flurb/../flurb/shlum) +systemd-mount --canonicalize=yes --tmpfs "$WORK_DIR"/flurb/../flurb/shlum +systemd-umount "$WORK_DIR/"/flurb/shlum + +# Validate that we can correctly create dir and reg files inodes if needed +systemd-mount --tmpfs "$WORK_DIR"/flurb/shlum/some/more/dirs +systemd-umount "$WORK_DIR/"/flurb/shlum/some/more/dirs +systemd-mount /bin/ls "$WORK_DIR"/flurb/shlum/some/more/dirs/file -o bind +systemd-umount "$WORK_DIR/"/flurb/shlum/some/more/dirs/file