[libvirt] [PATCH] daemon: Fix core dumps if unix_sock_group is set

[Daniel P. Berrange]

On Fri, Jan 07, 2011 at 12:50:25PM +0100, Jiri Denemark wrote:
> Setting unix_sock_group to something else than default "root" in
> /etc/libvirt/libvirtd.conf prevents system libvirtd from dumping core on
> crash. This is because we used setgid(unix_sock_group) before binding to
> /var/run/libvirt/libvirt-sock* and setgid() back to original group.
> However, if a process changes its effective or filesystem group ID, it
> will be forbidden from leaving core dumps unless fs.suid_dumpable sysctl
> is set to something else then 0 (and it is 0 by default).
> 
> Changing socket's group ownership after bind works better. And we can do
> so without introducing a race condition since we loosen access rights by
> changing the group from root to something else.

If you use  fchown(sock->fd) then you avoid any possible race issues.

> ---
>  daemon/libvirtd.c |   17 ++++++++---------
>  1 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index 2df9337..b1539b1 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -542,7 +542,6 @@ static int qemudListenUnix(struct qemud_server *server,
>                             char *path, int readonly, int auth) {
>      struct qemud_socket *sock;
>      mode_t oldmask;
> -    gid_t oldgrp;
>      char ebuf[1024];
>  
>      if (VIR_ALLOC(sock) < 0) {
> @@ -579,21 +578,21 @@ static int qemudListenUnix(struct qemud_server *server,
>      if (sock->addr.data.un.sun_path[0] == '@')
>          sock->addr.data.un.sun_path[0] = '\0';
>  
> -    oldgrp = getgid();
>      oldmask = umask(readonly ? ~unix_sock_ro_mask : ~unix_sock_rw_mask);
> -    if (server->privileged && setgid(unix_sock_gid)) {
> -        VIR_ERROR(_("Failed to set group ID to %d"), unix_sock_gid);
> -        goto cleanup;
> -    }
> -
>      if (bind(sock->fd, &sock->addr.data.sa, sock->addr.len) < 0) {
>          VIR_ERROR(_("Failed to bind socket to '%s': %s"),
>                    path, virStrerror(errno, ebuf, sizeof ebuf));
>          goto cleanup;
>      }
>      umask(oldmask);
> -    if (server->privileged && setgid(oldgrp)) {
> -        VIR_ERROR(_("Failed to restore group ID to %d"), oldgrp);
> +
> +    /* chown() doesn't work for abstract sockets but we use them only
> +     * if libvirtd runs unprivileged
> +     */

fchown() would work on abstract sockets too

> +    if (server->privileged && chown(path, -1, unix_sock_gid)) {
> +        VIR_ERROR(_("Failed to change group ID of '%s' to %d: %s"),
> +                  path, unix_sock_gid,
> +                  virStrerror(errno, ebuf, sizeof ebuf));
>          goto cleanup;
>      }

Regards,
Daniel