[libvirt] [PATCH 0/6] Automatic sVirt management of host device labelling
This patch series extends the libvirt security driver API, and sVirt implementation to cover management of host device labelling. Previously users would have to set a global boolean tunable virt_use_pci/usb to allow all domains access to all host devices. With this series applied libvirt will automatically relabel only the individual PCI/USB devices which are assigned to a guest. ie it should make host device assignment 'just work' when sVirt is enforcing, and improve security It also attempts to address a problem with restoration of disk labels. The current code uses matchpathcon() to find the defalt label for a path. This works fine for locations which have a defined label in the policy (eg like /var/lib/libvirt/images), but if storing disk images in non-defualt locations, eg a external USB drive mounted under a place like /media/myusbdisk/virtual-images/, matchpathcon() returns NULL. In this scenario the disk would remain labelled with the MCS level specific to the just stopped VM. Since MCS labels are allocated on demand on each boot, this could allow a future VMs to access disks that it ought not to be able to. Dan Walsh suggested that we default to using the label defined for matchpathcon("/var/libvirt/images/00-DEFAULT") in this case, but this doesn't work for restoring USB/PCI device labels[1]. In all the case I've had this problem so far, the files' original label matched that of the directory it was contained in, so this patch just uses the containing directory's label when restoring labels. Dan didn't like this idea when I first mentioned it in IRC though, so perhaps I need to implement different logic still... ? Regards, Daniel [1] PCI device access from VMs requires labelling /sys/bus/pci/devices/$DOMAIN:$BUS:$SLOT:FUNCTION/{config, resource*, rom} while USB device access requires labelling /dev/bus/usb/$BUS/$DEVICE
* 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@redhat.com> + */ + +#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@redhat.com> + */ + +#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
On Tue, Sep 01, 2009 at 04:28:54PM +0100, Daniel P. Berrange wrote:
* 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
Sounds good [...]
+/* For virReportOOMError() and virReportSystemError() */ +#define VIR_FROM_THIS VIR_FROM_NONE
VIR_FROM_STORAGE might be a bit more precise
+usbDevice * +usbGetDevice(virConnectPtr conn, + unsigned bus, + unsigned devno) +{
The function code should probably be #ifdef linux
+ 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;
with a #else emitting an error Those tiny things apart, ACK, looks fine ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
* 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
On Tue, Sep 01, 2009 at 04:28:55PM +0100, Daniel P. Berrange wrote: [...]
@@ -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) {
hum "%s/devices/%04x:%02x:%02x.%x ", PCI_SYSFS, ... would be a bit better I guess Fine, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
* 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
On Tue, Sep 01, 2009 at 04:28:56PM +0100, Daniel P. Berrange wrote:
* src/qemu_driver.c: Remove usbfs/sysfs iterator code and call into generic helper APIs instead when setting device permissions
Yup, nice cleanup, ACK, just one thing, it would be good in 1/6 and 2/6 to detail as a comment on the iterators in the headers the expected return value and the fact that non-zero will break iteration and be the returned value from top level. Usually when having to program those callbacks it's the classic pitfall :) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
* 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
On Tue, Sep 01, 2009 at 04:28:57PM +0100, Daniel P. Berrange wrote:
* 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;
shouldn't we test against the entry point being null ? if (driver->securityDriver && driver->securityDriver->domainSetSecurityHostdevLabel && driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm, dev->data.hostdev) < 0) Also the long line should probably be split. [...]
+ if (driver->securityDriver && + driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm, dev->data.hostdev) < 0) + VIR_WARN0("Failed to restore device labelling"); +
idem [...]
static int +SELinuxRestoreSecurityImageLabel(virConnectPtr conn, + virDomainDiskDefPtr disk) +{ + /* Don't restore labels on readoly/shared disks, because
typo readonly
+ * 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); +}
it would be nice if we could check the type of the opaque passed in some ways, even if we know it was called a few line below.
+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; +}
Looks fine, ACK, but I won't pretend I understand all the implications of the security calls though, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Apart from DV's comments, ACK to patches 1-3 On Tue, 2009-09-01 at 16:28 +0100, Daniel P. Berrange wrote:
* 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);
What's this about? ...
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 ... @@ -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;
Either you're missing some code here, or I'm missing some understanding :-) The rest looks fine, ACK Cheers, Mark.
On Thu, Sep 03, 2009 at 01:04:30PM +0100, Mark McLoughlin wrote:
Apart from DV's comments, ACK to patches 1-3
On Tue, 2009-09-01 at 16:28 +0100, Daniel P. Berrange wrote:
* 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);
What's this about?
The very same call is issued a few lines later :-)
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 ... @@ -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;
Either you're missing some code here, or I'm missing some understanding :-)
Bizarrely the code secretly moved itself into the next patch while I wasn't looking :-) 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 :|
* 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
On Tue, 2009-09-01 at 16:28 +0100, Daniel P. Berrange wrote:
* src/security_selinux.c: Use virReportSystemError whereever an errno is involved * src/qemu_driver.c: Don't overwrite error message from the security driver
ACK, looks good - poor wee errno was being forgotten
@@ -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; + } + }
Ahrrr! There she is! :-) Looks good, I'd perhaps have just passed the image label as the opaque pointer to the iterator but ... Cheers, Mark.
* 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, 2009-09-01 at 16:28 +0100, Daniel P. Berrange wrote:
* src/security_selinux.c: matchpath() may well return NULL for many directories, to try and fallback to using parent directory label in that scenario.
When have you seen this happen? matchpathcon() ultimately should fall back to the top-level regex (/.*) and map any otherwise unmatched files to default_t, and should generally have a fallback regex for each subtree (e.g. any file under /dev that isn't otherwise matched would get device_t). So I wouldn't expect this to happen. Also, files will inherit their SELinux type from the parent directory by default upon creation unless a type transition rule is specified, so it isn't clear why you need to replicate this copying from parent behavior in the application.
--- 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); -- Stephen Smalley National Security Agency
On Tue, Sep 01, 2009 at 01:00:13PM -0400, Stephen Smalley wrote:
On Tue, 2009-09-01 at 16:28 +0100, Daniel P. Berrange wrote:
* src/security_selinux.c: matchpath() may well return NULL for many directories, to try and fallback to using parent directory label in that scenario.
When have you seen this happen? matchpathcon() ultimately should fall back to the top-level regex (/.*) and map any otherwise unmatched files to default_t, and should generally have a fallback regex for each subtree (e.g. any file under /dev that isn't otherwise matched would get device_t). So I wouldn't expect this to happen.
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
Also, files will inherit their SELinux type from the parent directory by default upon creation unless a type transition rule is specified, so it isn't clear why you need to replicate this copying from parent behavior in the application.
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
--- 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); -- Stephen Smalley National Security Agency
-- |: 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 :|
On Tue, 2009-09-01 at 18:10 +0100, Daniel P. Berrange wrote:
On Tue, Sep 01, 2009 at 01:00:13PM -0400, Stephen Smalley wrote:
On Tue, 2009-09-01 at 16:28 +0100, Daniel P. Berrange wrote:
* src/security_selinux.c: matchpath() may well return NULL for many directories, to try and fallback to using parent directory label in that scenario.
When have you seen this happen? matchpathcon() ultimately should fall back to the top-level regex (/.*) and map any otherwise unmatched files to default_t, and should generally have a fallback regex for each subtree (e.g. any file under /dev that isn't otherwise matched would get device_t). So I wouldn't expect this to happen.
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
Ah, I had forgotten about the behavior in the case where the path is marked with <<none>> in the file_contexts configuration, in which case matchpathcon() does return NULL as you describe. That gets used for two situations: - mount point directories for filesystems that do not support labeling, like /sys and /selinux. That should be obsoleted by the support in kernels >= 2.6.30 to report seclabel support in /proc/mounts, which gets used by setfiles when available. So we can likely remove those <<none>> entries going forward. - directories that contain runtime-created files whose labels we do not wish to reset, like /tmp or /var/run. Those entries are still necessary to avoid clobbering runtime labels on such files upon a relabel. So I guess you do need this.
Also, files will inherit their SELinux type from the parent directory by default upon creation unless a type transition rule is specified, so it isn't clear why you need to replicate this copying from parent behavior in the application.
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
-- Stephen Smalley National Security Agency
On Tue, 2009-09-01 at 13:33 -0400, Stephen Smalley wrote:
On Tue, 2009-09-01 at 18:10 +0100, Daniel P. Berrange wrote:
On Tue, Sep 01, 2009 at 01:00:13PM -0400, Stephen Smalley wrote:
On Tue, 2009-09-01 at 16:28 +0100, Daniel P. Berrange wrote:
* src/security_selinux.c: matchpath() may well return NULL for many directories, to try and fallback to using parent directory label in that scenario.
When have you seen this happen? matchpathcon() ultimately should fall back to the top-level regex (/.*) and map any otherwise unmatched files to default_t, and should generally have a fallback regex for each subtree (e.g. any file under /dev that isn't otherwise matched would get device_t). So I wouldn't expect this to happen.
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
Ah, I had forgotten about the behavior in the case where the path is marked with <<none>> in the file_contexts configuration, in which case matchpathcon() does return NULL as you describe. That gets used for two situations: - mount point directories for filesystems that do not support labeling, like /sys and /selinux. That should be obsoleted by the support in kernels >= 2.6.30 to report seclabel support in /proc/mounts, which gets used by setfiles when available. So we can likely remove those <<none>> entries going forward. - directories that contain runtime-created files whose labels we do not wish to reset, like /tmp or /var/run. Those entries are still necessary to avoid clobbering runtime labels on such files upon a relabel.
So I guess you do need this.
Actually, why can't you just save the original file label somewhere (i.e. call getfilecon() and store that) and then just use that original label when restoring the label. Then you don't need to use matchpathcon() or this extra logic at all.
Also, files will inherit their SELinux type from the parent directory by default upon creation unless a type transition rule is specified, so it isn't clear why you need to replicate this copying from parent behavior in the application.
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
-- Stephen Smalley National Security Agency
On 09/01/2009 02:20 PM, Stephen Smalley wrote:
On Tue, 2009-09-01 at 13:33 -0400, Stephen Smalley wrote:
On Tue, 2009-09-01 at 18:10 +0100, Daniel P. Berrange wrote:
On Tue, Sep 01, 2009 at 01:00:13PM -0400, Stephen Smalley wrote:
On Tue, 2009-09-01 at 16:28 +0100, Daniel P. Berrange wrote:
* src/security_selinux.c: matchpath() may well return NULL for many directories, to try and fallback to using parent directory label in that scenario.
When have you seen this happen? matchpathcon() ultimately should fall back to the top-level regex (/.*) and map any otherwise unmatched files to default_t, and should generally have a fallback regex for each subtree (e.g. any file under /dev that isn't otherwise matched would get device_t). So I wouldn't expect this to happen.
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
Ah, I had forgotten about the behavior in the case where the path is marked with <<none>> in the file_contexts configuration, in which case matchpathcon() does return NULL as you describe. That gets used for two situations: - mount point directories for filesystems that do not support labeling, like /sys and /selinux. That should be obsoleted by the support in kernels >= 2.6.30 to report seclabel support in /proc/mounts, which gets used by setfiles when available. So we can likely remove those <<none>> entries going forward. - directories that contain runtime-created files whose labels we do not wish to reset, like /tmp or /var/run. Those entries are still necessary to avoid clobbering runtime labels on such files upon a relabel.
So I guess you do need this.
Actually, why can't you just save the original file label somewhere (i.e. call getfilecon() and store that) and then just use that original label when restoring the label. Then you don't need to use matchpathcon() or this extra logic at all.
Also, files will inherit their SELinux type from the parent directory by default upon creation unless a type transition rule is specified, so it isn't clear why you need to replicate this copying from parent behavior in the application.
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
In Rawhide, these are the files that are labeled as <<none>>
Which ones should I eliminate. I tend to agree with Steven, the best solution to the problem would be MAKEFILE/DEVICE getfilecon(path, oldcon) setfilecon(path, svirtcon) run virtual machine, end virtual machine setfilecon(path, oldcon)
participants (5)
-
Daniel J Walsh -
Daniel P. Berrange -
Daniel Veillard -
Mark McLoughlin -
Stephen Smalley