util: make virSetUIDGID async-signal-safe

https://bugzilla.redhat.com/show_bug.cgi?id=964358

POSIX states that multi-threaded apps should not use functions
that are not async-signal-safe between fork and exec, yet we
were using getpwuid_r and initgroups.  Although rare, it is
possible to hit deadlock in the child, when it tries to grab
a mutex that was already held by another thread in the parent.
I actually hit this deadlock when testing multiple domains
being started in parallel with a command hook, with the following
backtrace in the child:

 Thread 1 (Thread 0x7fd56bbf2700 (LWP 3212)):
 #0  __lll_lock_wait ()
     at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:136
 #1  0x00007fd5761e7388 in _L_lock_854 () from /lib64/libpthread.so.0
 #2  0x00007fd5761e7257 in __pthread_mutex_lock (mutex=0x7fd56be00360)
     at pthread_mutex_lock.c:61
 #3  0x00007fd56bbf9fc5 in _nss_files_getpwuid_r (uid=0, result=0x7fd56bbf0c70,
     buffer=0x7fd55c2a65f0 "", buflen=1024, errnop=0x7fd56bbf25b8)
     at nss_files/files-pwd.c:40
 #4  0x00007fd575aeff1d in __getpwuid_r (uid=0, resbuf=0x7fd56bbf0c70,
     buffer=0x7fd55c2a65f0 "", buflen=1024, result=0x7fd56bbf0cb0)
     at ../nss/getXXbyYY_r.c:253
 #5  0x00007fd578aebafc in virSetUIDGID (uid=0, gid=0) at util/virutil.c:1031
 #6  0x00007fd578aebf43 in virSetUIDGIDWithCaps (uid=0, gid=0, capBits=0,
     clearExistingCaps=true) at util/virutil.c:1388
 #7  0x00007fd578a9a20b in virExec (cmd=0x7fd55c231f10) at util/vircommand.c:654
 #8  0x00007fd578a9dfa2 in virCommandRunAsync (cmd=0x7fd55c231f10, pid=0x0)
     at util/vircommand.c:2247
 #9  0x00007fd578a9d74e in virCommandRun (cmd=0x7fd55c231f10, exitstatus=0x0)
     at util/vircommand.c:2100
 #10 0x00007fd56326fde5 in qemuProcessStart (conn=0x7fd53c000df0,
     driver=0x7fd55c0dc4f0, vm=0x7fd54800b100, migrateFrom=0x0, stdin_fd=-1,
     stdin_path=0x0, snapshot=0x0, vmop=VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
     flags=1) at qemu/qemu_process.c:3694
 ...

The solution is to split the work of getpwuid_r/initgroups into the
unsafe portions (getgrouplist, called pre-fork) and safe portions
(setgroups, called post-fork).

* src/util/virutil.h (virSetUIDGID, virSetUIDGIDWithCaps): Adjust
signature.
* src/util/virutil.c (virSetUIDGID): Add parameters.
(virSetUIDGIDWithCaps): Adjust clients.
* src/util/vircommand.c (virExec): Likewise.
* src/util/virfile.c (virFileAccessibleAs, virFileOpenForked)
(virDirCreate): Likewise.
* src/security/security_dac.c (virSecurityDACSetProcessLabel):
Likewise.
* src/lxc/lxc_container.c (lxcContainerSetID): Likewise.
* configure.ac (AC_CHECK_FUNCS_ONCE): Check for setgroups, not
initgroups.

Signed-off-by: Eric Blake <eblake@redhat.com>

diff --git a/configure.ac b/configure.ac
index 015d6c25d699ea5f42fe9391dfff3a1d9112ceca..b5af0d35c2cdc61c66b73311dfa64cc6504cb166 100644 (file)
--- a/configure.ac
+++ b/configure.ac
@@ -206,8 +206,8 @@ AC_CHECK_SIZEOF([long])
 dnl Availability of various common functions (non-fatal if missing),
 dnl and various less common threadsafe functions
 AC_CHECK_FUNCS_ONCE([cfmakeraw geteuid getgid getgrnam_r getmntent_r \
-  getpwuid_r getuid initgroups kill mmap newlocale posix_fallocate \
-  posix_memalign prlimit regexec sched_getaffinity setns setrlimit symlink])
+  getpwuid_r getuid kill mmap newlocale posix_fallocate posix_memalign \
+  prlimit regexec sched_getaffinity setgroups setns setrlimit symlink])
 
 dnl Availability of pthread functions (if missing, win32 threading is
 dnl assumed).  Because of $LIB_PTHREAD, we cannot use AC_CHECK_FUNCS_ONCE.

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 7434264d8ee88e9b51f2ed212a40ff5e3f365678..fcd9b745431998ce15b35cf54170faf030ccae0a 100644 (file)
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008-2012 Red Hat, Inc.
+ * Copyright (C) 2008-2013 Red Hat, Inc.
  * Copyright (C) 2008 IBM Corp.
  *
  * lxc_container.c: file description
@@ -351,17 +351,24 @@ int lxcContainerWaitForContinue(int control)
  */
 static int lxcContainerSetID(virDomainDefPtr def)
 {
+    gid_t *groups;
+    int ngroups;
+
     /* Only call virSetUIDGID when user namespace is enabled
      * for this container. And user namespace is only enabled
      * when nuidmap&ngidmap is not zero */
 
     VIR_DEBUG("Set UID/GID to 0/0");
-    if (def->idmap.nuidmap && virSetUIDGID(0, 0) < 0) {
+    if (def->idmap.nuidmap &&
+        ((ngroups = virGetGroupList(0, 0, &groups) < 0) ||
+         virSetUIDGID(0, 0, groups, ngroups) < 0)) {
         virReportSystemError(errno, "%s",
                              _("setuid or setgid failed"));
+        VIR_FREE(groups);
         return -1;
     }
 
+    VIR_FREE(groups);
     return 0;
 }
 

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index baa987ccf9474588a5d01e4d2e583f6c97b8ac54..9b5eaa890ee51713fe64f6a72c1488dedcd8c4e3 100644 (file)
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -997,17 +997,25 @@ virSecurityDACSetProcessLabel(virSecurityManagerPtr mgr,
     uid_t user;
     gid_t group;
     virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+    gid_t *groups;
+    int ngroups;
+    int ret = -1;
 
     if (virSecurityDACGetIds(def, priv, &user, &group))
         return -1;
+    ngroups = virGetGroupList(user, group, &groups);
+    if (ngroups < 0)
+        return -1;
 
     VIR_DEBUG("Dropping privileges of DEF to %u:%u",
               (unsigned int) user, (unsigned int) group);
 
-    if (virSetUIDGID(user, group) < 0)
-        return -1;
-
-    return 0;
+    if (virSetUIDGID(user, group, groups, ngroups) < 0)
+        goto cleanup;
+    ret = 0;
+cleanup:
+    VIR_FREE(groups);
+    return ret;
 }
 
 

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 033b55b6ee8c66a193dc97ad6a32c273797a3113..387950401efd28e4aa5f6e74299a4110f861a9d1 100644 (file)
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -402,6 +402,8 @@ virExec(virCommandPtr cmd)
     const char *binary = NULL;
     int forkRet, ret;
     struct sigaction waxon, waxoff;
+    gid_t *groups = NULL;
+    int ngroups;
 
     if (cmd->args[0][0] != '/') {
         if (!(binary = virFindFileInPath(cmd->args[0]))) {
@@ -472,6 +474,9 @@ virExec(virCommandPtr cmd)
         childerr = null;
     }
 
+    if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, &groups)) < 0)
+        goto cleanup;
+
     forkRet = virFork(&pid);
 
     if (pid < 0) {
@@ -497,6 +502,7 @@ virExec(virCommandPtr cmd)
 
         if (binary != cmd->args[0])
             VIR_FREE(binary);
+        VIR_FREE(groups);
 
         return 0;
     }
@@ -656,7 +662,8 @@ virExec(virCommandPtr cmd)
         cmd->capabilities || (cmd->flags & VIR_EXEC_CLEAR_CAPS)) {
         VIR_DEBUG("Setting child uid:gid to %d:%d with caps %llx",
                   (int)cmd->uid, (int)cmd->gid, cmd->capabilities);
-        if (virSetUIDGIDWithCaps(cmd->uid, cmd->gid, cmd->capabilities,
+        if (virSetUIDGIDWithCaps(cmd->uid, cmd->gid, groups, ngroups,
+                                 cmd->capabilities,
                                  !!(cmd->flags & VIR_EXEC_CLEAR_CAPS)) < 0) {
             goto fork_error;
         }
@@ -700,6 +707,7 @@ virExec(virCommandPtr cmd)
     /* This is cleanup of parent process only - child
        should never jump here on error */
 
+    VIR_FREE(groups);
     if (binary != cmd->args[0])
         VIR_FREE(binary);
 

diff --git a/src/util/virfile.c b/src/util/virfile.c
index feb351a4142e7351783d03596d2fda41d4972de9..8f0eec3d64af0d8ba14b119bd2f6a0138184366a 100644 (file)
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -1447,18 +1447,26 @@ virFileAccessibleAs(const char *path, int mode,
     pid_t pid = 0;
     int status, ret = 0;
     int forkRet = 0;
+    gid_t *groups;
+    int ngroups;
 
     if (uid == getuid() &&
         gid == getgid())
         return access(path, mode);
 
+    ngroups = virGetGroupList(uid, gid, &groups);
+    if (ngroups < 0)
+        return -1;
+
     forkRet = virFork(&pid);
 
     if (pid < 0) {
+        VIR_FREE(groups);
         return -1;
     }
 
     if (pid) { /* parent */
+        VIR_FREE(groups);
         if (virProcessWait(pid, &status) < 0) {
             /* virProcessWait() already
              * reported error */
@@ -1487,7 +1495,7 @@ virFileAccessibleAs(const char *path, int mode,
         goto childerror;
     }
 
-    if (virSetUIDGID(uid, gid) < 0) {
+    if (virSetUIDGID(uid, gid, groups, ngroups) < 0) {
         ret = errno;
         goto childerror;
     }
@@ -1563,17 +1571,24 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
     int fd = -1;
     int pair[2] = { -1, -1 };
     int forkRet;
+    gid_t *groups;
+    int ngroups;
 
     /* parent is running as root, but caller requested that the
      * file be opened as some other user and/or group). The
      * following dance avoids problems caused by root-squashing
      * NFS servers. */
 
+    ngroups = virGetGroupList(uid, gid, &groups);
+    if (ngroups < 0)
+        return -errno;
+
     if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) < 0) {
         ret = -errno;
         virReportSystemError(errno,
                              _("failed to create socket needed for '%s'"),
                              path);
+        VIR_FREE(groups);
         return ret;
     }
 
@@ -1594,7 +1609,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
 
         /* set desired uid/gid, then attempt to create the file */
 
-        if (virSetUIDGID(uid, gid) < 0) {
+        if (virSetUIDGID(uid, gid, groups, ngroups) < 0) {
             ret = -errno;
             goto childerror;
         }
@@ -1642,6 +1657,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
 
     /* parent */
 
+    VIR_FREE(groups);
     VIR_FORCE_CLOSE(pair[1]);
 
     do {
@@ -1843,6 +1859,8 @@ virDirCreate(const char *path,
     pid_t pid;
     int waitret;
     int status, ret = 0;
+    gid_t *groups;
+    int ngroups;
 
     /* allow using -1 to mean "current value" */
     if (uid == (uid_t) -1)
@@ -1857,15 +1875,21 @@ virDirCreate(const char *path,
         return virDirCreateNoFork(path, mode, uid, gid, flags);
     }
 
+    ngroups = virGetGroupList(uid, gid, &groups);
+    if (ngroups < 0)
+        return -errno;
+
     int forkRet = virFork(&pid);
 
     if (pid < 0) {
         ret = -errno;
+        VIR_FREE(groups);
         return ret;
     }
 
     if (pid) { /* parent */
         /* wait for child to complete, and retrieve its exit code */
+        VIR_FREE(groups);
         while ((waitret = waitpid(pid, &status, 0) == -1)  && (errno == EINTR));
         if (waitret == -1) {
             ret = -errno;
@@ -1892,7 +1916,7 @@ parenterror:
 
     /* set desired uid/gid, then attempt to create the directory */
 
-    if (virSetUIDGID(uid, gid) < 0) {
+    if (virSetUIDGID(uid, gid, groups, ngroups) < 0) {
         ret = -errno;
         goto childerror;
     }

diff --git a/src/util/virutil.c b/src/util/virutil.c
index 136ff2565202604e1341447ab73c7193b9825228..0b54ef76e5d3546fd7df5b9d169966800e8c0132 100644 (file)
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1000,85 +1000,38 @@ virGetGroupList(uid_t uid, gid_t gid, gid_t **list)
 }
 
 
-/* Set the real and effective uid and gid to the given values, and call
- * initgroups so that the process has all the assumed group membership of
- * that uid. return 0 on success, -1 on failure (the original system error
- * remains in errno).
+/* Set the real and effective uid and gid to the given values, as well
+ * as all the supplementary groups, so that the process has all the
+ * assumed group membership of that uid. Return 0 on success, -1 on
+ * failure (the original system error remains in errno).
  */
 int
-virSetUIDGID(uid_t uid, gid_t gid)
+virSetUIDGID(uid_t uid, gid_t gid, gid_t *groups ATTRIBUTE_UNUSED,
+             int ngroups ATTRIBUTE_UNUSED)
 {
-    int err;
-    char *buf = NULL;
-
-    if (gid != (gid_t)-1) {
-        if (setregid(gid, gid) < 0) {
-            virReportSystemError(err = errno,
-                                 _("cannot change to '%u' group"),
-                                 (unsigned int) gid);
-            goto error;
-        }
+    if (gid != (gid_t)-1 && setregid(gid, gid) < 0) {
+        virReportSystemError(errno,
+                             _("cannot change to '%u' group"),
+                             (unsigned int) gid);
+        return -1;
     }
 
-    if (uid != (uid_t)-1) {
-# ifdef HAVE_INITGROUPS
-        struct passwd pwd, *pwd_result;
-        size_t bufsize;
-        int rc;
-
-        bufsize = sysconf(_SC_GETPW_R_SIZE_MAX);
-        if (bufsize == -1)
-            bufsize = 16384;
-
-        if (VIR_ALLOC_N(buf, bufsize) < 0) {
-            err = ENOMEM;
-            goto error;
-        }
-        while ((rc = getpwuid_r(uid, &pwd, buf, bufsize,
-                                &pwd_result)) == ERANGE) {
-            if (VIR_RESIZE_N(buf, bufsize, bufsize, bufsize) < 0) {
-                err = ENOMEM;
-                goto error;
-            }
-        }
-
-        if (rc) {
-            virReportSystemError(err = rc, _("cannot getpwuid_r(%u)"),
-                                 (unsigned int) uid);
-            goto error;
-        }
-
-        if (!pwd_result) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("getpwuid_r failed to retrieve data "
-                             "for uid '%u'"),
-                           (unsigned int) uid);
-            err = EINVAL;
-            goto error;
-        }
-
-        if (initgroups(pwd.pw_name, pwd.pw_gid) < 0) {
-            virReportSystemError(err = errno,
-                                 _("cannot initgroups(\"%s\", %d)"),
-                                 pwd.pw_name, (unsigned int) pwd.pw_gid);
-            goto error;
-        }
+# if HAVE_SETGROUPS
+    if (ngroups && setgroups(ngroups, groups) < 0) {
+        virReportSystemError(errno, "%s",
+                             _("cannot set supplemental groups"));
+        return -1;
+    }
 # endif
-        if (setreuid(uid, uid) < 0) {
-            virReportSystemError(err = errno,
-                                 _("cannot change to uid to '%u'"),
-                                 (unsigned int) uid);
-            goto error;
-        }
+
+    if (uid != (uid_t)-1 && setreuid(uid, uid) < 0) {
+        virReportSystemError(errno,
+                             _("cannot change to uid to '%u'"),
+                             (unsigned int) uid);
+        return -1;
     }
 
-    VIR_FREE(buf);
     return 0;
-
-error:
-    VIR_FREE(buf);
-    errno = err;
-    return -1;
 }
 
 #else /* ! HAVE_GETPWUID_R */
@@ -1281,7 +1234,9 @@ int virGetGroupID(const char *name ATTRIBUTE_UNUSED,
 
 int
 virSetUIDGID(uid_t uid ATTRIBUTE_UNUSED,
-             gid_t gid ATTRIBUTE_UNUSED)
+             gid_t gid ATTRIBUTE_UNUSED,
+             gid_t *groups ATTRIBUTE_UNUSED,
+             int ngroups ATTRIBUTE_UNUSED)
 {
     virReportError(VIR_ERR_INTERNAL_ERROR,
                    "%s", _("virSetUIDGID is not available"));
@@ -1305,8 +1260,8 @@ virGetGroupName(gid_t gid ATTRIBUTE_UNUSED)
  * errno).
  */
 int
-virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits,
-                     bool clearExistingCaps)
+virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups,
+                     unsigned long long capBits, bool clearExistingCaps)
 {
     size_t i;
     int capng_ret, ret = -1;
@@ -1377,7 +1332,7 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits,
         }
     }
 
-    if (virSetUIDGID(uid, gid) < 0)
+    if (virSetUIDGID(uid, gid, groups, ngroups) < 0)
         goto cleanup;
 
     /* Tell it we are done keeping capabilities */
@@ -1421,11 +1376,11 @@ cleanup:
  */
 
 int
-virSetUIDGIDWithCaps(uid_t uid, gid_t gid,
+virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups,
                      unsigned long long capBits ATTRIBUTE_UNUSED,
                      bool clearExistingCaps ATTRIBUTE_UNUSED)
 {
-    return virSetUIDGID(uid, gid);
+    return virSetUIDGID(uid, gid, groups, ngroups);
 }
 #endif
 

diff --git a/src/util/virutil.h b/src/util/virutil.h
index 6480b073e8d88fba0d0858239e84419c6cfd141f..0083c8830ed4ce25513ba1c183bdd84d5ad7b5aa 100644 (file)
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -46,8 +46,9 @@ int virSetCloseExec(int fd) ATTRIBUTE_RETURN_CHECK;
 int virPipeReadUntilEOF(int outfd, int errfd,
                         char **outbuf, char **errbuf);
 
-int virSetUIDGID(uid_t uid, gid_t gid);
-int virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits,
+int virSetUIDGID(uid_t uid, gid_t gid, gid_t *groups, int ngroups);
+int virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups,
+                         unsigned long long capBits,
                          bool clearExistingCaps);
 
 int virScaleInteger(unsigned long long *value, const char *suffix,