* src/Makefile.am: Add usb.h and usb.h to libvirt_util.la * src/libvirt_private.syms: Export symbols * src/usb.c, src/usb.h: Helper APIs for USB host devices --- src/Makefile.am | 1 + src/hostusb.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++ src/hostusb.h | 45 ++++++++++++++++++++ src/libvirt_private.syms | 4 ++ 4 files changed, 153 insertions(+), 0 deletions(-) create mode 100644 src/hostusb.c create mode 100644 src/hostusb.h diff --git a/src/Makefile.am b/src/Makefile.am index 9567490..62946a6 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -51,6 +51,7 @@ UTIL_SOURCES = \ logging.c logging.h \ memory.c memory.h \ pci.c pci.h \ + hostusb.c hostusb.h \ qparams.c qparams.h \ threads.c threads.h \ threads-pthread.h \ diff --git a/src/hostusb.c b/src/hostusb.c new file mode 100644 index 0000000..07e10b1 --- /dev/null +++ b/src/hostusb.c @@ -0,0 +1,103 @@ +/* + * Copyright (C) 2009 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Authors: + * Daniel P. Berrange <berrange(a)redhat.com&gt; + */ + +#include <config.h> + +#include <dirent.h> +#include <fcntl.h> +#include <inttypes.h> +#include <limits.h> +#include <stdio.h> +#include <string.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <unistd.h> + +#include "hostusb.h" +#include "logging.h" +#include "memory.h" +#include "util.h" +#include "virterror_internal.h" + +#define USB_DEVFS "/dev/bus/usb/" +#define USB_ID_LEN 10 /* "XXXX XXXX" */ +#define USB_ADDR_LEN 8 /* "XXX:XXX" */ + +struct _usbDevice { + unsigned bus; + unsigned dev; + + char name[USB_ADDR_LEN]; /* domain:bus:slot.function */ + char id[USB_ID_LEN]; /* product vendor */ + char path[PATH_MAX]; +}; + +/* For virReportOOMError() and virReportSystemError() */ +#define VIR_FROM_THIS VIR_FROM_NONE + +#define usbReportError(conn, code, fmt...) \ + virReportErrorHelper(conn, VIR_FROM_NONE, code, __FILE__, \ + __FUNCTION__, __LINE__, fmt) + + +usbDevice * +usbGetDevice(virConnectPtr conn, + unsigned bus, + unsigned devno) +{ + usbDevice *dev; + + if (VIR_ALLOC(dev) < 0) { + virReportOOMError(conn); + return NULL; + } + + dev->bus = bus; + dev->dev = devno; + + snprintf(dev->name, sizeof(dev->name), "%.3o:%.3o", + dev->bus, dev->dev); + snprintf(dev->path, sizeof(dev->path), + USB_DEVFS "%03o/%03o", dev->bus, dev->dev); + + /* XXX fixme. this should be product/vendor */ + snprintf(dev->id, sizeof(dev->id), "%d %d", dev->bus, dev->dev); + + VIR_DEBUG("%s %s: initialized", dev->id, dev->name); + + return dev; +} + +void +usbFreeDevice(virConnectPtr conn ATTRIBUTE_UNUSED, usbDevice *dev) +{ + VIR_DEBUG("%s %s: freeing", dev->id, dev->name); + VIR_FREE(dev); +} + + +int usbDeviceFileIterate(virConnectPtr conn, + usbDevice *dev, + usbDeviceFileActor actor, + void *opaque) +{ + return (actor)(conn, dev, dev->path, opaque); +} diff --git a/src/hostusb.h b/src/hostusb.h new file mode 100644 index 0000000..634548e --- /dev/null +++ b/src/hostusb.h @@ -0,0 +1,45 @@ +/* + * Copyright (C) 2009 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Authors: + * Daniel P. Berrange <berrange(a)redhat.com&gt; + */ + +#ifndef __VIR_USB_H__ +#define __VIR_USB_H__ + +#include "internal.h" +#include "domain_conf.h" + +typedef struct _usbDevice usbDevice; + +usbDevice *usbGetDevice (virConnectPtr conn, + unsigned bus, + unsigned devno); +void usbFreeDevice (virConnectPtr conn, + usbDevice *dev); + +typedef int (*usbDeviceFileActor)(virConnectPtr conn, usbDevice *dev, + const char *path, void *opaque); + +int usbDeviceFileIterate(virConnectPtr conn, + usbDevice *dev, + usbDeviceFileActor actor, + void *opaque); + + +#endif /* __VIR_USB_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2bf4e15..67f7aa2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -401,6 +401,10 @@ virGetUserName; virGetUserID; virGetGroupID; +# usb.h +usbGetDevice; +usbFreeDevice; +usbDeviceFileIterate; # uuid.h virUUIDFormat; -- 1.6.2.5

* src/pci.h, src/pci.c: Helper for iterating over PCI device resource files * src/libvirt_private.syms: Export pciDeviceFileIterate --- src/libvirt_private.syms | 2 + src/pci.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++ src/pci.h | 9 +++++++- 3 files changed, 62 insertions(+), 1 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 67f7aa2..93d55ca 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -291,6 +291,8 @@ pciDeviceListNew; pciDeviceListFree; pciDeviceListAdd; pciDeviceListDel; +pciDeviceFileIterate; + # qparams.h qparam_get_query; diff --git a/src/pci.c b/src/pci.c index 96e5d6d..feaa6e8 100644 --- a/src/pci.c +++ b/src/pci.c @@ -1022,3 +1022,55 @@ pciDeviceListFind(pciDeviceList *list, pciDevice *dev) return list->devs[i]; return NULL; } + + +int pciDeviceFileIterate(virConnectPtr conn, + pciDevice *dev, + pciDeviceFileActor actor, + void *opaque) +{ + 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", + dev->domain, dev->bus, dev->slot, dev->function) < 0) { + virReportOOMError(conn); + goto cleanup; + } + + if (!(dir = opendir(pcidir))) { + virReportSystemError(conn, errno, + _("cannot open %s"), pcidir); + goto cleanup; + } + + while ((ent = readdir(dir)) != NULL) { + /* 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; + } + if ((actor)(conn, dev, file, opaque) < 0) + goto cleanup; + + VIR_FREE(file); + } + } + + ret = 0; + +cleanup: + if (dir) + closedir(dir); + VIR_FREE(file); + VIR_FREE(pcidir); + return ret; +} diff --git a/src/pci.h b/src/pci.h index 685b0af..d23ee0e 100644 --- a/src/pci.h +++ b/src/pci.h @@ -22,7 +22,6 @@ #ifndef __VIR_PCI_H__ #define __VIR_PCI_H__ -#include <config.h> #include "internal.h" typedef struct _pciDevice pciDevice; @@ -62,4 +61,12 @@ void pciDeviceListDel (virConnectPtr conn, pciDevice * pciDeviceListFind (pciDeviceList *list, pciDevice *dev); +typedef int (*pciDeviceFileActor)(virConnectPtr conn, pciDevice *dev, + const char *path, void *opaque); + +int pciDeviceFileIterate(virConnectPtr conn, + pciDevice *dev, + pciDeviceFileActor actor, + void *opaque); + #endif /* __VIR_PCI_H__ */ -- 1.6.2.5

* src/qemu_driver.c: Remove usbfs/sysfs iterator code and call into generic helper APIs instead when setting device permissions --- src/qemu_driver.c | 107 +++++++++++++++++++++++++++-------------------------- 1 files changed, 54 insertions(+), 53 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 3ebe802..e9a09df 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -65,6 +65,7 @@ #include "domain_conf.h" #include "node_device_conf.h" #include "pci.h" +#include "hostusb.h" #include "security.h" #include "cgroup.h" @@ -1683,31 +1684,62 @@ static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct qemud_driver * #ifdef __linux__ +struct qemuFileOwner { + uid_t uid; + gid_t gid; +}; + +static int qemuDomainSetHostdevUSBOwnershipActor(virConnectPtr conn, + usbDevice *dev ATTRIBUTE_UNUSED, + const char *file, void *opaque) +{ + struct qemuFileOwner *owner = opaque; + + if (chown(file, owner->uid, owner->gid) < 0) { + virReportSystemError(conn, errno, _("cannot set ownership on %s"), file); + return -1; + } + + return 0; +} + static int qemuDomainSetHostdevUSBOwnership(virConnectPtr conn, virDomainHostdevDefPtr def, uid_t uid, gid_t gid) { - char *usbpath = NULL; + struct qemuFileOwner owner = { uid, gid }; + int ret = -1; /* 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; - } + usbDevice *dev = usbGetDevice(conn, + def->source.subsys.u.usb.bus, + def->source.subsys.u.usb.device); + + if (!dev) + goto cleanup; + + ret = usbDeviceFileIterate(conn, dev, + qemuDomainSetHostdevUSBOwnershipActor, &owner); + + usbFreeDevice(conn, dev); +cleanup: + return ret; +} + +static int qemuDomainSetHostdevPCIOwnershipActor(virConnectPtr conn, + pciDevice *dev ATTRIBUTE_UNUSED, + const char *file, void *opaque) +{ + struct qemuFileOwner *owner = opaque; - 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); + if (chown(file, owner->uid, owner->gid) < 0) { + virReportSystemError(conn, errno, _("cannot set ownership on %s"), file); return -1; } - VIR_FREE(usbpath); return 0; } @@ -1716,54 +1748,23 @@ static int qemuDomainSetHostdevPCIOwnership(virConnectPtr conn, virDomainHostdevDefPtr def, uid_t uid, gid_t gid) { - char *pcidir = NULL; - char *file = NULL; - DIR *dir = NULL; + struct qemuFileOwner owner = { uid, gid }; 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; - } + pciDevice *dev = pciGetDevice(conn, + def->source.subsys.u.pci.domain, + def->source.subsys.u.pci.bus, + def->source.subsys.u.pci.slot, + def->source.subsys.u.pci.function); - if (!(dir = opendir(pcidir))) { - virReportSystemError(conn, errno, - _("cannot open %s"), pcidir); + if (!dev) 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; + ret = pciDeviceFileIterate(conn, dev, + qemuDomainSetHostdevPCIOwnershipActor, &owner); + pciFreeDevice(conn, dev); cleanup: - if (dir) - closedir(dir); - VIR_FREE(file); - VIR_FREE(pcidir); return ret; } #endif -- 1.6.2.5

* src/security.h: Driver API for relabelling host devices * src/security_selinux.c: Implement relabelling of PCI and USB devices * src/qemu_driver.c: Relabel USB/PCI devices before hotplug --- src/qemu_driver.c | 12 ++- src/security.h | 7 ++ src/security_selinux.c | 175 +++++++++++++++++++++++++++++++++++++++++++----- 3 files changed, 174 insertions(+), 20 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index e9a09df..d75e28e 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -5498,6 +5498,9 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn, if (qemuDomainSetDeviceOwnership(conn, driver, dev, 0) < 0) return -1; + if (driver->securityDriver && + driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm, dev->data.hostdev) < 0) + return -1; switch (hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: @@ -5566,9 +5569,6 @@ static int qemudDomainAttachDevice(virDomainPtr dom, } } - if (driver->securityDriver) - driver->securityDriver->domainSetSecurityImageLabel(dom->conn, vm, dev->data.disk); - switch (dev->data.disk->device) { case VIR_DOMAIN_DISK_DEVICE_CDROM: case VIR_DOMAIN_DISK_DEVICE_FLOPPY: @@ -5958,8 +5958,12 @@ static int qemudDomainDetachHostDevice(virConnectPtr conn, return -1; } + if (driver->securityDriver && + driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm, dev->data.hostdev) < 0) + VIR_WARN0("Failed to restore device labelling"); + if (qemuDomainSetDeviceOwnership(conn, driver, dev, 1) < 0) - VIR_WARN0("Fail to restore disk device ownership"); + VIR_WARN0("Failed to restore device ownership"); return ret; } diff --git a/src/security.h b/src/security.h index 5fc3086..40f9d95 100644 --- a/src/security.h +++ b/src/security.h @@ -36,6 +36,11 @@ typedef int (*virSecurityDomainRestoreImageLabel) (virConnectPtr conn, typedef int (*virSecurityDomainSetImageLabel) (virConnectPtr conn, virDomainObjPtr vm, virDomainDiskDefPtr disk); +typedef int (*virSecurityDomainRestoreHostdevLabel) (virConnectPtr conn, + virDomainHostdevDefPtr dev); +typedef int (*virSecurityDomainSetHostdevLabel) (virConnectPtr conn, + virDomainObjPtr vm, + virDomainHostdevDefPtr dev); typedef int (*virSecurityDomainGenLabel) (virConnectPtr conn, virDomainObjPtr sec); typedef int (*virSecurityDomainReserveLabel) (virConnectPtr conn, @@ -63,6 +68,8 @@ struct _virSecurityDriver { virSecurityDomainGetLabel domainGetSecurityLabel; virSecurityDomainSetLabel domainSetSecurityLabel; virSecurityDomainRestoreLabel domainRestoreSecurityLabel; + virSecurityDomainRestoreHostdevLabel domainRestoreSecurityHostdevLabel; + virSecurityDomainSetHostdevLabel domainSetSecurityHostdevLabel; /* * This is internally managed driver state and should only be accessed diff --git a/src/security_selinux.c b/src/security_selinux.c index 3b2e88f..5b7b038 100644 --- a/src/security_selinux.c +++ b/src/security_selinux.c @@ -25,6 +25,8 @@ #include "util.h" #include "memory.h" #include "logging.h" +#include "pci.h" +#include "hostusb.h" #define VIR_FROM_THIS VIR_FROM_SECURITY @@ -335,8 +337,10 @@ SELinuxSetFilecon(virConnectPtr conn, const char *path, char *tcon) } /* if the error complaint is related to an image hosted on - * an nfs mount, then ignore it. - * rhbz 517157 + * an nfs mount, or a usbfs/sysfs filesystem not supporting + * labelling, then just ignore it & hope for the best. + * The user hopefully set one of the neccessary SELinux + * virt_use_{nfs,usb,pci} boolean tunables to allow it... */ if (setfilecon_errno != EOPNOTSUPP) { virSecurityReportError(conn, VIR_ERR_ERROR, @@ -353,26 +357,14 @@ SELinuxSetFilecon(virConnectPtr conn, const char *path, char *tcon) } static int -SELinuxRestoreSecurityImageLabel(virConnectPtr conn, - virDomainDiskDefPtr disk) +SELinuxRestoreSecurityFileLabel(virConnectPtr conn, + const char *path) { struct stat buf; security_context_t fcon = NULL; int rc = -1; int err; char *newpath = NULL; - const char *path = disk->src; - - /* Don't restore labels on readoly/shared disks, because - * other VMs may still be accessing these - * Alternatively we could iterate over all running - * domains and try to figure out if it is in use, but - * this would not work for clustered filesystems, since - * we can't see running VMs using the file on other nodes - * Safest bet is thus to skip the restore step. - */ - if (disk->readonly || disk->shared) - return 0; if ((err = virFileResolveLink(path, &newpath)) < 0) { virReportSystemError(conn, err, @@ -393,6 +385,27 @@ err: } static int +SELinuxRestoreSecurityImageLabel(virConnectPtr conn, + virDomainDiskDefPtr disk) +{ + /* Don't restore labels on readoly/shared disks, because + * other VMs may still be accessing these + * Alternatively we could iterate over all running + * domains and try to figure out if it is in use, but + * this would not work for clustered filesystems, since + * we can't see running VMs using the file on other nodes + * Safest bet is thus to skip the restore step. + */ + if (disk->readonly || disk->shared) + return 0; + + if (!disk->src) + return 0; + + return SELinuxRestoreSecurityFileLabel(conn, disk->src); +} + +static int SELinuxSetSecurityImageLabel(virConnectPtr conn, virDomainObjPtr vm, virDomainDiskDefPtr disk) @@ -414,6 +427,126 @@ SELinuxSetSecurityImageLabel(virConnectPtr conn, return 0; } + +static int +SELinuxSetSecurityPCILabel(virConnectPtr conn, + pciDevice *dev ATTRIBUTE_UNUSED, + const char *file, void *opaque) +{ + virDomainObjPtr vm = opaque; + const virSecurityLabelDefPtr secdef = &vm->def->seclabel; + + return SELinuxSetFilecon(conn, file, secdef->imagelabel); +} + +static int +SELinuxSetSecurityHostdevLabel(virConnectPtr conn, + virDomainObjPtr vm, + virDomainHostdevDefPtr dev) + +{ + int ret = -1; + + if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + return 0; + + switch (dev->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: + break; + + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { + pciDevice *pci = pciGetDevice(conn, + dev->source.subsys.u.pci.domain, + dev->source.subsys.u.pci.bus, + dev->source.subsys.u.pci.slot, + dev->source.subsys.u.pci.function); + + if (!pci) + goto done; + + ret = pciDeviceFileIterate(conn, pci, SELinuxSetSecurityPCILabel, vm); + pciFreeDevice(conn, pci); + + break; + } + + default: + ret = 0; + break; + } + +done: + return ret; +} + +static int +SELinuxRestoreSecurityPCILabel(virConnectPtr conn, + pciDevice *dev ATTRIBUTE_UNUSED, + const char *file, + void *opaque ATTRIBUTE_UNUSED) +{ + return SELinuxRestoreSecurityFileLabel(conn, file); +} + +static int +SELinuxRestoreSecurityUSBLabel(virConnectPtr conn, + usbDevice *dev ATTRIBUTE_UNUSED, + const char *file, + void *opaque ATTRIBUTE_UNUSED) +{ + return SELinuxRestoreSecurityFileLabel(conn, file); +} + +static int +SELinuxRestoreSecurityHostdevLabel(virConnectPtr conn, + virDomainHostdevDefPtr dev) + +{ + int ret = -1; + + if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + return 0; + + switch (dev->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { + usbDevice *usb = usbGetDevice(conn, + dev->source.subsys.u.usb.bus, + dev->source.subsys.u.usb.device); + + if (!usb) + goto done; + + ret = usbDeviceFileIterate(conn, usb, SELinuxRestoreSecurityUSBLabel, NULL); + usbFreeDevice(conn, usb); + + break; + } + + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { + pciDevice *pci = pciGetDevice(conn, + dev->source.subsys.u.pci.domain, + dev->source.subsys.u.pci.bus, + dev->source.subsys.u.pci.slot, + dev->source.subsys.u.pci.function); + + if (!pci) + goto done; + + ret = pciDeviceFileIterate(conn, pci, SELinuxRestoreSecurityPCILabel, NULL); + pciFreeDevice(conn, pci); + + break; + } + + default: + ret = 0; + break; + } + +done: + return ret; +} + static int SELinuxRestoreSecurityLabel(virConnectPtr conn, virDomainObjPtr vm) @@ -422,6 +555,10 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn, int i; int rc = 0; if (secdef->imagelabel) { + for (i = 0 ; i < vm->def->nhostdevs ; i++) { + if (SELinuxRestoreSecurityHostdevLabel(conn, vm->def->hostdevs[i]) < 0) + rc = -1; + } for (i = 0 ; i < vm->def->ndisks ; i++) { if (SELinuxRestoreSecurityImageLabel(conn, vm->def->disks[i]) < 0) rc = -1; @@ -486,6 +623,10 @@ SELinuxSetSecurityLabel(virConnectPtr conn, if (SELinuxSetSecurityImageLabel(conn, vm, vm->def->disks[i]) < 0) return -1; } + for (i = 0 ; i < vm->def->nhostdevs ; i++) { + if (SELinuxSetSecurityHostdevLabel(conn, vm, vm->def->hostdevs[i]) < 0) + return -1; + } } return 0; @@ -503,4 +644,6 @@ virSecurityDriver virSELinuxSecurityDriver = { .domainGetSecurityLabel = SELinuxGetSecurityLabel, .domainRestoreSecurityLabel = SELinuxRestoreSecurityLabel, .domainSetSecurityLabel = SELinuxSetSecurityLabel, + .domainSetSecurityHostdevLabel = SELinuxSetSecurityHostdevLabel, + .domainRestoreSecurityHostdevLabel = SELinuxRestoreSecurityHostdevLabel, }; -- 1.6.2.5

Apart from DV's comments, ACK to patches 1-3 On Tue, 2009-09-01 at 16:28 +0100, Daniel P. Berrange wrote: What's this about? ... ... Either you're missing some code here, or I'm missing some understanding :-) The rest looks fine, ACK Cheers, Mark.

* src/security_selinux.c: Use virReportSystemError whereever an errno is involved * src/qemu_driver.c: Don't overwrite error message from the security driver --- src/qemu_driver.c | 13 +++-- src/security_selinux.c | 121 ++++++++++++++++++++++++++++-------------------- 2 files changed, 77 insertions(+), 57 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index d75e28e..fad518c 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1695,6 +1695,8 @@ static int qemuDomainSetHostdevUSBOwnershipActor(virConnectPtr conn, { struct qemuFileOwner *owner = opaque; + VIR_DEBUG("Setting ownership on %s to %d:%d", file, owner->uid, owner->gid); + if (chown(file, owner->uid, owner->gid) < 0) { virReportSystemError(conn, errno, _("cannot set ownership on %s"), file); return -1; @@ -1736,6 +1738,8 @@ static int qemuDomainSetHostdevPCIOwnershipActor(virConnectPtr conn, { struct qemuFileOwner *owner = opaque; + VIR_DEBUG("Setting ownership on %s to %d:%d", file, owner->uid, owner->gid); + if (chown(file, owner->uid, owner->gid) < 0) { virReportSystemError(conn, errno, _("cannot set ownership on %s"), file); return -1; @@ -1900,18 +1904,15 @@ static int qemudSecurityHook(void *data) { if (qemuAddToCgroup(h->driver, h->vm->def) < 0) return -1; - if (qemudDomainSetSecurityLabel(h->conn, h->driver, h->vm) < 0) { - qemudReportError(h->conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, - "%s", _("Failed to set security label")); + if (qemudDomainSetSecurityLabel(h->conn, h->driver, h->vm) < 0) 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; + DEBUG("Dropping privileges of VM to %d:%d", h->driver->user, h->driver->group); + if (h->driver->group) { if (setregid(h->driver->group, h->driver->group) < 0) { virReportSystemError(NULL, errno, diff --git a/src/security_selinux.c b/src/security_selinux.c index 5b7b038..bc295b1 100644 --- a/src/security_selinux.c +++ b/src/security_selinux.c @@ -106,24 +106,21 @@ SELinuxInitialize(virConnectPtr conn) { char *ptr = NULL; int fd = 0; - char ebuf[1024]; virRandomInitialize(time(NULL) ^ getpid()); fd = open(selinux_virtual_domain_context_path(), O_RDONLY); if (fd < 0) { - virSecurityReportError(conn, VIR_ERR_ERROR, - _("%s: cannot open SELinux virtual domain context file %s: %s"), - __func__,selinux_virtual_domain_context_path(), - virStrerror(errno, ebuf, sizeof ebuf)); + virReportSystemError(conn, errno, + _("cannot open SELinux virtual domain context file '%s'"), + selinux_virtual_domain_context_path()); return -1; } if (saferead(fd, default_domain_context, sizeof(default_domain_context)) < 0) { - virSecurityReportError(conn, VIR_ERR_ERROR, - _("%s: cannot read SELinux virtual domain context file %s: %s"), - __func__,selinux_virtual_domain_context_path(), - virStrerror(errno, ebuf, sizeof ebuf)); + virReportSystemError(conn, errno, + _("cannot read SELinux virtual domain context file %s"), + selinux_virtual_domain_context_path()); close(fd); return -1; } @@ -133,18 +130,16 @@ SELinuxInitialize(virConnectPtr conn) *ptr = '\0'; if ((fd = open(selinux_virtual_image_context_path(), O_RDONLY)) < 0) { - virSecurityReportError(conn, VIR_ERR_ERROR, - _("%s: cannot open SELinux virtual image context file %s: %s"), - __func__,selinux_virtual_image_context_path(), - virStrerror(errno, ebuf, sizeof ebuf)); + virReportSystemError(conn, errno, + _("cannot open SELinux virtual image context file %s"), + selinux_virtual_image_context_path()); return -1; } if (saferead(fd, default_image_context, sizeof(default_image_context)) < 0) { - virSecurityReportError(conn, VIR_ERR_ERROR, - _("%s: cannot read SELinux virtual image context file %s: %s"), - __func__,selinux_virtual_image_context_path(), - virStrerror(errno, ebuf, sizeof ebuf)); + virReportSystemError(conn, errno, + _("cannot read SELinux virtual image context file %s"), + selinux_virtual_image_context_path()); close(fd); return -1; } @@ -232,10 +227,8 @@ SELinuxReserveSecurityLabel(virConnectPtr conn, const char *mcs; if (getpidcon(vm->pid, &pctx) == -1) { - char ebuf[1024]; - virSecurityReportError(conn, VIR_ERR_ERROR, _("%s: error calling " - "getpidcon(): %s"), __func__, - virStrerror(errno, ebuf, sizeof ebuf)); + virReportSystemError(conn, errno, + _("unable to get PID %d security context"), vm->pid); return -1; } @@ -286,17 +279,16 @@ SELinuxGetSecurityLabel(virConnectPtr conn, security_context_t ctx; if (getpidcon(vm->pid, &ctx) == -1) { - char ebuf[1024]; - virSecurityReportError(conn, VIR_ERR_ERROR, _("%s: error calling " - "getpidcon(): %s"), __func__, - virStrerror(errno, ebuf, sizeof ebuf)); + virReportSystemError(conn, errno, + _("unable to get PID %d security context"), + vm->pid); return -1; } if (strlen((char *) ctx) >= VIR_SECURITY_LABEL_BUFLEN) { virSecurityReportError(conn, VIR_ERR_ERROR, - _("%s: security label exceeds " - "maximum lenth: %d"), __func__, + _("security label exceeds " + "maximum lenth: %d"), VIR_SECURITY_LABEL_BUFLEN - 1); return -1; } @@ -306,10 +298,8 @@ SELinuxGetSecurityLabel(virConnectPtr conn, sec->enforcing = security_getenforce(); if (sec->enforcing == -1) { - char ebuf[1024]; - virSecurityReportError(conn, VIR_ERR_ERROR, _("%s: error calling " - "security_getenforce(): %s"), __func__, - virStrerror(errno, ebuf, sizeof ebuf)); + virReportSystemError(conn, errno, "%s", + _("error calling security_getenforce()")); return -1; } @@ -319,7 +309,6 @@ SELinuxGetSecurityLabel(virConnectPtr conn, static int SELinuxSetFilecon(virConnectPtr conn, const char *path, char *tcon) { - char ebuf[1024]; security_context_t econ; VIR_INFO("Setting SELinux context on '%s' to '%s'", path, tcon); @@ -343,14 +332,14 @@ SELinuxSetFilecon(virConnectPtr conn, const char *path, char *tcon) * virt_use_{nfs,usb,pci} boolean tunables to allow it... */ if (setfilecon_errno != EOPNOTSUPP) { - virSecurityReportError(conn, VIR_ERR_ERROR, - _("%s: unable to set security context " - "'\%s\' on %s: %s."), __func__, - tcon, - path, - virStrerror(errno, ebuf, sizeof ebuf)); + virReportSystemError(conn, setfilecon_errno, + _("unable to set security context '%s' on '%s'"), + tcon, path); if (security_getenforce() == 1) return -1; + } else { + VIR_INFO("Setting security context '%s' on '%s' not supported", + tcon, path); } } return 0; @@ -366,6 +355,8 @@ SELinuxRestoreSecurityFileLabel(virConnectPtr conn, int err; char *newpath = NULL; + VIR_INFO("Restoring SELinux context on '%s'", path); + if ((err = virFileResolveLink(path, &newpath)) < 0) { virReportSystemError(conn, err, _("cannot resolve symlink %s"), path); @@ -440,6 +431,17 @@ SELinuxSetSecurityPCILabel(virConnectPtr conn, } static int +SELinuxSetSecurityUSBLabel(virConnectPtr conn, + usbDevice *dev ATTRIBUTE_UNUSED, + const char *file, void *opaque) +{ + virDomainObjPtr vm = opaque; + const virSecurityLabelDefPtr secdef = &vm->def->seclabel; + + return SELinuxSetFilecon(conn, file, secdef->imagelabel); +} + +static int SELinuxSetSecurityHostdevLabel(virConnectPtr conn, virDomainObjPtr vm, virDomainHostdevDefPtr dev) @@ -451,8 +453,24 @@ SELinuxSetSecurityHostdevLabel(virConnectPtr conn, return 0; switch (dev->source.subsys.type) { - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: - break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { + if (dev->source.subsys.u.usb.bus && dev->source.subsys.u.usb.device) { + usbDevice *usb = usbGetDevice(conn, + dev->source.subsys.u.usb.bus, + dev->source.subsys.u.usb.device); + + if (!usb) + goto done; + + ret = usbDeviceFileIterate(conn, usb, SELinuxSetSecurityUSBLabel, vm); + usbFreeDevice(conn, usb); + + break; + } else { + /* XXX deal with product/vendor better */ + ret = 0; + } + } case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { pciDevice *pci = pciGetDevice(conn, @@ -554,6 +572,9 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn, const virSecurityLabelDefPtr secdef = &vm->def->seclabel; int i; int rc = 0; + + VIR_DEBUG("Restoring security label on %s", vm->def->name); + if (secdef->imagelabel) { for (i = 0 ; i < vm->def->nhostdevs ; i++) { if (SELinuxRestoreSecurityHostdevLabel(conn, vm->def->hostdevs[i]) < 0) @@ -597,25 +618,23 @@ SELinuxSetSecurityLabel(virConnectPtr conn, /* TODO: verify DOI */ const virSecurityLabelDefPtr secdef = &vm->def->seclabel; int i; - char ebuf[1024]; if (!STREQ(drv->name, secdef->model)) { virSecurityReportError(conn, VIR_ERR_ERROR, - _("%s: security label driver mismatch: " - "\'%s\' model configured for domain, but " - "hypervisor driver is \'%s\'."), - __func__, secdef->model, drv->name); + _("security label driver mismatch: " + "'%s' model configured for domain, but " + "hypervisor driver is '%s'."), + secdef->model, drv->name); if (security_getenforce() == 1) - return -1; + return -1; } if (setexeccon(secdef->label) == -1) { - virSecurityReportError(conn, VIR_ERR_ERROR, - _("%s: unable to set security context " - "'\%s\': %s."), __func__, secdef->label, - virStrerror(errno, ebuf, sizeof ebuf)); + virReportSystemError(conn, errno, + _("unable to set security context '%s'"), + secdef->label); if (security_getenforce() == 1) - return -1; + return -1; } if (secdef->imagelabel) { -- 1.6.2.5

* src/security_selinux.c: matchpath() may well return NULL for many directories, to try and fallback to using parent directory label in that scenario. --- src/security_selinux.c | 29 ++++++++++++++++++++++++++++- 1 files changed, 28 insertions(+), 1 deletions(-) diff --git a/src/security_selinux.c b/src/security_selinux.c index bc295b1..0072360 100644 --- a/src/security_selinux.c +++ b/src/security_selinux.c @@ -366,8 +366,35 @@ SELinuxRestoreSecurityFileLabel(virConnectPtr conn, if (stat(newpath, &buf) != 0) goto err; - if (matchpathcon(newpath, buf.st_mode, &fcon) == 0) { + /* We try real hard to reset the context + * + * - Prefer an explicit context from policy for the file + * - Otherwise copy from parent directory. + * + * NB this is not just for disk images - PCI/USB device/sysfs + * files here too + */ + if (matchpathcon(newpath, buf.st_mode, &fcon) == 0) { rc = SELinuxSetFilecon(conn, newpath, fcon); + } else { + char *dir = strdup(newpath); + char *sep; + if (!dir) { + virReportOOMError(conn); + goto err; + } + VIR_WARN("Cannot find default context for %s, copying from parent", newpath); + sep = strrchr(dir, '/'); + if (sep) { + *sep = '\0'; + if (getfilecon(dir, &fcon) >= 0) + rc = SELinuxSetFilecon(conn, newpath, fcon); + else + VIR_ERROR("Unable to get security context for directory %s", dir); + } else { + VIR_ERROR("File %s did not contain a directory separator", newpath); + } + VIR_FREE(dir); } err: VIR_FREE(fcon); -- 1.6.2.5

On Tue, Sep 01, 2009 at 01:00:13PM -0400, Stephen Smalley wrote: That describes what I always imagined would happen, but in my recent testing it doesn't appear to be doing that in practice in some cases eg, running matchpathcon("/media/usbdisk/virtual-images/demo.img") returned an error. Likewise for /sys/bus/pci/devices/NNNN.NN.NN.NN/*. I was testing this on a Fedora 11 host. Perhaps the policy is simply missing some rules This code is being run upon VM shutdown, to get rid of the dynamic label that was assigned at VM startup. THe file already exists, so the default rule for file creation wouldn't come into effect at this point, so I was aiming to replicate that behaviour for the existing file. If we could guarentee that matchpathcon($PATH) would always return something usable, I would happily kill this code Regard, Daniel -- |: 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 :|