From 21022b9dded0baa21f7715625fbc24db9aebebde Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 8 Sep 2017 16:16:29 +0200 Subject: [PATCH] util-lib: wrap personality() to fix up broken glibc error handling (#6766) glibc appears to propagate different errors in different ways, let's fix this up, so that our own code doesn't get confused by this. See #6752 + #6737 for details. Fixes: #6755 --- src/basic/process-util.c | 26 +++++++++++++++++-- src/basic/process-util.h | 1 + src/core/execute.c | 8 +++--- src/nspawn/nspawn.c | 10 +++++--- src/test/test-seccomp.c | 67 +++++++++++++----------------------------------- 5 files changed, 54 insertions(+), 58 deletions(-) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index e4cde65..9d01541 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -904,6 +904,28 @@ const char* personality_to_string(unsigned long p) { return architecture_to_string(architecture); } +int safe_personality(unsigned long p) { + int ret; + + /* So here's the deal, personality() is weirdly defined by glibc. In some cases it returns a failure via errno, + * and in others as negative return value containing an errno-like value. Let's work around this: this is a + * wrapper that uses errno if it is set, and uses the return value otherwise. And then it sets both errno and + * the return value indicating the same issue, so that we are definitely on the safe side. + * + * See https://github.com/systemd/systemd/issues/6737 */ + + errno = 0; + ret = personality(p); + if (ret < 0) { + if (errno != 0) + return -errno; + + errno = -ret; + } + + return ret; +} + int opinionated_personality(unsigned long *ret) { int current; @@ -911,9 +933,9 @@ int opinionated_personality(unsigned long *ret) { * opinionated though, and ignores all the finer-grained bits and exotic personalities, only distinguishing the * two most relevant personalities: PER_LINUX and PER_LINUX32. */ - current = personality(PERSONALITY_INVALID); + current = safe_personality(PERSONALITY_INVALID); if (current < 0) - return -errno; + return current; if (((unsigned long) current & 0xffff) == PER_LINUX32) *ret = PER_LINUX32; diff --git a/src/basic/process-util.h b/src/basic/process-util.h index d71db2d..82af2f9 100644 --- a/src/basic/process-util.h +++ b/src/basic/process-util.h @@ -91,6 +91,7 @@ bool oom_score_adjust_is_valid(int oa); unsigned long personality_from_string(const char *p); const char *personality_to_string(unsigned long); +int safe_personality(unsigned long p); int opinionated_personality(unsigned long *ret); int ioprio_class_to_string_alloc(int i, char **s); diff --git a/src/core/execute.c b/src/core/execute.c index e4bf237..21c0149 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2614,11 +2614,13 @@ static int exec_child( return -errno; } - if (context->personality != PERSONALITY_INVALID) - if (personality(context->personality) < 0) { + if (context->personality != PERSONALITY_INVALID) { + r = safe_personality(context->personality); + if (r < 0) { *exit_status = EXIT_PERSONALITY; - return -errno; + return r; } + } if (context->utmp_id) utmp_put_init_process(context->utmp_id, getpid_cached(), getsid(0), diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 0cbd8c3..24a3da6 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -2267,11 +2267,13 @@ static int inner_child( setup_hostname(); if (arg_personality != PERSONALITY_INVALID) { - if (personality(arg_personality) < 0) - return log_error_errno(errno, "personality() failed: %m"); + r = safe_personality(arg_personality); + if (r < 0) + return log_error_errno(r, "personality() failed: %m"); } else if (secondary) { - if (personality(PER_LINUX32) < 0) - return log_error_errno(errno, "personality() failed: %m"); + r = safe_personality(PER_LINUX32); + if (r < 0) + return log_error_errno(r, "personality() failed: %m"); } #ifdef HAVE_SELINUX diff --git a/src/test/test-seccomp.c b/src/test/test-seccomp.c index 30b87a8..5056a08 100644 --- a/src/test/test-seccomp.c +++ b/src/test/test-seccomp.c @@ -582,59 +582,28 @@ static void test_lock_personality(void) { assert_se(pid >= 0); if (pid == 0) { - int ret; - assert_se(seccomp_lock_personality(current) >= 0); - assert_se((unsigned long) personality(current) == current); - - errno = EUCLEAN; - ret = personality(PER_LINUX | ADDR_NO_RANDOMIZE); - assert_se((ret == -1 && errno == EPERM) || (ret == -EPERM && errno == EUCLEAN)); - - errno = EUCLEAN; - ret = personality(PER_LINUX | MMAP_PAGE_ZERO); - assert_se((ret == -1 && errno == EPERM) || (ret == -EPERM && errno == EUCLEAN)); - - errno = EUCLEAN; - ret = personality(PER_LINUX | ADDR_COMPAT_LAYOUT); - assert_se((ret == -1 && errno == EPERM) || (ret == -EPERM && errno == EUCLEAN)); - - errno = EUCLEAN; - ret = personality(PER_LINUX | READ_IMPLIES_EXEC); - assert_se((ret == -1 && errno == EPERM) || (ret == -EPERM && errno == EUCLEAN)); - - errno = EUCLEAN; - ret = personality(PER_LINUX_32BIT); - assert_se((ret == -1 && errno == EPERM) || (ret == -EPERM && errno == EUCLEAN)); + assert_se((unsigned long) safe_personality(current) == current); - errno = EUCLEAN; - ret = personality(PER_SVR4); - assert_se((ret == -1 && errno == EPERM) || (ret == -EPERM && errno == EUCLEAN)); - - errno = EUCLEAN; - ret = personality(PER_BSD); - assert_se((ret == -1 && errno == EPERM) || (ret == -EPERM && errno == EUCLEAN)); - - errno = EUCLEAN; - ret = personality(current == PER_LINUX ? PER_LINUX32 : PER_LINUX); - assert_se((ret == -1 && errno == EPERM) || (ret == -EPERM && errno == EUCLEAN)); - - errno = EUCLEAN; - ret = personality(PER_LINUX32_3GB); - assert_se((ret == -1 && errno == EPERM) || (ret == -EPERM && errno == EUCLEAN)); - - errno = EUCLEAN; - ret = personality(PER_UW7); - assert_se((ret == -1 && errno == EPERM) || (ret == -EPERM && errno == EUCLEAN)); - - errno = EUCLEAN; - ret = personality(0x42); - assert_se((ret == -1 && errno == EPERM) || (ret == -EPERM && errno == EUCLEAN)); + /* Note, we also test that safe_personality() works correctly, by checkig whether errno is properly + * set, in addition to the return value */ + errno = 0; + assert_se(safe_personality(PER_LINUX | ADDR_NO_RANDOMIZE) == -EPERM); + assert_se(errno == EPERM); - errno = EUCLEAN; - ret = personality(PERSONALITY_INVALID); /* maybe remove this later */ - assert_se((ret == -1 && errno == EPERM) || (ret == -EPERM && errno == EUCLEAN)); + assert_se(safe_personality(PER_LINUX | MMAP_PAGE_ZERO) == -EPERM); + assert_se(safe_personality(PER_LINUX | ADDR_COMPAT_LAYOUT) == -EPERM); + assert_se(safe_personality(PER_LINUX | READ_IMPLIES_EXEC) == -EPERM); + assert_se(safe_personality(PER_LINUX_32BIT) == -EPERM); + assert_se(safe_personality(PER_SVR4) == -EPERM); + assert_se(safe_personality(PER_BSD) == -EPERM); + assert_se(safe_personality(current == PER_LINUX ? PER_LINUX32 : PER_LINUX) == -EPERM); + assert_se(safe_personality(PER_LINUX32_3GB) == -EPERM); + assert_se(safe_personality(PER_UW7) == -EPERM); + assert_se(safe_personality(0x42) == -EPERM); + + assert_se(safe_personality(PERSONALITY_INVALID) == -EPERM); /* maybe remove this later */ assert_se((unsigned long) personality(current) == current); _exit(EXIT_SUCCESS); -- 2.7.4