From 65b0ee31b1c1fbc731fd8b15ec9ceeabba5bca44 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 21 May 2013 20:59:10 -0600 Subject: [PATCH] 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 (cherry picked from commit ee777e994927ed5f2d427fbc5a53cbe8b5969bda) Conflicts: src/lxc/lxc_container.c - did not use setUIDGID before 1.1.0 src/util/virutil.c - oom handling changes not backported; no virSetUIDGIDWithCaps src/util/virfile.c - functions still lived in virutil.c this far back configure.ac - context with previous commit src/util/command.c - no UID/GID handling in vircommand.c... src/storage/storage_backend.c - ...so do it in the one hook user instead --- configure.ac | 4 +- src/lxc/lxc_container.c | 2 +- src/security/security_dac.c | 16 +++-- src/storage/storage_backend.c | 16 ++++- src/util/util.c | 129 ++++++++++++++-------------------- src/util/util.h | 2 +- 6 files changed, 83 insertions(+), 86 deletions(-) diff --git a/configure.ac b/configure.ac index aeff11d622..ca34ac18ab 100644 --- a/configure.ac +++ b/configure.ac @@ -172,8 +172,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 getgrouplist getmntent_r \ - getpwuid_r getuid initgroups kill mmap newlocale posix_fallocate \ - posix_memalign regexec sched_getaffinity]) + getpwuid_r getuid kill mmap newlocale posix_fallocate posix_memalign \ + prlimit regexec sched_getaffinity setgroups]) 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 8faa664d74..9d94042034 100644 --- 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 diff --git a/src/security/security_dac.c b/src/security/security_dac.c index ae5d8aacc3..61b705c11d 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -839,16 +839,24 @@ 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", user, 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/storage/storage_backend.c b/src/storage/storage_backend.c index 75a3667bfb..c820d74ca1 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -538,6 +538,8 @@ cleanup: struct hookdata { virStorageVolDefPtr vol; bool skip; + gid_t *groups; + int ngroups; }; static int virStorageBuildSetUIDHook(void *data) { @@ -547,9 +549,13 @@ static int virStorageBuildSetUIDHook(void *data) { if (tmp->skip) return 0; - if (virSetUIDGID(vol->target.perms.uid, vol->target.perms.gid) < 0) + if (virSetUIDGID(vol->target.perms.uid, vol->target.perms.gid, + tmp->groups, tmp->ngroups) < 0) { + VIR_FREE(tmp->groups); return -1; + } + VIR_FREE(tmp->groups); return 0; } @@ -560,7 +566,7 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, gid_t gid; uid_t uid; int filecreated = 0; - struct hookdata data = {vol, false}; + struct hookdata data = {vol, false, NULL, 0}; if ((pool->def->type == VIR_STORAGE_POOL_NETFS) && (((getuid() == 0) @@ -569,6 +575,11 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, || ((vol->target.perms.gid != -1) && (vol->target.perms.gid != getgid())))) { + if ((data.ngroups = virGetGroupList(vol->target.perms.uid, + vol->target.perms.gid, + &data.groups)) < 0) + return -1; + virCommandSetPreExecHook(cmd, virStorageBuildSetUIDHook, &data); if (virCommandRun(cmd, NULL) == 0) { @@ -576,6 +587,7 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, if (stat(vol->target.path, &st) >=0) filecreated = 1; } + VIR_FREE(data.groups); } data.skip = true; diff --git a/src/util/util.c b/src/util/util.c index 5ca5034148..f0ccaf4dcf 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -718,18 +718,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 */ @@ -758,7 +766,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; } @@ -834,17 +842,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; } @@ -865,7 +880,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; } @@ -913,6 +928,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, /* parent */ + VIR_FREE(groups); VIR_FORCE_CLOSE(pair[1]); do { @@ -1123,6 +1139,8 @@ int virDirCreate(const char *path, mode_t mode, 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) @@ -1137,15 +1155,21 @@ int virDirCreate(const char *path, mode_t mode, 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; @@ -1172,7 +1196,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; } @@ -2635,87 +2659,38 @@ no_memory: goto cleanup; } -/* 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 > 0) { - if (setregid(gid, gid) < 0) { - virReportSystemError(err = errno, - _("cannot change to '%d' 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 > 0) { -# 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(%d)"), - (unsigned int) uid); - goto error; - } - - if (!pwd_result) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("getpwuid_r failed to retrieve data " - "for uid '%d'"), - (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 '%d'"), - (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 */ @@ -2932,7 +2907,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")); diff --git a/src/util/util.h b/src/util/util.h index 98964b298f..0f904c3831 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -53,7 +53,7 @@ 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 virSetUIDGID(uid_t uid, gid_t gid, gid_t *groups, int ngroups); int virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK;