[libvirt] Resubmission: [PATCH 1/6] sVirt AppArmor security driver

[Daniel P. Berrange]

On Tue, Sep 08, 2009 at 04:19:59PM -0500, Jamie Strandboge wrote:
> On Tue, 08 Sep 2009, Jamie Strandboge wrote:
> 
> > > [PATCH 1*]
> > > patch_1a_reenable-nonfile-labels.patch:
> > > When James Morris originally submitted his sVirt patches (as seen in
> > > libvirt 0.6.1), he did not require on disk labelling for
> > > virSecurityDomainRestoreImageLabel. A later commit[2] changed this
> > > behavior to assume on disk labelling, which halts implementations for
> > > path-based MAC systems such as AppArmor and TOMOYO where
> > > vm->def->seclabel is required to obtain the label. This patch simply
> > > adds the 'virDomainObjPtr vm' argument back to *RestoreImageLabel.
> > > 
> > > patch_1b_optional.patch:
> > > Due to the above change, 'make syntax-check' fails because
> > > SELinuxRestoreSecurityImageLabel() does not use the 'virDomainObjPtr
> > > vm'. patch_1b_optional.patch is a simple patch to fix this by checking
> > > if vm->def->seclabel == NULL and returns with error if it does. I
> > > realize this may not be desired in the long term, but it should be
> > > harmless enough to include.

The 'ATTRIBUTE_UNUSED' anotation takes care of this much better.


> diff -Nurp ./libvirt.orig/src/qemu_driver.c ./libvirt/src/qemu_driver.c
> --- ./libvirt.orig/src/qemu_driver.c	2009-09-08 13:00:00.000000000 -0500
> +++ ./libvirt/src/qemu_driver.c	2009-09-08 15:32:22.000000000 -0500
> @@ -6144,7 +6144,7 @@ static int qemudDomainDetachDevice(virDo
>           dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO)) {
>          ret = qemudDomainDetachPciDiskDevice(dom->conn, vm, dev);
>          if (driver->securityDriver)
> -            driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn, dev->data.disk);
> +            driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn, vm, dev->data.disk);
>          if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0)
>              VIR_WARN0("Fail to restore disk device ownership");
>      } else if (dev->type == VIR_DOMAIN_DEVICE_NET) {
> diff -Nurp ./libvirt.orig/src/security.h ./libvirt/src/security.h
> --- ./libvirt.orig/src/security.h	2009-08-17 11:00:40.000000000 -0500
> +++ ./libvirt/src/security.h	2009-09-08 15:32:22.000000000 -0500
> @@ -32,6 +32,7 @@ typedef virSecurityDriverStatus (*virSec
>  typedef int (*virSecurityDriverOpen) (virConnectPtr conn,
>                                        virSecurityDriverPtr drv);
>  typedef int (*virSecurityDomainRestoreImageLabel) (virConnectPtr conn,
> +                                                   virDomainObjPtr vm,
>                                                     virDomainDiskDefPtr disk);
>  typedef int (*virSecurityDomainSetImageLabel) (virConnectPtr conn,
>                                                 virDomainObjPtr vm,
> diff -Nurp ./libvirt.orig/src/security_selinux.c ./libvirt/src/security_selinux.c
> --- ./libvirt.orig/src/security_selinux.c	2009-09-02 14:34:08.000000000 -0500
> +++ ./libvirt/src/security_selinux.c	2009-09-08 15:32:22.000000000 -0500
> @@ -354,6 +354,7 @@ SELinuxSetFilecon(virConnectPtr conn, co
>  
>  static int
>  SELinuxRestoreSecurityImageLabel(virConnectPtr conn,
> +                                 virDomainObjPtr vm,
>                                   virDomainDiskDefPtr disk)
>  {
>      struct stat buf;
> @@ -423,7 +429,8 @@ SELinuxRestoreSecurityLabel(virConnectPt
>      int rc = 0;
>      if (secdef->imagelabel) {
>          for (i = 0 ; i < vm->def->ndisks ; i++) {
> -            if (SELinuxRestoreSecurityImageLabel(conn, vm->def->disks[i]) < 0)
> +            if (SELinuxRestoreSecurityImageLabel(conn, vm,
> +                                                 vm->def->disks[i]) < 0)
>                  rc = -1;
>          }
>          VIR_FREE(secdef->model);


ACK to this part


> diff -Nurp ./libvirt.orig/src/security_selinux.c ./libvirt/src/security_selinux.c
> --- ./libvirt.orig/src/security_selinux.c	2009-09-02 14:34:08.000000000 -0500
> +++ ./libvirt/src/security_selinux.c	2009-09-08 15:32:22.000000000 -0500
> @@ -364,6 +365,11 @@ SELinuxRestoreSecurityImageLabel(virConn
>      char *newpath = NULL;
>      const char *path = disk->src;
>  
> +    if (&vm->def->seclabel == NULL)  {
> +        virSecurityReportError(conn, VIR_ERR_ERROR, _("seclabel is NULL"));
> +        return rc;
> +    }
> +
>      /* Don't restore labels on readoly/shared disks, because
>       * other VMs may still be accessing these
>       * Alternatively we could iterate over all running

This should have ATTRIBUTE_USED instead


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 :|