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@xxxxxxxxxx> > --- > 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 :/
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
