From c0d76959080e3ba6534bce1c7053c750668e8b55 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 16 Jan 2019 14:47:09 +0100 Subject: [PATCH 1/6] main: when bumping RLIMIT_MEMLOCK, save the previous value to pass to children Let's make sure that the bumping of RLIMIT_MEMLOCK does not leak into our children. --- src/core/main.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/core/main.c b/src/core/main.c index d0d589e26a..79f12c9209 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1327,6 +1327,17 @@ static int bump_rlimit_memlock(struct rlimit *saved_rlimit) { if (getrlimit(RLIMIT_MEMLOCK, saved_rlimit) < 0) return log_warning_errno(errno, "Reading RLIMIT_MEMLOCK failed, ignoring: %m"); + /* Pass the original value down to invoked processes */ + if (!arg_default_rlimit[RLIMIT_MEMLOCK]) { + struct rlimit *rl; + + rl = newdup(struct rlimit, saved_rlimit, 1); + if (!rl) + return log_oom(); + + arg_default_rlimit[RLIMIT_MEMLOCK] = rl; + } + r = setrlimit_closest(RLIMIT_MEMLOCK, &RLIMIT_MAKE_CONST(HIGH_RLIMIT_MEMLOCK)); if (r < 0) return log_warning_errno(r, "Setting RLIMIT_MEMLOCK failed, ignoring: %m"); From ddfa8b0b3bed5f5e8435a02cc6b5afc1d536cec6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 16 Jan 2019 14:50:03 +0100 Subject: [PATCH 2/6] main: add commenting, clean up handling of saved resource limits This doesn't really change behaviour, but adds comments and uses more symbolic names for everything, to make this more readable. --- src/core/main.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 79f12c9209..3a87c296cb 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1675,12 +1675,11 @@ static void do_reexecute( * we do that */ watchdog_close(true); - /* Reset the RLIMIT_NOFILE to the kernel default, so that the new systemd can pass the kernel default to its - * child processes */ - - if (saved_rlimit_nofile->rlim_cur > 0) + /* Reset RLIMIT_NOFILE + RLIMIT_MEMLOCK back to the kernel defaults, so that the new systemd can pass + * the kernel default to its child processes */ + if (saved_rlimit_nofile->rlim_cur != 0) (void) setrlimit(RLIMIT_NOFILE, saved_rlimit_nofile); - if (saved_rlimit_memlock->rlim_cur != (rlim_t) -1) + if (saved_rlimit_memlock->rlim_cur != RLIM_INFINITY) (void) setrlimit(RLIMIT_MEMLOCK, saved_rlimit_memlock); if (switch_root_dir) { @@ -2313,7 +2312,11 @@ int main(int argc, char *argv[]) { dual_timestamp initrd_timestamp = DUAL_TIMESTAMP_NULL, userspace_timestamp = DUAL_TIMESTAMP_NULL, kernel_timestamp = DUAL_TIMESTAMP_NULL, security_start_timestamp = DUAL_TIMESTAMP_NULL, security_finish_timestamp = DUAL_TIMESTAMP_NULL; - struct rlimit saved_rlimit_nofile = RLIMIT_MAKE_CONST(0), saved_rlimit_memlock = RLIMIT_MAKE_CONST((rlim_t) -1); + struct rlimit saved_rlimit_nofile = RLIMIT_MAKE_CONST(0), + saved_rlimit_memlock = RLIMIT_MAKE_CONST(RLIM_INFINITY); /* The original rlimits we passed + * in. Note we use different values + * for the two that indicate whether + * these fields are initialized! */ bool skip_setup, loaded_policy = false, queue_default_job = false, first_boot = false, reexecute = false; char *switch_root_dir = NULL, *switch_root_init = NULL; usec_t before_startup, after_startup; From cda7faa9a5ae2fa1ebc27b08e84d5ce62e46e37b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 16 Jan 2019 18:05:14 +0100 Subject: [PATCH 3/6] main: don't bump resource limits if they are higher than we need them anyway This matters in particular in the case of --user, since there we lack the privs to bump the limits up again later on when invoking children. --- src/core/main.c | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 3a87c296cb..1b81542e8c 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1269,6 +1269,7 @@ static void bump_file_max_and_nr_open(void) { } static int bump_rlimit_nofile(struct rlimit *saved_rlimit) { + struct rlimit new_rlimit; int r, nr; assert(saved_rlimit); @@ -1306,9 +1307,22 @@ static int bump_rlimit_nofile(struct rlimit *saved_rlimit) { arg_default_rlimit[RLIMIT_NOFILE] = rl; } + /* Calculate the new limits to use for us. Never lower from what we inherited. */ + new_rlimit = (struct rlimit) { + .rlim_cur = MAX((rlim_t) nr, saved_rlimit->rlim_cur), + .rlim_max = MAX((rlim_t) nr, saved_rlimit->rlim_max), + }; + + /* Shortcut if nothing changes. */ + if (saved_rlimit->rlim_max >= new_rlimit.rlim_max && + saved_rlimit->rlim_cur >= new_rlimit.rlim_cur) { + log_debug("RLIMIT_NOFILE is already as high or higher than we need it, not bumping."); + return 0; + } + /* Bump up the resource limit for ourselves substantially, all the way to the maximum the kernel allows, for * both hard and soft. */ - r = setrlimit_closest(RLIMIT_NOFILE, &RLIMIT_MAKE_CONST(nr)); + r = setrlimit_closest(RLIMIT_NOFILE, &new_rlimit); if (r < 0) return log_warning_errno(r, "Setting RLIMIT_NOFILE failed, ignoring: %m"); @@ -1316,6 +1330,7 @@ static int bump_rlimit_nofile(struct rlimit *saved_rlimit) { } static int bump_rlimit_memlock(struct rlimit *saved_rlimit) { + struct rlimit new_rlimit; int r; assert(saved_rlimit); @@ -1338,7 +1353,22 @@ static int bump_rlimit_memlock(struct rlimit *saved_rlimit) { arg_default_rlimit[RLIMIT_MEMLOCK] = rl; } - r = setrlimit_closest(RLIMIT_MEMLOCK, &RLIMIT_MAKE_CONST(HIGH_RLIMIT_MEMLOCK)); + /* Using MAX() on resource limits only is safe if RLIM_INFINITY is > 0. POSIX declares that rlim_t + * must be unsigned, hence this is a given, but let's make this clear here. */ + assert_cc(RLIM_INFINITY > 0); + + new_rlimit = (struct rlimit) { + .rlim_cur = MAX(HIGH_RLIMIT_MEMLOCK, saved_rlimit->rlim_cur), + .rlim_max = MAX(HIGH_RLIMIT_MEMLOCK, saved_rlimit->rlim_max), + }; + + if (saved_rlimit->rlim_max >= new_rlimit.rlim_cur && + saved_rlimit->rlim_cur >= new_rlimit.rlim_max) { + log_debug("RLIMIT_MEMLOCK is already as high or higher than we need it, not bumping."); + return 0; + } + + r = setrlimit_closest(RLIMIT_MEMLOCK, &new_rlimit); if (r < 0) return log_warning_errno(r, "Setting RLIMIT_MEMLOCK failed, ignoring: %m"); From 99a2fd3bcaa123d38a2d757cb3693e31e138e56b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 16 Jan 2019 18:06:18 +0100 Subject: [PATCH 4/6] main: when generating the resource limit to pass to children, take FD_SETSIZE into consideration When we synthesize a "struct rlimit" structure to pass on for RLIMIT_NOFILE to our children, let's explicitly make sure that the soft limit is not above FD_SETSIZE, for compat reason with select(). Note this only applies when we derive the "struct rlimit" from what we inherited. If the user configures something explicitly it always takes precedence. --- src/core/main.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/core/main.c b/src/core/main.c index 1b81542e8c..561f956f0a 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1304,6 +1304,11 @@ static int bump_rlimit_nofile(struct rlimit *saved_rlimit) { if (arg_system) rl->rlim_max = MIN((rlim_t) nr, MAX(rl->rlim_max, (rlim_t) HIGH_RLIMIT_NOFILE)); + /* If for some reason we were invoked with a soft limit above 1024 (which should never + * happen!, but who knows what we get passed in from pam_limit when invoked as --user + * instance), then lower what we pass on to not confuse our children */ + rl->rlim_cur = MIN(rl->rlim_cur, (rlim_t) FD_SETSIZE); + arg_default_rlimit[RLIMIT_NOFILE] = rl; } From 498e265df1c63212ec1a0991c135877a23f1ba4f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Jan 2019 12:23:21 +0100 Subject: [PATCH 5/6] fd-util: rework how we determine highest possible fd --- src/basic/fd-util.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index 19d6a37197..3e6ef5a06a 100644 --- a/src/basic/fd-util.c +++ b/src/basic/fd-util.c @@ -189,6 +189,27 @@ _pure_ static bool fd_in_set(int fd, const int fdset[], size_t n_fdset) { return false; } +static int get_max_fd(void) { + struct rlimit rl; + rlim_t m; + + /* Return the highest possible fd, based RLIMIT_NOFILE, but enforcing FD_SETSIZE-1 as lower boundary + * and INT_MAX as upper boundary. */ + + if (getrlimit(RLIMIT_NOFILE, &rl) < 0) + return -errno; + + m = MAX(rl.rlim_cur, rl.rlim_max); + if (m < FD_SETSIZE) /* Let's always cover at least 1024 fds */ + return FD_SETSIZE-1; + + if (m == RLIM_INFINITY || m > INT_MAX) /* Saturate on overflow. After all fds are "int", hence can + * never be above INT_MAX */ + return INT_MAX; + + return (int) (m - 1); +} + int close_all_fds(const int except[], size_t n_except) { _cleanup_closedir_ DIR *d = NULL; struct dirent *de; @@ -198,20 +219,14 @@ int close_all_fds(const int except[], size_t n_except) { d = opendir("/proc/self/fd"); if (!d) { - struct rlimit rl; int fd, max_fd; - /* When /proc isn't available (for example in chroots) the fallback is brute forcing through the fd - * table */ + /* When /proc isn't available (for example in chroots) the fallback is brute forcing through + * the fd table */ - assert_se(getrlimit(RLIMIT_NOFILE, &rl) >= 0); - - if (rl.rlim_max == 0) - return -EINVAL; - - /* Let's take special care if the resource limit is set to unlimited, or actually larger than the range - * of 'int'. Let's avoid implicit overflows. */ - max_fd = (rl.rlim_max == RLIM_INFINITY || rl.rlim_max > INT_MAX) ? INT_MAX : (int) (rl.rlim_max - 1); + max_fd = get_max_fd(); + if (max_fd < 0) + return max_fd; for (fd = 3; fd >= 0; fd = fd < max_fd ? fd + 1 : -1) { int q; From ce932d2d331886190dca9d54571d3d99d9fe8cfc Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Jan 2019 12:24:14 +0100 Subject: [PATCH 6/6] execute: make sure to call into PAM after initializing resource limits We want that pam_limits takes precedence over our settings, after all. Fixes: #11386 --- src/core/execute.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 61861f9416..07c5a72721 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -3226,7 +3226,24 @@ static int exec_child( #endif } + if (needs_sandboxing) { + int which_failed; + + /* Let's set the resource limits before we call into PAM, so that pam_limits wins over what + * is set here. (See below.) */ + + r = setrlimit_closest_all((const struct rlimit* const *) context->rlimit, &which_failed); + if (r < 0) { + *exit_status = EXIT_LIMITS; + return log_unit_error_errno(unit, r, "Failed to adjust resource limit RLIMIT_%s: %m", rlimit_to_string(which_failed)); + } + } + if (needs_setuid) { + + /* Let's call into PAM after we set up our own idea of resource limits to that pam_limits + * wins here. (See above.) */ + if (context->pam_name && username) { r = setup_pam(context->pam_name, username, uid, gid, context->tty_path, &accum_env, fds, n_fds); if (r < 0) { @@ -3343,15 +3360,10 @@ static int exec_child( if (needs_sandboxing) { uint64_t bset; - int which_failed; - r = setrlimit_closest_all((const struct rlimit* const *) context->rlimit, &which_failed); - if (r < 0) { - *exit_status = EXIT_LIMITS; - return log_unit_error_errno(unit, r, "Failed to adjust resource limit RLIMIT_%s: %m", rlimit_to_string(which_failed)); - } - - /* Set the RTPRIO resource limit to 0, but only if nothing else was explicitly requested. */ + /* Set the RTPRIO resource limit to 0, but only if nothing else was explicitly + * requested. (Note this is placed after the general resource limit initialization, see + * above, in order to take precedence.) */ if (context->restrict_realtime && !context->rlimit[RLIMIT_RTPRIO]) { if (setrlimit(RLIMIT_RTPRIO, &RLIMIT_MAKE_CONST(0)) < 0) { *exit_status = EXIT_LIMITS;