[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 01/12] Make security drivers responsible for checking dynamic vs static labelling



On Wed, Jan 20, 2010 at 03:14:58PM +0000, Daniel P. Berrange wrote:
> The QEMU driver is doing 90% of the calls to check for static vs
> dynamic labelling. Except it is forgetting todo so in many places,
> in particular hotplug is mistakenly assigning disk labels. Move
> all this logic into the security drivers themselves, so the HV
> drivers don't have to think about it.
> 
> * src/security/security_driver.h: Add virDomainObjPtr parameter
>   to virSecurityDomainRestoreHostdevLabel and to
>   virSecurityDomainRestoreSavedStateLabel
> * src/security/security_selinux.c, src/security/security_apparmor.c:
>   Add explicit checks for VIR_DOMAIN_SECLABEL_STATIC and skip all
>   chcon() code in those cases
> * src/qemu/qemu_driver.c: Remove all checks for VIR_DOMAIN_SECLABEL_STATIC
>   or VIR_DOMAIN_SECLABEL_DYNAMIC. Add missing checks for possibly NULL
>   driver entry points.
> ---
>  src/qemu/qemu_driver.c           |   40 ++++++++++++-------
>  src/security/security_apparmor.c |   46 +++++++++++++++-------
>  src/security/security_driver.h   |    2 +
>  src/security/security_selinux.c  |   78 ++++++++++++++++++++++++++++----------
>  4 files changed, 117 insertions(+), 49 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 2d80774..446f6ff 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -855,8 +855,7 @@ qemuReconnectDomain(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaq
>          goto error;
>      }
>  
> -    if (obj->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
> -        driver->securityDriver &&
> +    if (driver->securityDriver &&
>          driver->securityDriver->domainReserveSecurityLabel &&
>          driver->securityDriver->domainReserveSecurityLabel(NULL, obj) < 0)
>          goto error;
> @@ -2441,11 +2440,14 @@ cleanup:
>  
>  static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm)
>  {
> -    if (vm->def->seclabel.label != NULL)
> -        if (driver->securityDriver && driver->securityDriver->domainSetSecurityLabel)
> -            return driver->securityDriver->domainSetSecurityLabel(conn, driver->securityDriver,
> -                                                                 vm);
> -    return 0;
> +    int rc = 0;
> +
> +    if (driver->securityDriver &&
> +        driver->securityDriver->domainSetSecurityLabel &&
> +        driver->securityDriver->domainSetSecurityLabel(conn, driver->securityDriver, vm) < 0)
> +        rc = -1;
> +
> +    return rc;
>  }
>  
>  
> @@ -2758,8 +2760,7 @@ static int qemudStartVMDaemon(virConnectPtr conn,
>  
>      /* If you are using a SecurityDriver with dynamic labelling,
>         then generate a security label for isolation */
> -    if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
> -        driver->securityDriver &&
> +    if (driver->securityDriver &&
>          driver->securityDriver->domainGenSecurityLabel &&
>          driver->securityDriver->domainGenSecurityLabel(conn, vm) < 0)
>          return -1;
> @@ -3054,7 +3055,8 @@ static void qemudShutdownVMDaemon(virConnectPtr conn,
>      virKillProcess(vm->pid, SIGKILL);
>  
>      /* Reset Security Labels */
> -    if (driver->securityDriver)
> +    if (driver->securityDriver &&
> +        driver->securityDriver->domainRestoreSecurityLabel)
>          driver->securityDriver->domainRestoreSecurityLabel(conn, vm);
>  
>      /* Clear out dynamically assigned labels */
> @@ -4166,7 +4168,7 @@ static int qemudDomainSave(virDomainPtr dom,
>  
>      if (driver->securityDriver &&
>          driver->securityDriver->domainRestoreSavedStateLabel &&
> -        driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, path) == -1)
> +        driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, vm, path) == -1)
>          goto endjob;
>  
>      ret = 0;
> @@ -4296,7 +4298,7 @@ static int qemudDomainCoreDump(virDomainPtr dom,
>  
>      if (driver->securityDriver &&
>          driver->securityDriver->domainRestoreSavedStateLabel &&
> -        driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, path) == -1)
> +        driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, vm, path) == -1)
>          goto endjob;
>  
>  endjob:
> @@ -5871,6 +5873,7 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn,
>      if (qemuDomainSetDeviceOwnership(conn, driver, dev, 0) < 0)
>          return -1;
>      if (driver->securityDriver &&
> +        driver->securityDriver->domainSetSecurityHostdevLabel &&
>          driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm, dev->data.hostdev) < 0)
>          return -1;
>  
> @@ -5947,7 +5950,8 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
>          switch (dev->data.disk->device) {
>          case VIR_DOMAIN_DISK_DEVICE_CDROM:
>          case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
> -            if (driver->securityDriver)
> +            if (driver->securityDriver &&
> +                driver->securityDriver->domainSetSecurityImageLabel)
>                  driver->securityDriver->domainSetSecurityImageLabel(dom->conn, vm, dev->data.disk);
>  
>              if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 0) < 0)
> @@ -5957,7 +5961,8 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
>              break;
>  
>          case VIR_DOMAIN_DISK_DEVICE_DISK:
> -            if (driver->securityDriver)
> +            if (driver->securityDriver &&
> +                driver->securityDriver->domainSetSecurityImageLabel)
>                  driver->securityDriver->domainSetSecurityImageLabel(dom->conn, vm, dev->data.disk);
>  
>              if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 0) < 0)
> @@ -6347,6 +6352,7 @@ static int qemudDomainDetachHostDevice(virConnectPtr conn,
>      }
>  
>      if (driver->securityDriver &&
> +        driver->securityDriver->domainSetSecurityHostdevLabel &&
>          driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm, dev->data.hostdev) < 0)
>          VIR_WARN0("Failed to restore device labelling");
>  
> @@ -6392,7 +6398,8 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
>          dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK &&
>          dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
>          ret = qemudDomainDetachPciDiskDevice(dom->conn, driver, vm, dev);
> -        if (driver->securityDriver)
> +        if (driver->securityDriver &&
> +            driver->securityDriver->domainRestoreSecurityImageLabel)
>              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");
> @@ -6409,6 +6416,9 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
>          }
>      } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) {
>          ret = qemudDomainDetachHostDevice(dom->conn, driver, vm, dev);
> +        if (driver->securityDriver &&
> +            driver->securityDriver->domainRestoreSecurityHostdevLabel)
> +            driver->securityDriver->domainRestoreSecurityHostdevLabel(dom->conn, vm, dev->data.hostdev);
>      } else {
>          qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
>                           "%s", _("This type of device cannot be hot unplugged"));
> diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
> index 5844768..0f9ff95 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -327,6 +327,9 @@ AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
>      int rc = -1;
>      char *profile_name = NULL;
>  
> +    if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
> +        return 0;
> +
>      if ((vm->def->seclabel.label) ||
>          (vm->def->seclabel.model) || (vm->def->seclabel.imagelabel)) {
>          virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
> @@ -425,7 +428,7 @@ AppArmorRestoreSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
>      const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
>      int rc = 0;
>  
> -    if (secdef->imagelabel) {
> +    if (secdef->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
>          if ((rc = remove_profile(secdef->label)) != 0) {
>              virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
>                                     _("could not remove profile for \'%s\'"),
> @@ -486,21 +489,23 @@ AppArmorRestoreSecurityImageLabel(virConnectPtr conn,
>      int rc = -1;
>      char *profile_name = NULL;
>  
> -    if (secdef->imagelabel) {
> -        if ((profile_name = get_profile_name(conn, vm)) == NULL)
> -            return rc;
> +    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
> +        return 0;
>  
> -        /* Update the profile only if it is loaded */
> -        if (profile_loaded(secdef->imagelabel) >= 0) {
> -            if (load_profile(conn, secdef->imagelabel, vm, NULL) < 0) {
> -                virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
> -                                       _("cannot update AppArmor profile "
> -                                       "\'%s\'"),
> -                                       secdef->imagelabel);
> -                goto clean;
> -            }
> +    if ((profile_name = get_profile_name(conn, vm)) == NULL)
> +        return rc;
> +
> +    /* Update the profile only if it is loaded */
> +    if (profile_loaded(secdef->imagelabel) >= 0) {
> +        if (load_profile(conn, secdef->imagelabel, vm, NULL) < 0) {
> +            virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                                   _("cannot update AppArmor profile "
> +                                     "\'%s\'"),
> +                                   secdef->imagelabel);
> +            goto clean;
>          }
>      }
> +
>      rc = 0;
>    clean:
>      VIR_FREE(profile_name);
> @@ -517,6 +522,9 @@ AppArmorSetSecurityImageLabel(virConnectPtr conn,
>      int rc = -1;
>      char *profile_name;
>  
> +    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
> +        return 0;
> +
>      if (!disk->src)
>          return 0;
>  
> @@ -576,19 +584,29 @@ AppArmorReserveSecurityLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
>  
>  static int
>  AppArmorSetSecurityHostdevLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
> -                                virDomainObjPtr vm ATTRIBUTE_UNUSED,
> +                                virDomainObjPtr vm,
>                                  virDomainHostdevDefPtr dev ATTRIBUTE_UNUSED)
>  
>  {
> +    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
> +
> +    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
> +        return 0;
> +
>      /* TODO: call load_profile with an update vm->def */
>      return 0;
>  }
>  
>  static int
>  AppArmorRestoreSecurityHostdevLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
> +                                    virDomainObjPtr vm,
>                                      virDomainHostdevDefPtr dev ATTRIBUTE_UNUSED)
>  
>  {
> +    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
> +    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
> +        return 0;
> +
>      /* TODO: call load_profile (needs virDomainObjPtr vm) */
>      return 0;
>  }
> diff --git a/src/security/security_driver.h b/src/security/security_driver.h
> index 5514962..97c9002 100644
> --- a/src/security/security_driver.h
> +++ b/src/security/security_driver.h
> @@ -38,6 +38,7 @@ typedef int (*virSecurityDomainSetImageLabel) (virConnectPtr conn,
>                                                 virDomainObjPtr vm,
>                                                 virDomainDiskDefPtr disk);
>  typedef int (*virSecurityDomainRestoreHostdevLabel) (virConnectPtr conn,
> +                                                     virDomainObjPtr vm,
>                                                       virDomainHostdevDefPtr dev);
>  typedef int (*virSecurityDomainSetHostdevLabel) (virConnectPtr conn,
>                                                   virDomainObjPtr vm,
> @@ -46,6 +47,7 @@ typedef int (*virSecurityDomainSetSavedStateLabel) (virConnectPtr conn,
>                                                      virDomainObjPtr vm,
>                                                      const char *savefile);
>  typedef int (*virSecurityDomainRestoreSavedStateLabel) (virConnectPtr conn,
> +                                                        virDomainObjPtr vm,
>                                                          const char *savefile);
>  typedef int (*virSecurityDomainGenLabel) (virConnectPtr conn,
>                                            virDomainObjPtr sec);
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index cb585ed..c48f4c8 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -165,6 +165,9 @@ SELinuxGenSecurityLabel(virConnectPtr conn,
>      int c1 = 0;
>      int c2 = 0;
>  
> +    if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
> +        return 0;
> +
>      if (vm->def->seclabel.label ||
>          vm->def->seclabel.model ||
>          vm->def->seclabel.imagelabel) {
> @@ -225,6 +228,9 @@ SELinuxReserveSecurityLabel(virConnectPtr conn,
>      context_t ctx = NULL;
>      const char *mcs;
>  
> +    if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
> +        return 0;
> +
>      if (getpidcon(vm->pid, &pctx) == -1) {
>          virReportSystemError(conn, errno,
>                               _("unable to get PID %d security context"), vm->pid);
> @@ -376,9 +382,14 @@ err:
>  
>  static int
>  SELinuxRestoreSecurityImageLabel(virConnectPtr conn,
> -                                 virDomainObjPtr vm ATTRIBUTE_UNUSED,
> +                                 virDomainObjPtr vm,
>                                   virDomainDiskDefPtr disk)
>  {
> +    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
> +
> +    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
> +        return 0;
> +
>      /* Don't restore labels on readoly/shared disks, because
>       * other VMs may still be accessing these
>       * Alternatively we could iterate over all running
> @@ -405,6 +416,9 @@ SELinuxSetSecurityImageLabel(virConnectPtr conn,
>      const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
>      const char *path;
>  
> +    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
> +        return 0;
> +
>      if (!disk->src)
>          return 0;
>  
> @@ -474,8 +488,12 @@ SELinuxSetSecurityHostdevLabel(virConnectPtr conn,
>                                 virDomainHostdevDefPtr dev)
>  
>  {
> +    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
>      int ret = -1;
>  
> +    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
> +        return 0;
> +
>      if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
>          return 0;
>  
> @@ -541,11 +559,16 @@ SELinuxRestoreSecurityUSBLabel(virConnectPtr conn,
>  
>  static int
>  SELinuxRestoreSecurityHostdevLabel(virConnectPtr conn,
> +                                   virDomainObjPtr vm,
>                                     virDomainHostdevDefPtr dev)
>  
>  {
> +    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
>      int ret = -1;
>  
> +    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
> +        return 0;
> +
>      if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
>          return 0;
>  
> @@ -601,25 +624,28 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn,
>  
>      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)
> -                rc = -1;
> -        }
> -        for (i = 0 ; i < vm->def->ndisks ; i++) {
> -            if (SELinuxRestoreSecurityImageLabel(conn, vm,
> -                                                 vm->def->disks[i]) < 0)
> -                rc = -1;
> -        }
> -        VIR_FREE(secdef->model);
> -        VIR_FREE(secdef->label);
> -        context_t con = context_new(secdef->imagelabel);
> -        if (con) {
> -            mcsRemove(context_range_get(con));
> -            context_free(con);
> -        }
> -        VIR_FREE(secdef->imagelabel);
> +    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
> +        return 0;
> +
> +    for (i = 0 ; i < vm->def->nhostdevs ; i++) {
> +        if (SELinuxRestoreSecurityHostdevLabel(conn, vm, vm->def->hostdevs[i]) < 0)
> +            rc = -1;
>      }
> +    for (i = 0 ; i < vm->def->ndisks ; i++) {
> +        if (SELinuxRestoreSecurityImageLabel(conn, vm,
> +                                             vm->def->disks[i]) < 0)
> +            rc = -1;
> +    }
> +    context_t con = context_new(secdef->label);
> +    if (con) {
> +        mcsRemove(context_range_get(con));
> +        context_free(con);
> +    }
> +
> +    VIR_FREE(secdef->model);
> +    VIR_FREE(secdef->label);
> +    VIR_FREE(secdef->imagelabel);
> +
>      return rc;
>  }
>  
> @@ -631,14 +657,23 @@ SELinuxSetSavedStateLabel(virConnectPtr conn,
>  {
>      const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
>  
> +    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
> +        return 0;
> +
>      return SELinuxSetFilecon(conn, savefile, secdef->imagelabel);
>  }
>  
>  
>  static int
>  SELinuxRestoreSavedStateLabel(virConnectPtr conn,
> +                              virDomainObjPtr vm,
>                                const char *savefile)
>  {
> +    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
> +
> +    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
> +        return 0;
> +
>      return SELinuxRestoreSecurityFileLabel(conn, savefile);
>  }
>  
> @@ -666,6 +701,9 @@ SELinuxSetSecurityLabel(virConnectPtr conn,
>      const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
>      int i;
>  
> +    if (vm->def->seclabel.label == NULL)
> +        return 0;
> +
>      if (!STREQ(drv->name, secdef->model)) {
>          virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
>                                 _("security label driver mismatch: "
> @@ -684,7 +722,7 @@ SELinuxSetSecurityLabel(virConnectPtr conn,
>              return -1;
>      }
>  
> -    if (secdef->imagelabel) {
> +    if (secdef->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
>          for (i = 0 ; i < vm->def->ndisks ; i++) {
>              /* XXX fixme - we need to recursively label the entriy tree :-( */
>              if (vm->def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_DIR)

  ACK, also adds some missing check for some driver->securityDriver
entry point. My only concern is dereferencing vm->def->seclabel without
any check for NULL, I would feel better if that extra safety belt was
added,

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/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]