[libvirt] [PATCH 4/6] Support relabelling of USB and PCI devices

[Daniel Veillard]

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 at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/