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

diff --git a/src/basic/process-util.c b/src/basic/process-util.c
index e4cde65..9d01541 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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);