External/libvirt
1
0
Fork 0
mirror of https://gitlab.com/libvirt/libvirt.git synced 2024-12-25 07:05:28 +00:00
Code Issues Packages Projects Releases Wiki Activity

util: make virSetUIDGID async-signal-safe

Browse Source 
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>
(cherry picked from commit ee777e9949)

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
This commit is contained in:
Eric Blake 2013-05-21 20:59:10 -06:00
parent 4e64a086e1
commit 65b0ee31b1
6 changed files with 83 additions and 86 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
configure.ac
View File

@ -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.

2
src/lxc/lxc_container.c
View File

@ -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

16
src/security/security_dac.c
View File

@ -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;
}

16
src/storage/storage_backend.c
View File

@ -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;

129
src/util/util.c
View File

@ -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"));

2
src/util/util.h
View File

@ -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;