[libvirt] [PATCH for 5.7.0 3/3] qemu_blockjob: Restore seclabels more frequently on job events

[Peter Krempa]

On Fri, Aug 30, 2019 at 15:19:08 +0200, Michal Privoznik wrote:
> If a block job reaches failed/cancelled state, or is completed
> without pivot then qemu no longer uses the mirror image. Since
> we've set its seclabels we must restore them back to avoid
> leaking perms/XATTRs.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1741456
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_blockjob.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> index 80302fb139..8411d8e223 100644
> --- a/src/qemu/qemu_blockjob.c
> +++ b/src/qemu/qemu_blockjob.c
> @@ -656,6 +656,13 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver,
>      } else {
>          if (disk->mirror) {
>              virDomainLockImageDetach(driver->lockManager, vm, disk->mirror);
> +
> +            /* QEMU no longer uses the image, so we can restore its label. */
> +            if (qemuSecurityRestoreImageLabel(driver, vm, disk->mirror, true) < 0) {
> +                VIR_WARN("Unable to restore security labels on vm %s disk %s",
> +                         vm->def->name, NULLSTR(disk->mirror->path));
> +            }
> +

So unfortunately this is true only for the block copy operation. Active
block commit will still continue using the image in read-only mode.

Also I'm afraid I remember why this code does not have any security
labeling code. The mirror may in fact contain parts of the original
backing chain (if the --reuse-external flag was used) and in that case
we'd not be able to tell which ones we want to remove labelling from, so
the only "safe" operation is to remove labels/locks from the top image
only.

>              virObjectUnref(disk->mirror);
>          }
>      }
> @@ -725,6 +732,13 @@ qemuBlockJobEventProcessLegacy(virQEMUDriverPtr driver,
>      case VIR_DOMAIN_BLOCK_JOB_CANCELED:
>          if (disk->mirror) {
>              virDomainLockImageDetach(driver->lockManager, vm, disk->mirror);
> +
> +            /* QEMU no longer uses the image, so we can restore its label. */
> +            if (qemuSecurityRestoreImageLabel(driver, vm, disk->mirror, true) < 0) {
> +                VIR_WARN("Unable to restore security labels on vm %s disk %s",
> +                         vm->def->name, NULLSTR(disk->mirror->path));
> +            }
> +
>              virObjectUnref(disk->mirror);
>              disk->mirror = NULL;
>          }
> @@ -1124,7 +1138,8 @@ qemuBlockJobProcessEventConcludedCopyAbort(virQEMUDriverPtr driver,
>  
>  
>  static void
> -qemuBlockJobProcessEventFailedActiveCommit(virDomainObjPtr vm,
> +qemuBlockJobProcessEventFailedActiveCommit(virQEMUDriverPtr driver,
> +                                           virDomainObjPtr vm,
>                                             qemuBlockJobDataPtr job)
>  {
>      VIR_DEBUG("active commit job '%s' on VM '%s' failed", job->name, vm->def->name);
> @@ -1132,6 +1147,12 @@ qemuBlockJobProcessEventFailedActiveCommit(virDomainObjPtr vm,
>      if (!job->disk)
>          return;
>  
> +    /* QEMU no longer uses the image, so we can restore its label. */
> +    if (qemuSecurityRestoreImageLabel(driver, vm, job->disk->mirror, true) < 0) {
> +        VIR_WARN("Unable to restore security labels on vm %s disk %s",
> +                 vm->def->name, NULLSTR(job->disk->mirror->path));

So, here this must return the security labels to readonly state rather
than removing it completely as this is still used.

I'm sorry these points didn't occur to me when we were discussing this
code :/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190830/ca8d8111/attachment-0001.sig>