[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2 3/3] util: make virSetUIDGID async-signal-safe



On 07/09/2013 09:17 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.
> * 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>
> ---
>  configure.ac                |   4 +-
>  src/lxc/lxc_container.c     |   9 +++-
>  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 +-
>  7 files changed, 90 insertions(+), 92 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index a6ad6a3..7f53308 100644
> --- 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 595c0f2..c32947b 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
> @@ -347,12 +347,17 @@ 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"));
>          return -1;
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 0d6defc..b6ddd51 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -1001,17 +1001,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 6e2eb1e..6dc2490 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 ad01845..e986ee7 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -1459,18 +1459,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 */
> @@ -1499,7 +1507,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;
>      }
> @@ -1575,17 +1583,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;
>      }
>
> @@ -1606,7 +1621,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;
>          }
> @@ -1654,6 +1669,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
>
>      /* parent */
>
> +    VIR_FREE(groups);
>      VIR_FORCE_CLOSE(pair[1]);
>
>      do {
> @@ -1855,6 +1871,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)
> @@ -1869,15 +1887,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;
> @@ -1904,7 +1928,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;
>      }


It's too late for me to be reviewing code - I can tell because I spent
several minutes being concerned that you didn't free groups in this code
path. Duh. That's really all I can say to myself :-)


> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 844c694..7e1133c 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -1023,87 +1023,37 @@ 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, 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;


Yep. Should have been organized like this to begin with. Join the
campaign to eliminate excessive indentation in our lifetime!


>      }
>
> -    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 */
> @@ -1306,7 +1256,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"));
> @@ -1330,8 +1282,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;
> @@ -1401,7 +1353,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 */
> @@ -1445,11 +1397,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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]