[libvirt] [PATCH] Fix labelling on QEMU restore images

[Daniel Veillard]

On Thu, Nov 19, 2009 at 12:11:24PM +0000, Daniel P. Berrange wrote:
> Even though QEMU does not directly open the saved image when
> restoring, it must be correctly labelled to allow QEMU to
> read from it because labelling is passed around with open
> file descriptors.
> 
> The labelling should not allow writing to the saved image
> again, only reading.
> 
> * src/qemu/qemu_driver.c: Label the save image when restoring
> * src/security/security_driver.h: Add a virSecurityDomainSetSavedStateLabelRO
>   method for labelling a saved image for restore
> * src/security/security_selinux.c: Implement labelling of RO
>   save images for restore
> ---
>  src/qemu/qemu_driver.c          |   31 ++++++++++++++++++++-----------
>  src/security/security_driver.h  |    5 +++++
>  src/security/security_selinux.c |   11 +++++++++++
>  3 files changed, 36 insertions(+), 11 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index a4a87ac..d500e14 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -3484,7 +3484,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;
> @@ -4036,6 +4036,11 @@ static int qemudDomainRestore(virConnectPtr conn,
>      if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
>          goto cleanup;
>  
> +    if (driver->securityDriver &&
> +        driver->securityDriver->domainSetSavedStateLabelRO &&
> +        driver->securityDriver->domainSetSavedStateLabelRO(conn, vm, path) == -1)
> +        goto endjob;
> +
>      if (header.version == 2) {
>          const char *intermediate_argv[3] = { NULL, "-dc", NULL };
>          const char *prog = qemudSaveCompressionTypeToString(header.compressed);
> @@ -4070,15 +4075,8 @@ static int qemudDomainRestore(virConnectPtr conn,
>          close(intermediatefd);
>      close(fd);
>      fd = -1;
> -    if (ret < 0) {
> -        if (!vm->persistent) {
> -            qemuDomainObjEndJob(vm);
> -            virDomainRemoveInactive(&driver->domains,
> -                                    vm);
> -            vm = NULL;
> -        }
> +    if (ret < 0)
>          goto endjob;
> -    }
>  
>      event = virDomainEventNewFromObj(vm,
>                                       VIR_DOMAIN_EVENT_STARTED,
> @@ -4102,8 +4100,19 @@ static int qemudDomainRestore(virConnectPtr conn,
>      ret = 0;
>  
>  endjob:
> -    if (vm)
> -        qemuDomainObjEndJob(vm);
> +    qemuDomainObjEndJob(vm);
> +    if (driver->securityDriver &&
> +        driver->securityDriver->domainRestoreSavedStateLabel &&
> +        driver->securityDriver->domainRestoreSavedStateLabel(conn, vm, path) == -1)
> +        VIR_WARN("Unable to restore labelling on %s", path);
> +
> +    if (ret < 0) {
> +        if (!vm->persistent) {
> +            virDomainRemoveInactive(&driver->domains,
> +                                    vm);
> +            vm = NULL;
> +        }
> +    }
>  
>  cleanup:
>      virDomainDefFree(def);
> diff --git a/src/security/security_driver.h b/src/security/security_driver.h
> index 5514962..5144976 100644
> --- a/src/security/security_driver.h
> +++ b/src/security/security_driver.h
> @@ -45,7 +45,11 @@ typedef int (*virSecurityDomainSetHostdevLabel) (virConnectPtr conn,
>  typedef int (*virSecurityDomainSetSavedStateLabel) (virConnectPtr conn,
>                                                      virDomainObjPtr vm,
>                                                      const char *savefile);
> +typedef int (*virSecurityDomainSetSavedStateLabelRO) (virConnectPtr conn,
> +                                                      virDomainObjPtr vm,
> +                                                      const char *savefile);
>  typedef int (*virSecurityDomainRestoreSavedStateLabel) (virConnectPtr conn,
> +                                                        virDomainObjPtr vm,
>                                                          const char *savefile);
>  typedef int (*virSecurityDomainGenLabel) (virConnectPtr conn,
>                                            virDomainObjPtr sec);
> @@ -77,6 +81,7 @@ struct _virSecurityDriver {
>      virSecurityDomainRestoreHostdevLabel domainRestoreSecurityHostdevLabel;
>      virSecurityDomainSetHostdevLabel domainSetSecurityHostdevLabel;
>      virSecurityDomainSetSavedStateLabel domainSetSavedStateLabel;
> +    virSecurityDomainSetSavedStateLabelRO domainSetSavedStateLabelRO;
>      virSecurityDomainRestoreSavedStateLabel domainRestoreSavedStateLabel;
>  
>      /*
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index bd838e6..49f6746 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -637,7 +637,17 @@ SELinuxSetSavedStateLabel(virConnectPtr conn,
>  
>  
>  static int
> +SELinuxSetSavedStateLabelRO(virConnectPtr conn,
> +                            virDomainObjPtr vm ATTRIBUTE_UNUSED,
> +                            const char *savefile)
> +{
> +    return SELinuxSetFilecon(conn, savefile, default_content_context);
> +}
> +
> +
> +static int
>  SELinuxRestoreSavedStateLabel(virConnectPtr conn,
> +                              virDomainObjPtr vm ATTRIBUTE_UNUSED,
>                                const char *savefile)
>  {
>      return SELinuxRestoreSecurityFileLabel(conn, savefile);

  ACK

> @@ -714,5 +724,6 @@ virSecurityDriver virSELinuxSecurityDriver = {
>      .domainSetSecurityHostdevLabel = SELinuxSetSecurityHostdevLabel,
>      .domainRestoreSecurityHostdevLabel = SELinuxRestoreSecurityHostdevLabel,
>      .domainSetSavedStateLabel = SELinuxSetSavedStateLabel,
> +    .domainSetSavedStateLabelRO = SELinuxSetSavedStateLabelRO,
>      .domainRestoreSavedStateLabel = SELinuxRestoreSavedStateLabel,
>  };

  maybe one could take the opportunity to convert to the explicit
complete initialization for the security driver too, as we did for
hypervisor ones.

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/