On 05/21/2013 11:24 PM, Eric Blake wrote: > 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. > * configure.ac (AC_CHECK_FUNCS_ONCE): Check for setgroups, not > initgroups. > > Signed-off-by: Eric Blake <eblake@xxxxxxxxxx> > --- > configure.ac | 5 +- > src/security/security_dac.c | 16 +++++-- > src/util/vircommand.c | 10 +++- > src/util/virfile.c | 30 ++++++++++-- > src/util/virutil.c | 108 ++++++++++++-------------------------------- > src/util/virutil.h | 5 +- > 6 files changed, 84 insertions(+), 90 deletions(-) > > diff --git a/configure.ac b/configure.ac > index d20b90e..26deac7 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -206,8 +206,9 @@ 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 getgrouplist \ > - getmntent_r getpwuid_r getuid initgroups kill mmap newlocale posix_fallocate \ > - posix_memalign prlimit regexec sched_getaffinity setns setrlimit symlink]) > + getmntent_r 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/security/security_dac.c b/src/security/security_dac.c > index d922ad2..907dbd5 100644 > --- a/src/security/security_dac.c > +++ b/src/security/security_dac.c > @@ -1000,17 +1000,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 665f549..ba03802 100644 > --- 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; > } > @@ -651,7 +657,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; > } > @@ -695,6 +702,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 4637919..20ebcbc 100644 > --- a/src/util/virfile.c > +++ b/src/util/virfile.c > @@ -1462,18 +1462,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 */ > @@ -1502,7 +1510,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; > } > @@ -1578,17 +1586,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; > } > > @@ -1609,7 +1624,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; > } > @@ -1657,6 +1672,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, > > /* parent */ > > + VIR_FREE(groups); > VIR_FORCE_CLOSE(pair[1]); > > do { > @@ -1858,6 +1874,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) > @@ -1872,15 +1890,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; > @@ -1907,7 +1931,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 a3cb64f..883672a 100644 > --- a/src/util/virutil.c > +++ b/src/util/virutil.c > @@ -1086,87 +1086,37 @@ error: > } > > > -/* 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, int ngroups) > { > - 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) { > - virReportOOMError(); > - err = ENOMEM; > - goto error; > - } > - while ((rc = getpwuid_r(uid, &pwd, buf, bufsize, > - &pwd_result)) == ERANGE) { > - if (VIR_RESIZE_N(buf, bufsize, bufsize, bufsize) < 0) { > - virReportOOMError(); > - 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 */ > @@ -1383,7 +1333,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")); > @@ -1407,8 +1359,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) > { > int ii, capng_ret, ret = -1; > bool need_setgid = false, need_setuid = false; > @@ -1478,7 +1430,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 */ > @@ -1522,11 +1474,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 6480b07..0083c88 100644 > --- 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, ACK. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list