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@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));
+            }
+
             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));
+    }
+
     virObjectUnref(job->disk->mirror);
     job->disk->mirror = NULL;
 }
@@ -1227,7 +1248,7 @@ qemuBlockJobEventProcessConcludedTransition(qemuBlockJobDataPtr job,
             break;
 
         case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT:
-            qemuBlockJobProcessEventFailedActiveCommit(vm, job);
+            qemuBlockJobProcessEventFailedActiveCommit(driver, vm, job);
             break;
 
         case QEMU_BLOCKJOB_TYPE_CREATE:
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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 :/
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 8/30/19 3:42 PM, Peter Krempa wrote:
> 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@redhat.com>
>> ---
>>   src/qemu/qemu_blockjob.c | 25 +++++++++++++++++++++++--
>>   1 file changed, 23 insertions(+), 2 deletions(-)
>>


>> @@ -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.

That is not implemented :-( Our virSecuritySELinuxRestoreImageLabel() 
acts like 'restorecon $path' which is not what we need here. You know 
what? I'll remove the XATTRs in all three cases (at least for the time 
being) as it's safe to do so (the worst thing is that we won't restore 
the original label - but we weren't doing that anyway). For the first 
two (legacy mode) we can't know if somebody else is not using the 
backing chain, and in this blockdev case we could know that but that 
would require bigger fix.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list