From a75fcef8fb6d70711fabf23df44766c94285822d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 15 Mar 2019 11:05:26 +0100 Subject: [PATCH 1/7] shared/bootspec: avoid signed-unsigned comparison MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ../src/shared/bootspec.c: In function ‘find_sections’: ../src/shared/bootspec.c:425:23: warning: comparison of integer expressions of different signedness: ‘ssize_t’ {aka ‘int’} and ‘uint32_t’ {aka ‘unsigned int’} [-Wsign-compare] 425 | if (n != size) | ^~ --- src/shared/bootspec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/bootspec.c b/src/shared/bootspec.c index 59930464bc..af8da15236 100644 --- a/src/shared/bootspec.c +++ b/src/shared/bootspec.c @@ -422,7 +422,7 @@ static int find_sections( n = pread(fd, k, size, offset); if (n < 0) return log_error_errno(errno, "Failed to read section payload: %m"); - if (n != size) + if ((size_t) n != size) return log_error_errno(SYNTHETIC_ERRNO(EIO), "Short read while reading section payload, refusing:"); /* Allow one trailing NUL byte, but nothing more. */ From e55bdf9b6c5f72475b258a7a4585a0480551cb60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 15 Mar 2019 11:57:49 +0100 Subject: [PATCH 2/7] seccomp: shm{get,at,dt} now have their own numbers everywhere E.g. on i686: (previously) arch x86: SCMP_SYS(mmap) = 90 arch x86: SCMP_SYS(mmap2) = 192 arch x86: SCMP_SYS(shmat) = -221 arch x86: SCMP_SYS(shmat) = -221 arch x86: SCMP_SYS(shmdt) = -222 (now) arch x86: SCMP_SYS(mmap) = 90 arch x86: SCMP_SYS(mmap2) = 192 arch x86: SCMP_SYS(shmat) = 397 arch x86: SCMP_SYS(shmat) = 397 arch x86: SCMP_SYS(shmdt) = 398 The relevant commit seems to be https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0d6040d46817. --- src/shared/seccomp-util.c | 5 ----- src/test/test-seccomp.c | 9 +++++++++ 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/shared/seccomp-util.c b/src/shared/seccomp-util.c index 54f9679e16..714734ad61 100644 --- a/src/shared/seccomp-util.c +++ b/src/shared/seccomp-util.c @@ -1502,14 +1502,9 @@ static int add_seccomp_syscall_filter(scmp_filter_ctx seccomp, assert_cc(SCMP_SYS(shmget) > 0); assert_cc(SCMP_SYS(shmat) > 0); assert_cc(SCMP_SYS(shmdt) > 0); -#elif defined(__i386__) || defined(__powerpc64__) -assert_cc(SCMP_SYS(shmget) < 0); -assert_cc(SCMP_SYS(shmat) < 0); -assert_cc(SCMP_SYS(shmdt) < 0); #endif int seccomp_memory_deny_write_execute(void) { - uint32_t arch; int r; diff --git a/src/test/test-seccomp.c b/src/test/test-seccomp.c index d79481853a..937d414c0c 100644 --- a/src/test/test-seccomp.c +++ b/src/test/test-seccomp.c @@ -507,9 +507,18 @@ static void test_memory_deny_write_execute_mmap(void) { static void test_memory_deny_write_execute_shmat(void) { int shmid; pid_t pid; + uint32_t arch; log_info("/* %s */", __func__); + SECCOMP_FOREACH_LOCAL_ARCH(arch) { + log_debug("arch %s: SCMP_SYS(mmap) = %d", seccomp_arch_to_string(arch), SCMP_SYS(mmap)); + log_debug("arch %s: SCMP_SYS(mmap2) = %d", seccomp_arch_to_string(arch), SCMP_SYS(mmap2)); + log_debug("arch %s: SCMP_SYS(shmget) = %d", seccomp_arch_to_string(arch), SCMP_SYS(shmget)); + log_debug("arch %s: SCMP_SYS(shmat) = %d", seccomp_arch_to_string(arch), SCMP_SYS(shmat)); + log_debug("arch %s: SCMP_SYS(shmdt) = %d", seccomp_arch_to_string(arch), SCMP_SYS(shmdt)); + } + if (!is_seccomp_available()) { log_notice("Seccomp not available, skipping %s", __func__); return; From 67fb5f338fad27250eeed502dd7d3675514d1d63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 15 Mar 2019 12:46:56 +0100 Subject: [PATCH 3/7] seccomp: allow shmat to be a separate syscall on architectures which use a multiplexer After https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0d6040d46817, those syscalls have their separate numbers and we can block them. But glibc might still use the old ones. So let's just do a best-effort block and not assume anything about how effective it is. --- src/shared/seccomp-util.c | 3 ++- src/test/test-seccomp.c | 9 ++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/shared/seccomp-util.c b/src/shared/seccomp-util.c index 714734ad61..905be0f6a9 100644 --- a/src/shared/seccomp-util.c +++ b/src/shared/seccomp-util.c @@ -1519,6 +1519,7 @@ int seccomp_memory_deny_write_execute(void) { case SCMP_ARCH_X86: filter_syscall = SCMP_SYS(mmap2); block_syscall = SCMP_SYS(mmap); + shmat_syscall = SCMP_SYS(shmat); break; case SCMP_ARCH_PPC: @@ -1585,7 +1586,7 @@ int seccomp_memory_deny_write_execute(void) { continue; #endif - if (shmat_syscall != 0) { + if (shmat_syscall > 0) { r = add_seccomp_syscall_filter(seccomp, arch, SCMP_SYS(shmat), 1, SCMP_A2(SCMP_CMP_MASKED_EQ, SHM_EXEC, SHM_EXEC)); diff --git a/src/test/test-seccomp.c b/src/test/test-seccomp.c index 937d414c0c..14b37eed2c 100644 --- a/src/test/test-seccomp.c +++ b/src/test/test-seccomp.c @@ -548,15 +548,18 @@ static void test_memory_deny_write_execute_shmat(void) { assert_se(seccomp_memory_deny_write_execute() >= 0); p = shmat(shmid, NULL, SHM_EXEC); + log_debug_errno(p == MAP_FAILED ? errno : 0, "shmat(SHM_EXEC): %m"); #if defined(__x86_64__) || defined(__arm__) || defined(__aarch64__) assert_se(p == MAP_FAILED); assert_se(errno == EPERM); -#else /* __i386__, __powerpc64__, and "unknown" architectures */ - assert_se(p != MAP_FAILED); - assert_se(shmdt(p) == 0); #endif + /* Depending on kernel, libseccomp, and glibc versions, other architectures + * might fail or not. Let's not assert success. */ + if (p != MAP_FAILED) + assert_se(shmdt(p) == 0); p = shmat(shmid, NULL, 0); + log_debug_errno(p == MAP_FAILED ? errno : 0, "shmat(0): %m"); assert_se(p != MAP_FAILED); assert_se(shmdt(p) == 0); From 9efb96315ae502dabeb94ab35816ea8955563b7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 15 Mar 2019 13:42:55 +0100 Subject: [PATCH 4/7] test-execute: allow filtering test cases by pattern When debugging failure in one of the cases, it's annoying to have to wade through the output from all the other cases. Let's allow picking select cases. --- src/test/test-execute.c | 110 ++++++++++++++++++++++------------------ 1 file changed, 61 insertions(+), 49 deletions(-) diff --git a/src/test/test-execute.c b/src/test/test-execute.c index e5880baec6..4d44a28a31 100644 --- a/src/test/test-execute.c +++ b/src/test/test-execute.c @@ -696,8 +696,15 @@ static void test_exec_standardoutput_append(Manager *m) { test(m, "exec-standardoutput-append.service", 0, CLD_EXITED); } -static int run_tests(UnitFileScope scope, const test_function_t *tests) { - const test_function_t *test = NULL; +typedef struct test_entry { + test_function_t f; + const char *name; +} test_entry; + +#define entry(x) {x, #x} + +static int run_tests(UnitFileScope scope, const test_entry tests[], char **patterns) { + const test_entry *test = NULL; _cleanup_(manager_freep) Manager *m = NULL; int r; @@ -709,57 +716,62 @@ static int run_tests(UnitFileScope scope, const test_function_t *tests) { assert_se(r >= 0); assert_se(manager_startup(m, NULL, NULL) >= 0); - for (test = tests; test && *test; test++) - (*test)(m); + for (test = tests; test && test->f; test++) + if (strv_fnmatch_or_empty(patterns, test->name, FNM_NOESCAPE)) + test->f(m); + else + log_info("Skipping %s because it does not match any pattern.", test->name); return 0; } + int main(int argc, char *argv[]) { _cleanup_(rm_rf_physical_and_freep) char *runtime_dir = NULL; _cleanup_free_ char *test_execute_path = NULL; - static const test_function_t user_tests[] = { - test_exec_basic, - test_exec_ambientcapabilities, - test_exec_bindpaths, - test_exec_capabilityboundingset, - test_exec_cpuaffinity, - test_exec_environment, - test_exec_environmentfile, - test_exec_group, - test_exec_ignoresigpipe, - test_exec_inaccessiblepaths, - test_exec_ioschedulingclass, - test_exec_oomscoreadjust, - test_exec_passenvironment, - test_exec_personality, - test_exec_privatedevices, - test_exec_privatenetwork, - test_exec_privatetmp, - test_exec_protecthome, - test_exec_protectkernelmodules, - test_exec_readonlypaths, - test_exec_readwritepaths, - test_exec_restrictnamespaces, - test_exec_runtimedirectory, - test_exec_standardinput, - test_exec_standardoutput, - test_exec_standardoutput_append, - test_exec_supplementarygroups, - test_exec_systemcallerrornumber, - test_exec_systemcallfilter, - test_exec_temporaryfilesystem, - test_exec_umask, - test_exec_unsetenvironment, - test_exec_user, - test_exec_workingdirectory, - NULL, + + static const test_entry user_tests[] = { + entry(test_exec_basic), + entry(test_exec_ambientcapabilities), + entry(test_exec_bindpaths), + entry(test_exec_capabilityboundingset), + entry(test_exec_cpuaffinity), + entry(test_exec_environment), + entry(test_exec_environmentfile), + entry(test_exec_group), + entry(test_exec_ignoresigpipe), + entry(test_exec_inaccessiblepaths), + entry(test_exec_ioschedulingclass), + entry(test_exec_oomscoreadjust), + entry(test_exec_passenvironment), + entry(test_exec_personality), + entry(test_exec_privatedevices), + entry(test_exec_privatenetwork), + entry(test_exec_privatetmp), + entry(test_exec_protecthome), + entry(test_exec_protectkernelmodules), + entry(test_exec_readonlypaths), + entry(test_exec_readwritepaths), + entry(test_exec_restrictnamespaces), + entry(test_exec_runtimedirectory), + entry(test_exec_standardinput), + entry(test_exec_standardoutput), + entry(test_exec_standardoutput_append), + entry(test_exec_supplementarygroups), + entry(test_exec_systemcallerrornumber), + entry(test_exec_systemcallfilter), + entry(test_exec_temporaryfilesystem), + entry(test_exec_umask), + entry(test_exec_unsetenvironment), + entry(test_exec_user), + entry(test_exec_workingdirectory), + {}, }; - static const test_function_t system_tests[] = { - test_exec_dynamicuser, - test_exec_specifier, - test_exec_systemcallfilter_system, - NULL, + static const test_entry system_tests[] = { + entry(test_exec_dynamicuser), + entry(test_exec_specifier), + entry(test_exec_systemcallfilter_system), + {}, }; int r; @@ -801,11 +813,11 @@ int main(int argc, char *argv[]) { assert_se(unsetenv("VAR2") == 0); assert_se(unsetenv("VAR3") == 0); - r = run_tests(UNIT_FILE_USER, user_tests); + r = run_tests(UNIT_FILE_USER, user_tests, argv + 1); if (r != 0) return r; - r = run_tests(UNIT_FILE_SYSTEM, system_tests); + r = run_tests(UNIT_FILE_SYSTEM, system_tests, argv + 1); if (r != 0) return r; @@ -827,11 +839,11 @@ int main(int argc, char *argv[]) { can_unshare = false; - r = run_tests(UNIT_FILE_USER, user_tests); + r = run_tests(UNIT_FILE_USER, user_tests, argv + 1); if (r != 0) return r; - return run_tests(UNIT_FILE_SYSTEM, system_tests); + return run_tests(UNIT_FILE_SYSTEM, system_tests, argv + 1); #else return 0; #endif From 6a461d1f59850ff27bd254a3b71fe9ade0523e76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 15 Mar 2019 15:13:25 +0100 Subject: [PATCH 5/7] basic/fd-util: refuse "infinite" loop in close_all_fds() I had a test machine with ulimit -n set to 1073741816 through pam ("session required pam_limits.so set_all", which copies the limits from PID 1, left over from testing of #10921). test-execute would "hang" and then fail with a timeout when running exec-inaccessiblepaths-proc.service. It turns out that the problem was in close_all_fds(), which would go to the fallback path of doing close() 1073741813 times. Let's just fail if we hit this case. This only matters for cases where both /proc is inaccessible, and the *soft* limit has been raised. (gdb) bt #0 0x00007f7e2e73fdc8 in close () from target:/lib64/libc.so.6 #1 0x00007f7e2e42cdfd in close_nointr () from target:/home/zbyszek/src/systemd-work3/build-rawhide/src/shared/libsystemd-shared-241.so #2 0x00007f7e2e42d525 in close_all_fds () from target:/home/zbyszek/src/systemd-work3/build-rawhide/src/shared/libsystemd-shared-241.so #3 0x0000000000426e53 in exec_child () #4 0x0000000000429578 in exec_spawn () #5 0x00000000004ce1ab in service_spawn () #6 0x00000000004cff77 in service_enter_start () #7 0x00000000004d028f in service_enter_start_pre () #8 0x00000000004d16f2 in service_start () #9 0x00000000004568f4 in unit_start () #10 0x0000000000416987 in test () #11 0x0000000000417632 in test_exec_inaccessiblepaths () #12 0x0000000000419362 in run_tests () #13 0x0000000000419632 in main () --- src/basic/fd-util.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index 3e6ef5a06a..b3b840145f 100644 --- a/src/basic/fd-util.c +++ b/src/basic/fd-util.c @@ -25,6 +25,10 @@ #include "util.h" #include "tmpfile-util.h" +/* The maximum number of iterations in the loop to close descriptors in the fallback case + * when /proc/self/fd/ is inaccessible. */ +#define MAX_FD_LOOP_LIMIT (1024*1024) + int close_nointr(int fd) { assert(fd >= 0); @@ -228,6 +232,13 @@ int close_all_fds(const int except[], size_t n_except) { if (max_fd < 0) return max_fd; + /* Refuse to do the loop over more too many elements. It's better to fail immediately than to + * spin the CPU for a long time. */ + if (max_fd > MAX_FD_LOOP_LIMIT) + return log_debug_errno(SYNTHETIC_ERRNO(EPERM), + "/proc/self/fd is inaccessible. Refusing to loop over %d potential fds.", + max_fd); + for (fd = 3; fd >= 0; fd = fd < max_fd ? fd + 1 : -1) { int q; From 054d871d41039fcfc1a4a661c979941b9660c9e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 15 Mar 2019 15:27:32 +0100 Subject: [PATCH 6/7] test-execute: block /sys not /proc As explained in the previous commit, blocking /proc can cause us to go into a long loop or fail the test. --- src/test/test-execute.c | 2 +- test/meson.build | 2 +- ...epaths-proc.service => exec-inaccessiblepaths-sys.service} | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) rename test/test-execute/{exec-inaccessiblepaths-proc.service => exec-inaccessiblepaths-sys.service} (63%) diff --git a/src/test/test-execute.c b/src/test/test-execute.c index 4d44a28a31..f59d267776 100644 --- a/src/test/test-execute.c +++ b/src/test/test-execute.c @@ -327,7 +327,7 @@ static void test_exec_inaccessiblepaths(Manager *m) { return; } - test(m, "exec-inaccessiblepaths-proc.service", can_unshare ? 0 : EXIT_FAILURE, CLD_EXITED); + test(m, "exec-inaccessiblepaths-sys.service", can_unshare ? 0 : EXIT_FAILURE, CLD_EXITED); if (path_is_read_only_fs("/") > 0) { log_notice("Root directory is readonly, skipping remaining tests in %s", __func__); diff --git a/test/meson.build b/test/meson.build index 870f8bc6a7..7255c79dcb 100644 --- a/test/meson.build +++ b/test/meson.build @@ -67,7 +67,7 @@ test_data_files = ''' test-execute/exec-ignoresigpipe-no.service test-execute/exec-ignoresigpipe-yes.service test-execute/exec-inaccessiblepaths-mount-propagation.service - test-execute/exec-inaccessiblepaths-proc.service + test-execute/exec-inaccessiblepaths-sys.service test-execute/exec-ioschedulingclass-best-effort.service test-execute/exec-ioschedulingclass-idle.service test-execute/exec-ioschedulingclass-none.service diff --git a/test/test-execute/exec-inaccessiblepaths-proc.service b/test/test-execute/exec-inaccessiblepaths-sys.service similarity index 63% rename from test/test-execute/exec-inaccessiblepaths-proc.service rename to test/test-execute/exec-inaccessiblepaths-sys.service index ebdb4843d1..5ff6f3605b 100644 --- a/test/test-execute/exec-inaccessiblepaths-proc.service +++ b/test/test-execute/exec-inaccessiblepaths-sys.service @@ -2,6 +2,6 @@ Description=Test to make sure that mount namespace setup works properly with the 'InaccessiblePaths=/proc' option [Service] -InaccessiblePaths=/proc -ExecStart=/bin/sh -x -c 'test "$$(stat -c %%a /proc)" = "0"' +InaccessiblePaths=/sys +ExecStart=/bin/sh -x -c 'test "$$(stat -c %%a /sys)" = "0"' Type=oneshot From 7acf581a58ba7d4efa65b86f5a94f00a993b16a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 15 Mar 2019 15:35:49 +0100 Subject: [PATCH 7/7] Handle or voidify all calls to close_all_fds() In activate, it is important that we close the fds. In other cases, meh. --- src/activate/activate.c | 4 +++- src/basic/process-util.c | 2 +- src/nspawn/nspawn-setuid.c | 2 +- src/nspawn/nspawn-stub-pid1.c | 2 +- 4 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/activate/activate.c b/src/activate/activate.c index 7eae9e22c2..35ab08f22d 100644 --- a/src/activate/activate.c +++ b/src/activate/activate.c @@ -74,7 +74,9 @@ static int open_sockets(int *epoll_fd, bool accept) { except[fd] = fd; log_close(); - close_all_fds(except, 3 + n); + r = close_all_fds(except, 3 + n); + if (r < 0) + return log_error_errno(r, "Failed to close all file descriptors: %m"); } /** Note: we leak some fd's on error here. I doesn't matter diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 86d5c3adb5..c41a2aa5c9 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -1001,7 +1001,7 @@ _noreturn_ void freeze(void) { log_close(); /* Make sure nobody waits for us on a socket anymore */ - close_all_fds(NULL, 0); + (void) close_all_fds(NULL, 0); sync(); diff --git a/src/nspawn/nspawn-setuid.c b/src/nspawn/nspawn-setuid.c index 0026e4e3fc..f207bcec04 100644 --- a/src/nspawn/nspawn-setuid.c +++ b/src/nspawn/nspawn-setuid.c @@ -43,7 +43,7 @@ static int spawn_getent(const char *database, const char *key, pid_t *rpid) { if (rearrange_stdio(-1, pipe_fds[1], -1) < 0) _exit(EXIT_FAILURE); - close_all_fds(NULL, 0); + (void) close_all_fds(NULL, 0); (void) rlimit_nofile_safe(); diff --git a/src/nspawn/nspawn-stub-pid1.c b/src/nspawn/nspawn-stub-pid1.c index 5d17df39d9..ebf4f0f523 100644 --- a/src/nspawn/nspawn-stub-pid1.c +++ b/src/nspawn/nspawn-stub-pid1.c @@ -67,7 +67,7 @@ int stub_pid1(sd_id128_t uuid) { reset_all_signal_handlers(); log_close(); - close_all_fds(NULL, 0); + (void) close_all_fds(NULL, 0); log_open(); /* Flush out /proc/self/environ, so that we don't leak the environment from the host into the container. Also,