[libvirt] [PATCH 5/5] qemuDomainDetachDeviceUnlink: Don't unlink files we haven't created

[Cedric Bosdonnat]

On Fri, 2017-04-28 at 13:22 +0200, Michal Privoznik wrote:
> Even though there are several checks before calling this function
> and for some scenarios we don't call it at all (e.g. on disk hot
> unplug), it may be possible to sneak in some weird files (e.g. if
> domain would have RNG with /dev/shm/some_file as its backend). No
> matter how improbable, we shouldn't unlink it as we would be
> unlinking a file from the host which we haven't created in the
> first place.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_domain.c | 86 ++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 76 insertions(+), 10 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 60f8f01..c393d5e 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8395,14 +8395,32 @@ qemuDomainDetachDeviceUnlinkHelper(pid_t pid ATTRIBUTE_UNUSED,
>  static int
>  qemuDomainDetachDeviceUnlink(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>                               virDomainObjPtr vm,
> -                             const char *file)
> +                             const char *file,
> +                             char * const *devMountsPath,
> +                             size_t ndevMountsPath)
>  {
> -    if (virProcessRunInMountNamespace(vm->pid,
> -                                      qemuDomainDetachDeviceUnlinkHelper,
> -                                      (void *)file) < 0)
> -        return -1;
> +    int ret = -1;
> +    size_t i;
>  
> -    return 0;
> +    if (STRPREFIX(file, DEVPREFIX)) {
> +        for (i = 0; i < ndevMountsPath; i++) {
> +            if (STREQ(devMountsPath[i], "/dev"))
> +                continue;
> +            if (STRPREFIX(file, devMountsPath[i]))
> +                break;
> +        }
> +
> +        if (i == ndevMountsPath) {
> +            if (virProcessRunInMountNamespace(vm->pid,
> +                                              qemuDomainDetachDeviceUnlinkHelper,
> +                                              (void *)file) < 0)
> +                goto cleanup;
> +        }
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    return ret;
>  }
>  
>  
> @@ -8521,6 +8539,9 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver,
>                                     virDomainObjPtr vm,
>                                     virDomainHostdevDefPtr hostdev)
>  {
> +    virQEMUDriverConfigPtr cfg = NULL;
> +    char **devMountsPath = NULL;
> +    size_t ndevMountsPath = 0;
>      int ret = -1;
>      char **path = NULL;
>      size_t i, npaths = 0;
> @@ -8532,8 +8553,15 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver,
>                                   &npaths, &path, NULL) < 0)
>          goto cleanup;
>  
> +    cfg = virQEMUDriverGetConfig(driver);
> +    if (qemuDomainGetPreservedMounts(cfg, vm,
> +                                     &devMountsPath, NULL,
> +                                     &ndevMountsPath) < 0)
> +        goto cleanup;
> +
>      for (i = 0; i < npaths; i++) {
> -        if (qemuDomainDetachDeviceUnlink(driver, vm, path[i]) < 0)
> +        if (qemuDomainDetachDeviceUnlink(driver, vm, path[i],
> +                                         devMountsPath, ndevMountsPath) < 0)
>              goto cleanup;
>      }
>  
> @@ -8542,6 +8570,8 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver,
>      for (i = 0; i < npaths; i++)
>          VIR_FREE(path[i]);
>      VIR_FREE(path);
> +    virStringListFreeCount(devMountsPath, ndevMountsPath);
> +    virObjectUnref(cfg);
>      return ret;
>  }
>  
> @@ -8584,6 +8614,9 @@ qemuDomainNamespaceTeardownMemory(virQEMUDriverPtr driver,
>                                    virDomainObjPtr vm,
>                                    virDomainMemoryDefPtr mem)
>  {
> +    virQEMUDriverConfigPtr cfg = NULL;
> +    char **devMountsPath = NULL;
> +    size_t ndevMountsPath = 0;
>      int ret = -1;
>  
>      if (mem->model != VIR_DOMAIN_MEMORY_MODEL_NVDIMM)
> @@ -8592,10 +8625,19 @@ qemuDomainNamespaceTeardownMemory(virQEMUDriverPtr driver,
>      if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
>          return 0;
>  
> -    if (qemuDomainDetachDeviceUnlink(driver, vm, mem->nvdimmPath) < 0)
> +    cfg = virQEMUDriverGetConfig(driver);
> +    if (qemuDomainGetPreservedMounts(cfg, vm,
> +                                     &devMountsPath, NULL,
> +                                     &ndevMountsPath) < 0)
> +        goto cleanup;
> +
> +    if (qemuDomainDetachDeviceUnlink(driver, vm, mem->nvdimmPath,
> +                                     devMountsPath, ndevMountsPath) < 0)
>          goto cleanup;
>      ret = 0;
>   cleanup:
> +    virStringListFreeCount(devMountsPath, ndevMountsPath);
> +    virObjectUnref(cfg);
>      return ret;
>  }
>  
> @@ -8643,6 +8685,9 @@ qemuDomainNamespaceTeardownChardev(virQEMUDriverPtr driver,
>                                     virDomainObjPtr vm,
>                                     virDomainChrDefPtr chr)
>  {
> +    virQEMUDriverConfigPtr cfg = NULL;
> +    char **devMountsPath = NULL;
> +    size_t ndevMountsPath = 0;
>      int ret = -1;
>      const char *path = NULL;
>  
> @@ -8654,11 +8699,20 @@ qemuDomainNamespaceTeardownChardev(virQEMUDriverPtr driver,
>  
>      path = chr->source->data.file.path;
>  
> -    if (qemuDomainDetachDeviceUnlink(driver, vm, path) < 0)
> +    cfg = virQEMUDriverGetConfig(driver);
> +    if (qemuDomainGetPreservedMounts(cfg, vm,
> +                                     &devMountsPath, NULL,
> +                                     &ndevMountsPath) < 0)
> +        goto cleanup;
> +
> +    if (qemuDomainDetachDeviceUnlink(driver, vm, path,
> +                                     devMountsPath, ndevMountsPath) < 0)
>          goto cleanup;
>  
>      ret = 0;
>   cleanup:
> +    virStringListFreeCount(devMountsPath, ndevMountsPath);
> +    virObjectUnref(cfg);
>      return ret;
>  }
>  
> @@ -8712,6 +8766,9 @@ qemuDomainNamespaceTeardownRNG(virQEMUDriverPtr driver,
>                                 virDomainObjPtr vm,
>                                 virDomainRNGDefPtr rng)
>  {
> +    virQEMUDriverConfigPtr cfg = NULL;
> +    char **devMountsPath = NULL;
> +    size_t ndevMountsPath = 0;
>      int ret = -1;
>      const char *path = NULL;
>  
> @@ -8729,11 +8786,20 @@ qemuDomainNamespaceTeardownRNG(virQEMUDriverPtr driver,
>          goto cleanup;
>      }
>  
> -    if (qemuDomainDetachDeviceUnlink(driver, vm, path) < 0)
> +    cfg = virQEMUDriverGetConfig(driver);
> +    if (qemuDomainGetPreservedMounts(cfg, vm,
> +                                     &devMountsPath, NULL,
> +                                     &ndevMountsPath) < 0)
> +        goto cleanup;
> +
> +    if (qemuDomainDetachDeviceUnlink(driver, vm, path,
> +                                     devMountsPath, ndevMountsPath) < 0)
>          goto cleanup;
>  
>      ret = 0;
>   cleanup:
> +    virStringListFreeCount(devMountsPath, ndevMountsPath);
> +    virObjectUnref(cfg);
>      return ret;
>  }
>  
ACK

--
Cedric