From a3e8e154807f44eeb7df2bb3e4301006862fac3b Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 30 Nov 2023 19:13:12 +0800 Subject: [PATCH 1/5] core/execute-serialize: FOREACH_ARRAY at one more place --- src/core/execute-serialize.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/core/execute-serialize.c b/src/core/execute-serialize.c index 6c19cd42a2..a874a1eb14 100644 --- a/src/core/execute-serialize.c +++ b/src/core/execute-serialize.c @@ -373,8 +373,7 @@ static int exec_cgroup_context_serialize(const CGroupContext *c, FILE *f) { if (il->limits[type] == cgroup_io_limit_defaults[type]) continue; - key = strjoin("exec-cgroup-context-io-device-limit-", - cgroup_io_limit_type_to_string(type)); + key = strjoin("exec-cgroup-context-io-device-limit-", cgroup_io_limit_type_to_string(type)); if (!key) return -ENOMEM; @@ -1479,8 +1478,8 @@ static int exec_parameters_deserialize(ExecParameters *p, FILE *f, FDSet *fds) { return log_oom_debug(); /* Ensure we don't leave any FD uninitialized on error, it makes the fuzzer sad */ - for (size_t i = 0; i < p->n_socket_fds + p->n_storage_fds; ++i) - p->fds[i] = -EBADF; + FOREACH_ARRAY(i, p->fds, p->n_socket_fds + p->n_storage_fds) + *i = -EBADF; r = deserialize_fd_many(fds, val, p->n_socket_fds + p->n_storage_fds, p->fds); if (r < 0) From d8da25b5d9c9251baf64cff1c8612a465d7b5415 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 30 Nov 2023 19:16:49 +0800 Subject: [PATCH 2/5] core/exec-invoke: rename flags_fds to flag_fds --- src/core/exec-invoke.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index 1e08296b46..a5298329b0 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -105,7 +105,7 @@ static int shift_fds(int fds[], size_t n_fds) { return 0; } -static int flags_fds( +static int flag_fds( const int fds[], size_t n_socket_fds, size_t n_fds, @@ -113,10 +113,7 @@ static int flags_fds( int r; - if (n_fds <= 0) - return 0; - - assert(fds); + assert(fds || n_fds == 0); /* Drops/Sets O_NONBLOCK and FD_CLOEXEC from the file flags. * O_NONBLOCK only applies to socket activation though. */ @@ -4807,7 +4804,7 @@ int exec_invoke( if (r >= 0) r = shift_fds(fds, n_fds); if (r >= 0) - r = flags_fds(fds, n_socket_fds, n_fds, context->non_blocking); + r = flag_fds(fds, n_socket_fds, n_fds, context->non_blocking); if (r < 0) { *exit_status = EXIT_FDS; return log_exec_error_errno(context, params, r, "Failed to adjust passed file descriptors: %m"); From a2467ea894b37b0861b92e35edd93788f8e2a342 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Fri, 1 Dec 2023 00:00:27 +0800 Subject: [PATCH 3/5] fdset: set all collected fds to CLOEXEC in fdset_new_fill() --- src/core/main.c | 2 -- src/notify/notify.c | 4 ---- src/shared/fdset.c | 12 +++++++++++- src/test/test-fdset.c | 5 ++++- 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 2ac59dabf5..3f71cc0947 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -2739,8 +2739,6 @@ static int collect_fds(FDSet **ret_fds, const char **ret_error_message) { "MESSAGE_ID=" SD_MESSAGE_CORE_FD_SET_FAILED_STR); } - (void) fdset_cloexec(*ret_fds, true); - /* The serialization fd should have O_CLOEXEC turned on already, let's verify that we didn't pick it up here */ assert_se(!arg_serialization || !fdset_contains(*ret_fds, fileno(arg_serialization))); diff --git a/src/notify/notify.c b/src/notify/notify.c index 675fbda752..f63ec8b355 100644 --- a/src/notify/notify.c +++ b/src/notify/notify.c @@ -225,10 +225,6 @@ static int parse_argv(int argc, char *argv[]) { r = fdset_new_fill(/* filter_cloexec= */ 0, &passed); if (r < 0) return log_error_errno(r, "Failed to take possession of passed file descriptors: %m"); - - r = fdset_cloexec(passed, true); - if (r < 0) - return log_error_errno(r, "Failed to enable O_CLOEXEC for passed file descriptors: %m"); } if (fdnr < 3) { diff --git a/src/shared/fdset.c b/src/shared/fdset.c index b62f15c649..e5b8e92e80 100644 --- a/src/shared/fdset.c +++ b/src/shared/fdset.c @@ -150,13 +150,15 @@ int fdset_remove(FDSet *s, int fd) { int fdset_new_fill( int filter_cloexec, /* if < 0 takes all fds, otherwise only those with O_CLOEXEC set (1) or unset (0) */ FDSet **ret) { + _cleanup_(fdset_shallow_freep) FDSet *s = NULL; _cleanup_closedir_ DIR *d = NULL; int r; assert(ret); - /* Creates an fdset and fills in all currently open file descriptors. */ + /* Creates an fdset and fills in all currently open file descriptors. Also set all collected fds + * to CLOEXEC. */ d = opendir("/proc/self/fd"); if (!d) { @@ -191,6 +193,7 @@ int fdset_new_fill( /* If user asked for that filter by O_CLOEXEC. This is useful so that fds that have * been passed in can be collected and fds which have been created locally can be * ignored, under the assumption that only the latter have O_CLOEXEC set. */ + fl = fcntl(fd, F_GETFD); if (fl < 0) return -errno; @@ -199,6 +202,13 @@ int fdset_new_fill( continue; } + /* We need to set CLOEXEC manually only if we're collecting non-CLOEXEC fds. */ + if (filter_cloexec <= 0) { + r = fd_cloexec(fd, true); + if (r < 0) + return r; + } + r = fdset_put(s, fd); if (r < 0) return r; diff --git a/src/test/test-fdset.c b/src/test/test-fdset.c index 8947a319b6..8f00e598fd 100644 --- a/src/test/test-fdset.c +++ b/src/test/test-fdset.c @@ -11,8 +11,8 @@ #include "tmpfile-util.h" TEST(fdset_new_fill) { - int fd = -EBADF; _cleanup_fdset_free_ FDSet *fdset = NULL; + int fd = -EBADF, flags; log_close(); log_set_open_when_needed(true); @@ -50,6 +50,9 @@ TEST(fdset_new_fill) { assert_se(fdset_new_fill(/* filter_cloexec= */ 0, &fdset) >= 0); assert_se(fdset_contains(fdset, fd)); + flags = fcntl(fd, F_GETFD); + assert_se(flags >= 0); + assert_se(FLAGS_SET(flags, FD_CLOEXEC)); fdset = fdset_free(fdset); assert_se(fcntl(fd, F_GETFD) < 0); assert_se(errno == EBADF); From f38cbaff63e662c8a1aa0c7708e0e796d6c3aee2 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 30 Nov 2023 19:24:01 +0800 Subject: [PATCH 4/5] core/exec-invoke: remove redundant fd_cloexec() call --- src/core/execute-serialize.c | 12 ------------ src/core/executor.c | 4 +++- 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/src/core/execute-serialize.c b/src/core/execute-serialize.c index a874a1eb14..b67a4f9141 100644 --- a/src/core/execute-serialize.c +++ b/src/core/execute-serialize.c @@ -1610,12 +1610,6 @@ static int exec_parameters_deserialize(ExecParameters *p, FILE *f, FDSet *fds) { if (fd < 0) continue; - /* This is special and relies on close-on-exec semantics, make sure it's - * there */ - r = fd_cloexec(fd, true); - if (r < 0) - return r; - p->exec_fd = fd; } else if ((val = startswith(l, "exec-parameters-bpf-outer-map-fd="))) { int fd; @@ -1624,12 +1618,6 @@ static int exec_parameters_deserialize(ExecParameters *p, FILE *f, FDSet *fds) { if (fd < 0) continue; - /* This is special and relies on close-on-exec semantics, make sure it's - * there */ - r = fd_cloexec(fd, true); - if (r < 0) - return r; - p->bpf_outer_map_fd = fd; } else if ((val = startswith(l, "exec-parameters-notify-socket="))) { r = free_and_strdup(&p->notify_socket, val); diff --git a/src/core/executor.c b/src/core/executor.c index f55bacdbd8..993cd4a4d2 100644 --- a/src/core/executor.c +++ b/src/core/executor.c @@ -204,7 +204,9 @@ int main(int argc, char *argv[]) { log_set_prohibit_ipc(false); log_open(); - /* The serialization fd is set to CLOEXEC in parse_argv, so it's also filtered. */ + /* This call would collect all passed fds and enable CLOEXEC. We'll unset it in exec_invoke (flag_fds) + * for fds that shall be passed to the child. + * The serialization fd is set to CLOEXEC in parse_argv, so it's also filtered. */ r = fdset_new_fill(/* filter_cloexec= */ 0, &fdset); if (r < 0) return log_error_errno(r, "Failed to create fd set: %m"); From 5a5fdfe3ac27914e0487f0a7c506882227991ec5 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 30 Nov 2023 20:09:29 +0800 Subject: [PATCH 5/5] core/exec-invoke: prevent potential double-close of exec_fd If exec_fd is closed in add_shifted_fd() by close_and_replace(), but something goes wrong later, we may close exec_fd twice in exec_params_shallow_clear(). --- src/core/exec-invoke.c | 47 +++++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 26 deletions(-) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index a5298329b0..ffb9db1d58 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -3601,32 +3601,29 @@ static int exec_context_cpu_affinity_from_numa(const ExecContext *c, CPUSet *ret return cpu_set_add_all(ret, &s); } -static int add_shifted_fd(int *fds, size_t fds_size, size_t *n_fds, int fd, int *ret_fd) { +static int add_shifted_fd(int *fds, size_t fds_size, size_t *n_fds, int *fd) { int r; assert(fds); assert(n_fds); assert(*n_fds < fds_size); - assert(ret_fd); + assert(fd); - if (fd < 0) { - *ret_fd = -EBADF; - return 0; - } + if (*fd < 0) + return 0; - if (fd < 3 + (int) *n_fds) { + if (*fd < 3 + (int) *n_fds) { /* Let's move the fd up, so that it's outside of the fd range we will use to store * the fds we pass to the process (or which are closed only during execve). */ - r = fcntl(fd, F_DUPFD_CLOEXEC, 3 + (int) *n_fds); + r = fcntl(*fd, F_DUPFD_CLOEXEC, 3 + (int) *n_fds); if (r < 0) return -errno; - close_and_replace(fd, r); + close_and_replace(*fd, r); } - *ret_fd = fds[*n_fds] = fd; - (*n_fds) ++; + fds[(*n_fds)++] = *fd; return 1; } @@ -3926,7 +3923,7 @@ int exec_invoke( int *exit_status) { _cleanup_strv_free_ char **our_env = NULL, **pass_env = NULL, **joined_exec_search_path = NULL, **accum_env = NULL, **replaced_argv = NULL; - int r, ngids = 0, exec_fd; + int r, ngids = 0; _cleanup_free_ gid_t *supplementary_gids = NULL; const char *username = NULL, *groupname = NULL; _cleanup_free_ char *home_buffer = NULL, *memory_pressure_path = NULL; @@ -4063,19 +4060,17 @@ int exec_invoke( memcpy_safe(keep_fds, fds, n_fds * sizeof(int)); n_keep_fds = n_fds; - r = add_shifted_fd(keep_fds, ELEMENTSOF(keep_fds), &n_keep_fds, params->exec_fd, &exec_fd); + r = add_shifted_fd(keep_fds, ELEMENTSOF(keep_fds), &n_keep_fds, ¶ms->exec_fd); if (r < 0) { *exit_status = EXIT_FDS; - return log_exec_error_errno(context, params, r, "Failed to shift fd and set FD_CLOEXEC: %m"); + return log_exec_error_errno(context, params, r, "Failed to collect shifted fd: %m"); } #if HAVE_LIBBPF - if (params->bpf_outer_map_fd >= 0) { - r = add_shifted_fd(keep_fds, ELEMENTSOF(keep_fds), &n_keep_fds, params->bpf_outer_map_fd, (int *)¶ms->bpf_outer_map_fd); - if (r < 0) { - *exit_status = EXIT_FDS; - return log_exec_error_errno(context, params, r, "Failed to shift fd and set FD_CLOEXEC: %m"); - } + r = add_shifted_fd(keep_fds, ELEMENTSOF(keep_fds), &n_keep_fds, ¶ms->bpf_outer_map_fd); + if (r < 0) { + *exit_status = EXIT_FDS; + return log_exec_error_errno(context, params, r, "Failed to collect shifted fd: %m"); } #endif @@ -4756,10 +4751,10 @@ int exec_invoke( "EXECUTABLE=%s", command->path); } - r = add_shifted_fd(keep_fds, ELEMENTSOF(keep_fds), &n_keep_fds, executable_fd, &executable_fd); + r = add_shifted_fd(keep_fds, ELEMENTSOF(keep_fds), &n_keep_fds, &executable_fd); if (r < 0) { *exit_status = EXIT_FDS; - return log_exec_error_errno(context, params, r, "Failed to shift fd and set FD_CLOEXEC: %m"); + return log_exec_error_errno(context, params, r, "Failed to collect shifted fd: %m"); } #if HAVE_SELINUX @@ -5207,13 +5202,13 @@ int exec_invoke( log_command_line(context, params, "Executing", executable, final_argv); - if (exec_fd >= 0) { + if (params->exec_fd >= 0) { uint8_t hot = 1; /* We have finished with all our initializations. Let's now let the manager know that. From this point * on, if the manager sees POLLHUP on the exec_fd, then execve() was successful. */ - if (write(exec_fd, &hot, sizeof(hot)) < 0) { + if (write(params->exec_fd, &hot, sizeof(hot)) < 0) { *exit_status = EXIT_EXEC; return log_exec_error_errno(context, params, errno, "Failed to enable exec_fd: %m"); } @@ -5221,13 +5216,13 @@ int exec_invoke( r = fexecve_or_execve(executable_fd, executable, final_argv, accum_env); - if (exec_fd >= 0) { + if (params->exec_fd >= 0) { uint8_t hot = 0; /* The execve() failed. This means the exec_fd is still open. Which means we need to tell the manager * that POLLHUP on it no longer means execve() succeeded. */ - if (write(exec_fd, &hot, sizeof(hot)) < 0) { + if (write(params->exec_fd, &hot, sizeof(hot)) < 0) { *exit_status = EXIT_EXEC; return log_exec_error_errno(context, params, errno, "Failed to disable exec_fd: %m"); }