This patch makes it such that the privileges libvirtd daemon can run unprivileged QEMU guests. The default remains unchanged with QEMU running as root:root, but the package maintainer can request an alternative default user at build time, and the sysadmin can also override this at install time with /etc/libvirt/qemu.conf. As well as making QEMU setuid/gid to the non-root user, this patch takes care of chown'ing all resources it needs to access. This currently includes - /dev/bus/usb/$BUS/$DEVICE for any assigned USB devices - /sys/bus/pci/$ADDR/{config,resource*,rom} for PCI devs - All disk paths Upon shutdown it will restore ownership to root for all of thesem, except shared/readonly disk images NB one minor problem is that USB devices attached based on vendor/product ID aren't handled. Need to figure out a way to deal with this.... Daniel commit 5283a949f4f5cc75e471ddd624362261280378d9 Author: Daniel P. Berrange <berrange(a)redhat.com&gt; Date: Wed Jul 15 22:25:01 2009 +0100 Run QEMU guests as an unprivileged user * configure.in: Add --with-qemu-user and --with-qemu-group args * libvirt.spec.in: use 'qemu' for user/group for Fedora >= 12 * qemud/libvirtd_qemu.arg, qemud/test_libvirtd_qemu.aug, src/qemu.conf: Add 'user' and 'group' args for configuration * src/Makefile.am: Create %localstatedir/cache/libvirt/qemu * src/qemu_conf.c, src/qemu_conf.h: Load user/group from config * src/qemu_driver.c: Change user ID/group ID when launching QEMU guests. Change user/group ownership on disks/usb/pci devs. Put memory dumps in %localstatedir/cache/libvirt/qemu * src/util.c, src/util.h: Add convenient APIs for converting username/groupname to user ID / group ID diff --git a/configure.in b/configure.in index 552089a..634e812 100644 --- a/configure.in +++ b/configure.in @@ -1437,6 +1437,19 @@ AM_CONDITIONAL([WITH_NODE_DEVICES], [test "$with_nodedev" = "yes"]) AM_CONDITIONAL([WITH_LINUX], [test `uname -s` = "Linux"]) + +AC_ARG_WITH([qemu-user], + [ --with-qemu-user username to run QEMU system instance as], + [QEMU_USER=${withval}], + [QEMU_USER=root]) +AC_ARG_WITH([qemu-group], + [ --with-qemu-group groupname to run QEMU system instance as], + [QEMU_GROUP=${withval}], + [QEMU_GROUP=root]) +AC_DEFINE_UNQUOTED([QEMU_USER], ["$QEMU_USER"], [QEMU user account]) +AC_DEFINE_UNQUOTED([QEMU_GROUP], ["$QEMU_GROUP"], [QEMU group account]) + + # Only COPYING.LIB is under version control, yet COPYING # is included as part of the distribution tarball. # Copy one to the other, but only if this is a srcdir-build. @@ -1578,3 +1591,7 @@ AC_MSG_NOTICE([ Debug: $enable_debug]) AC_MSG_NOTICE([ Warnings: $enable_compile_warnings]) AC_MSG_NOTICE([ Readline: $lv_use_readline]) AC_MSG_NOTICE([]) +AC_MSG_NOTICE([Privileges]) +AC_MSG_NOTICE([]) +AC_MSG_NOTICE([ QEMU: $QEMU_USER:$QEMU_GROUP]) +AC_MSG_NOTICE([]) \ No newline at end of file diff --git a/libvirt.spec.in b/libvirt.spec.in index 6321eaa..1bde6c5 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -46,6 +46,14 @@ %define with_capng 0%{!?_without_capng:1} %endif +%if 0%{?fedora} >= 12 +%define qemu_user qemu +%define qemu_group qemu +%else +%define qemu_user root +%define qemu_group root +%endif + # # If building on RHEL switch on the specific support # @@ -309,6 +317,8 @@ of recent versions of Linux (and other OSes). %{?_without_storage_iscsi} \ %{?_without_storage_disk} \ %{?_without_numactl} \ + --with-qemu-user=%{qemu_user} \ + --with-qemu-group=%{qemu_group} \ --with-init-script=redhat \ --with-qemud-pid-file=%{_localstatedir}/run/libvirt_qemud.pid \ --with-remote-file=%{_localstatedir}/run/libvirtd.pid @@ -446,8 +456,9 @@ fi %dir %attr(0700, root, root) %{_localstatedir}/cache/libvirt/ %if %{with_qemu} -%dir %{_localstatedir}/run/libvirt/qemu/ -%dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/qemu/ +%dir %attr(0700, %{qemu_user}, %{qemu_group}) %{_localstatedir}/run/libvirt/qemu/ +%dir %attr(0700, %{qemu_user}, %{qemu_group}) %{_localstatedir}/lib/libvirt/qemu/ +%dir %attr(0700, %{qemu_user}, %{qemu_group}) %{_localstatedir}/cache/libvirt/qemu/ %endif %if %{with_lxc} %dir %{_localstatedir}/run/libvirt/lxc/ diff --git a/qemud/libvirtd_qemu.aug b/qemud/libvirtd_qemu.aug index 02699d6..abd6b62 100644 --- a/qemud/libvirtd_qemu.aug +++ b/qemud/libvirtd_qemu.aug @@ -29,6 +29,8 @@ module Libvirtd_qemu = | str_entry "vnc_password" | bool_entry "vnc_sasl" | str_entry "vnc_sasl_dir" + | str_entry "user" + | str_entry "group" (* Each enty in the config is one of the following three ... *) let entry = vnc_entry diff --git a/qemud/test_libvirtd_qemu.aug b/qemud/test_libvirtd_qemu.aug index 6f38e47..f62da01 100644 --- a/qemud/test_libvirtd_qemu.aug +++ b/qemud/test_libvirtd_qemu.aug @@ -79,6 +79,10 @@ vnc_sasl = 1 # point to the directory, and create a qemu.conf in that location # vnc_sasl_dir = \"/some/directory/sasl2\" + +user = \"root\" + +group = \"root\" " test Libvirtd_qemu.lns get conf = @@ -161,3 +165,7 @@ vnc_sasl_dir = \"/some/directory/sasl2\" { "#comment" = "point to the directory, and create a qemu.conf in that location" } { "#comment" = "" } { "vnc_sasl_dir" = "/some/directory/sasl2" } +{ "#comment" = "" } +{ "user"= "root" } +{ "#comment" = "" } +{ "group" = "root" } \ No newline at end of file diff --git a/src/Makefile.am b/src/Makefile.am index 889ede4..9b662ae 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -684,6 +684,7 @@ install-exec-local: if WITH_QEMU $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/qemu" $(MKDIR_P) "$(DESTDIR)$(localstatedir)/run/libvirt/qemu" + $(MKDIR_P) "$(DESTDIR)$(localstatedir)/cache/libvirt/qemu" endif if WITH_LXC $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/lxc" diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0534d53..59c78d5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -378,6 +378,8 @@ virRun; virSkipSpaces; virKillProcess; virGetUserDirectory; +virGetUserID; +virGetGroupID; # uuid.h diff --git a/src/qemu.conf b/src/qemu.conf index c2a53c5..3009725 100644 --- a/src/qemu.conf +++ b/src/qemu.conf @@ -88,3 +88,10 @@ # to 'none' instead # # security_driver = "selinux" + + +# The user ID for QEMU processes run by the system instance +#user = "root" + +# The group ID for QEMU processes run by the system instance +#group = "root" diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 414b71b..0718f39 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -92,6 +92,8 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, const char *filename) { virConfPtr conf; virConfValuePtr p; + char *user; + char *group; /* Setup 2 critical defaults */ if (!(driver->vncListen = strdup("127.0.0.1"))) { @@ -186,6 +188,36 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, } } + p = virConfGetValue (conf, "user"); + CHECK_TYPE ("user", VIR_CONF_STRING); + if (!(user = strdup(p && p->str ? p->str : QEMU_USER))) { + virReportOOMError(NULL); + virConfFree(conf); + return -1; + } + if (virGetUserID(NULL, user, &driver->user) < 0) { + VIR_FREE(user); + virConfFree(conf); + return -1; + } + VIR_FREE(user); + + p = virConfGetValue (conf, "group"); + CHECK_TYPE ("group", VIR_CONF_STRING); + if (!(group = strdup(p && p->str ? p->str : QEMU_GROUP))) { + virReportOOMError(NULL); + virConfFree(conf); + return -1; + } + + + if (virGetGroupID(NULL, group, &driver->group) < 0) { + VIR_FREE(group); + virConfFree(conf); + return -1; + } + VIR_FREE(group); + virConfFree (conf); return 0; } diff --git a/src/qemu_conf.h b/src/qemu_conf.h index 175173d..fbf2ab9 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -66,6 +66,9 @@ struct qemud_driver { int privileged; + uid_t user; + gid_t group; + unsigned int qemuVersion; int nextvmid; diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 342ba01..f2d021c 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -71,12 +71,11 @@ #define VIR_FROM_THIS VIR_FROM_QEMU /* For storing short-lived temporary files. */ -#define TEMPDIR LOCAL_STATE_DIR "/cache/libvirt" +#define TEMPDIR LOCAL_STATE_DIR "/cache/libvirt/qemu" #define QEMU_CMD_PROMPT "\n(qemu) " #define QEMU_PASSWD_PROMPT "Password: " - static int qemudShutdown(void); static void qemuDriverLock(struct qemud_driver *driver) @@ -1367,6 +1366,205 @@ static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct qemud_driver * return 0; } + +#ifdef __linux__ +static int qemuDomainSetHostdevUSBOwnership(virConnectPtr conn, + virDomainHostdevDefPtr def, + uid_t uid, gid_t gid) +{ + char *usbpath = NULL; + + /* XXX what todo for USB devs assigned based on product/vendor ? Doom :-( */ + if (!def->source.subsys.u.usb.bus || + !def->source.subsys.u.usb.device) + return 0; + + if (virAsprintf(&usbpath, "/dev/bus/usb/%03d/%03d", + def->source.subsys.u.usb.bus, + def->source.subsys.u.usb.device) < 0) { + virReportOOMError(conn); + return -1; + } + + VIR_DEBUG("Setting ownership on %s to %d:%d", usbpath, uid, gid); + if (chown(usbpath, uid, gid) < 0) { + virReportSystemError(conn, errno, _("cannot set ownership on %s"), usbpath); + VIR_FREE(usbpath); + return -1; + } + VIR_FREE(usbpath); + + return 0; +} + +static int qemuDomainSetHostdevPCIOwnership(virConnectPtr conn, + virDomainHostdevDefPtr def, + uid_t uid, gid_t gid) +{ + char *pcidir = NULL; + char *file = NULL; + DIR *dir = NULL; + int ret = -1; + struct dirent *ent; + + if (virAsprintf(&pcidir, "/sys/bus/pci/devices/%04x:%02x:%02x.%x", + def->source.subsys.u.pci.domain, + def->source.subsys.u.pci.bus, + def->source.subsys.u.pci.slot, + def->source.subsys.u.pci.function) < 0) { + virReportOOMError(conn); + goto cleanup; + } + + if (!(dir = opendir(pcidir))) { + virReportSystemError(conn, errno, + _("cannot open %s"), pcidir); + goto cleanup; + } + + while ((ent = readdir(dir)) != NULL) { + /* QEMU device assignment requires: + * $PCIDIR/config, $PCIDIR/resource, $PCIDIR/resourceNNN, $PCIDIR/rom + */ + if (STREQ(ent->d_name, "config") || + STRPREFIX(ent->d_name, "resource") || + STREQ(ent->d_name, "rom")) { + if (virAsprintf(&file, "%s/%s", pcidir, ent->d_name) < 0) { + virReportOOMError(conn); + goto cleanup; + } + VIR_DEBUG("Setting ownership on %s to %d:%d", file, uid, gid); + if (chown(file, uid, gid) < 0) { + virReportSystemError(conn, errno, _("cannot set ownership on %s"), file); + goto cleanup; + } + VIR_FREE(file); + } + } + + ret = 0; + +cleanup: + if (dir) + closedir(dir); + VIR_FREE(file); + VIR_FREE(pcidir); + return ret; +} +#endif + + +static int qemuDomainSetHostdevOwnership(virConnectPtr conn, + virDomainHostdevDefPtr def, + uid_t uid, gid_t gid) +{ + if (def->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + return 0; + +#ifdef __linux__ + switch (def->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: + return qemuDomainSetHostdevUSBOwnership(conn, def, uid, gid); + + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: + return qemuDomainSetHostdevPCIOwnership(conn, def, uid, gid); + + } + return 0; +#else + qemudReportError(conn, NULL, NULL, "%s", + _("unable to set host device ownership on this platform")); + return -1; +#endif + +} + +static int qemuDomainSetDiskOwnership(virConnectPtr conn, + virDomainDiskDefPtr def, + uid_t uid, gid_t gid) +{ + + if (!def->src) + return 0; + + VIR_DEBUG("Setting ownership on %s to %d:%d", def->src, uid, gid); + if (chown(def->src, uid, gid) < 0) { + virReportSystemError(conn, errno, _("cannot set ownership on %s"), + def->src); + return -1; + } + return 0; +} + +static int qemuDomainSetDeviceOwnership(virConnectPtr conn, + struct qemud_driver *driver, + virDomainDeviceDefPtr def, + int restore) +{ + uid_t uid; + gid_t gid; + + if (!driver->privileged) + return 0; + + /* short circuit case of root:root */ + if (!driver->user && !driver->group) + return 0; + + uid = restore ? 0 : driver->user; + gid = restore ? 0 : driver->group; + + switch (def->type) { + case VIR_DOMAIN_DEVICE_DISK: + if (restore && + (def->data.disk->readonly || def->data.disk->shared)) + return 0; + + return qemuDomainSetDiskOwnership(conn, def->data.disk, uid, gid); + + case VIR_DOMAIN_DEVICE_HOSTDEV: + return qemuDomainSetHostdevOwnership(conn, def->data.hostdev, uid, gid); + } + + return 0; +} + +static int qemuDomainSetAllDeviceOwnership(virConnectPtr conn, + struct qemud_driver *driver, + virDomainDefPtr def, + int restore) +{ + int i; + uid_t uid; + gid_t gid; + + if (!driver->privileged) + return 0; + + /* short circuit case of root:root */ + if (!driver->user && !driver->group) + return 0; + + uid = restore ? 0 : driver->user; + gid = restore ? 0 : driver->group; + + for (i = 0 ; i < def->ndisks ; i++) { + if (restore && + (def->disks[i]->readonly || def->disks[i]->shared)) + continue; + + if (qemuDomainSetDiskOwnership(conn, def->disks[i], uid, gid) < 0) + return -1; + } + + for (i = 0 ; i < def->nhostdevs ; i++) { + if (qemuDomainSetHostdevOwnership(conn, def->hostdevs[i], uid, gid) < 0) + return -1; + } + + return 0; +} + static virDomainPtr qemudDomainLookupByName(virConnectPtr conn, const char *name); @@ -1377,14 +1575,39 @@ struct gemudHookData { }; static int qemudSecurityHook(void *data) { - struct gemudHookData *h = (struct gemudHookData *) data; + struct gemudHookData *h = (struct gemudHookData *) data; + + if (qemudDomainSetSecurityLabel(h->conn, h->driver, h->vm) < 0) { + qemudReportError(h->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Failed to set security label")); + return -1; + } + + if (h->driver->privileged) { + DEBUG("Dropping privileges of VM to %d:%d", h->driver->user, h->driver->group); + + if (qemuDomainSetAllDeviceOwnership(h->conn, h->driver, h->vm->def, 0) < 0) + return -1; - if (qemudDomainSetSecurityLabel(h->conn, h->driver, h->vm) < 0) { - qemudReportError(h->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - _("Failed to set security label")); + if (h->driver->group) { + if (setregid(h->driver->group, h->driver->group) < 0) { + virReportSystemError(NULL, errno, + _("cannot change to '%d' group"), + h->driver->group); return -1; + } } - return 0; + if (h->driver->user) { + if (setreuid(h->driver->user, h->driver->user) < 0) { + virReportSystemError(NULL, errno, + _("cannot change to '%d' user"), + h->driver->user); + return -1; + } + } + } + + return 0; } static int @@ -1662,6 +1885,10 @@ static void qemudShutdownVMDaemon(virConnectPtr conn ATTRIBUTE_UNUSED, VIR_FREE(vm->def->seclabel.imagelabel); } + if (qemuDomainSetAllDeviceOwnership(conn, driver, vm->def, 1) < 0) + VIR_WARN("Failed to restore all device ownership for %s", + vm->def->name); + if (qemudRemoveDomainStatus(conn, driver, vm) < 0) { VIR_WARN(_("Failed to remove domain status for %s"), vm->def->name); @@ -4227,12 +4454,20 @@ static int qemudDomainAttachDevice(virDomainPtr dom, case VIR_DOMAIN_DISK_DEVICE_FLOPPY: if (driver->securityDriver) driver->securityDriver->domainSetSecurityImageLabel(dom->conn, vm, dev->data.disk); + + if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 0) < 0) + goto cleanup; + ret = qemudDomainChangeEjectableMedia(dom->conn, vm, dev); break; case VIR_DOMAIN_DISK_DEVICE_DISK: if (driver->securityDriver) driver->securityDriver->domainSetSecurityImageLabel(dom->conn, vm, dev->data.disk); + + if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 0) < 0) + goto cleanup; + if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) { ret = qemudDomainAttachUsbMassstorageDevice(dom->conn, vm, dev); } else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI || @@ -4255,6 +4490,9 @@ static int qemudDomainAttachDevice(virDomainPtr dom, } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV && dev->data.hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && dev->data.hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) { + if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 0) < 0) + goto cleanup; + ret = qemudDomainAttachHostDevice(dom->conn, vm, dev); } else { qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, @@ -4267,8 +4505,11 @@ static int qemudDomainAttachDevice(virDomainPtr dom, ret = -1; cleanup: - if (ret < 0) + if (ret < 0) { + if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0) + VIR_WARN0("Fail to restore disk device ownership"); virDomainDeviceDefFree(dev); + } if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); @@ -4396,6 +4637,8 @@ static int qemudDomainDetachDevice(virDomainPtr dom, ret = qemudDomainDetachPciDiskDevice(dom->conn, vm, dev); if (driver->securityDriver) driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn, dev->data.disk); + if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0) + VIR_WARN0("Fail to restore disk device ownership"); } else qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, diff --git a/src/util.c b/src/util.c index 178ff0c..2420f8b 100644 --- a/src/util.c +++ b/src/util.c @@ -55,6 +55,7 @@ #include <netdb.h> #ifdef HAVE_GETPWUID_R #include <pwd.h> +#include <grp.h> #endif #if HAVE_CAPNG #include <cap-ng.h> @@ -1862,4 +1863,78 @@ char *virGetUserDirectory(virConnectPtr conn, return ret; } + + +int virGetUserID(virConnectPtr conn, + const char *name, + uid_t *uid) +{ + char *strbuf; + struct passwd pwbuf; + struct passwd *pw = NULL; + size_t strbuflen = sysconf(_SC_GETPW_R_SIZE_MAX); + + if (VIR_ALLOC_N(strbuf, strbuflen) < 0) { + virReportOOMError(conn); + return -1; + } + + /* + * From the manpage (terrifying but true): + * + * ERRORS + * 0 or ENOENT or ESRCH or EBADF or EPERM or ... + * The given name or uid was not found. + */ + if (getpwnam_r(name, &pwbuf, strbuf, strbuflen, &pw) != 0 || pw == NULL) { + virReportSystemError(conn, errno, + _("Failed to find user record for name '%s'"), + name); + VIR_FREE(strbuf); + return -1; + } + + *uid = pw->pw_uid; + + VIR_FREE(strbuf); + + return 0; +} + + +int virGetGroupID(virConnectPtr conn, + const char *name, + gid_t *gid) +{ + char *strbuf; + struct group grbuf; + struct group *gr = NULL; + size_t strbuflen = sysconf(_SC_GETGR_R_SIZE_MAX); + + if (VIR_ALLOC_N(strbuf, strbuflen) < 0) { + virReportOOMError(conn); + return -1; + } + + /* + * From the manpage (terrifying but true): + * + * ERRORS + * 0 or ENOENT or ESRCH or EBADF or EPERM or ... + * The given name or uid was not found. + */ + if (getgrnam_r(name, &grbuf, strbuf, strbuflen, &gr) != 0 || gr == NULL) { + virReportSystemError(conn, errno, + _("Failed to find group record for name '%s'"), + name); + VIR_FREE(strbuf); + return -1; + } + + *gid = gr->gr_gid; + + VIR_FREE(strbuf); + + return 0; +} #endif diff --git a/src/util.h b/src/util.h index 6dd005f..1a7286c 100644 --- a/src/util.h +++ b/src/util.h @@ -217,6 +217,12 @@ int virKillProcess(pid_t pid, int sig); #ifdef HAVE_GETPWUID_R char *virGetUserDirectory(virConnectPtr conn, uid_t uid); +int virGetUserID(virConnectPtr conn, + const char *name, + uid_t *uid); +int virGetGroupID(virConnectPtr conn, + const char *name, + gid_t *gid); #endif int virRandomInitialize(unsigned int seed); -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|