Adds a read-only attribute allowWrite to a disk's backingStore source element for the duration of a block commit operation, which records that write access is needed for that backingStore. This is used by the AppArmor security driver when re-generating profiles. Partial-Resolves: #692 Bug-Ubuntu: https://bugs.launchpad.net/bugs/2126574 Signed-off-by: Wesley Hershberger <wesley.hershberger@canonical.com> --- src/conf/domain_conf.c | 7 +++++++ src/conf/storage_source_conf.c | 2 ++ src/conf/storage_source_conf.h | 3 +++ src/qemu/qemu_block.c | 26 ++++++++++++++++++++++++++ src/qemu/qemu_blockjob.c | 8 ++++++++ src/qemu/qemu_security.c | 7 +++++++ src/security/virt-aa-helper.c | 7 ++----- 7 files changed, 55 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bfeed3dc96..02ff45626e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7842,6 +7842,10 @@ virDomainStorageSourceParse(xmlNodePtr node, return -1; } + if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && + g_strcmp0(virXMLPropString(node, "allowWrite"), "yes") == 0) + src->secAllowWrite = true; + if ((tmp = virXPathNode("./auth", ctxt)) && !(src->auth = virStorageAuthDefParse(tmp, ctxt))) return -1; @@ -23753,6 +23757,9 @@ virDomainDiskSourceFormat(virBuffer *buf, if (attrIndex && src->id != 0) virBufferAsprintf(&attrBuf, " index='%u'", src->id); + if ((flags & VIR_DOMAIN_DEF_FORMAT_STATUS) && src->secAllowWrite) + virBufferAddLit(&attrBuf, " allowWrite='yes'"); + if (virDomainDiskDataStoreFormat(&childBuf, src, xmlopt, flags) < 0) return -1; diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c index 087de1eaf2..4550a25500 100644 --- a/src/conf/storage_source_conf.c +++ b/src/conf/storage_source_conf.c @@ -911,6 +911,8 @@ virStorageSourceCopy(const virStorageSource *src, def->nfs_uid = src->nfs_uid; def->nfs_gid = src->nfs_gid; + def->secAllowWrite = src->secAllowWrite; + /* 'ioerror_timestamp' and 'ioerror_message' are deliberately not copied */ return g_steal_pointer(&def); diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h index fc868b31af..0a54ff8c9c 100644 --- a/src/conf/storage_source_conf.h +++ b/src/conf/storage_source_conf.h @@ -449,6 +449,9 @@ struct _virStorageSource { * to do this decision. */ bool seclabelSkipRemember; + /* Temporary write access to this source is required (currently only for + * QEMU blockcommit) */ + bool secAllowWrite; /* Last instance of hypervisor originated I/O error message and timestamp. * The error stored here is not designed to be stable. The message diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index a7062d3e96..3aed5d5a7f 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3763,6 +3763,20 @@ qemuBlockCommit(virDomainObj *vm, * operation succeeds, but doing that requires tracking the * operation in XML across libvirtd restarts. */ clean_access = true; + + /* Ensure needed perms are present in domstatus XML; this prevents races + * in the AppArmor secdriver */ + baseSource->secAllowWrite = true; + if (baseSource->dataFileStore) + baseSource->dataFileStore->secAllowWrite = true; + if (top_parent && top_parent != disk->src) { + top_parent->secAllowWrite = true; + if (top_parent->dataFileStore) + top_parent->dataFileStore->secAllowWrite = true; + } + + qemuDomainSaveStatus(vm); + if (qemuDomainStorageSourceAccessAllow(driver, vm, baseSource, false, false, false) < 0) goto cleanup; @@ -3838,6 +3852,18 @@ qemuBlockCommit(virDomainObj *vm, virErrorPtr orig_err; virErrorPreserveLast(&orig_err); + /* Revert changes to domstatus XML */ + baseSource->secAllowWrite = false; + if (baseSource->dataFileStore) + baseSource->dataFileStore->secAllowWrite = false; + if (top_parent && top_parent != disk->src) { + top_parent->secAllowWrite = false; + if (top_parent->dataFileStore) + top_parent->dataFileStore->secAllowWrite = false; + } + + qemuDomainSaveStatus(vm); + /* Revert access to read-only, if possible. */ if (baseSource->dataFileStore) { qemuDomainStorageSourceAccessAllow(driver, vm, baseSource->dataFileStore, diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index b54a5b3811..97a6afe7e3 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1065,20 +1065,24 @@ qemuBlockJobProcessEventCompletedCommit(virQEMUDriver *driver, /* revert access to images */ if (job->data.commit.base->dataFileStore) { + job->data.commit.base->dataFileStore->secAllowWrite = false; qemuDomainStorageSourceAccessAllow(driver, vm, job->data.commit.base->dataFileStore, true, false, false); qemuBlockReopenReadOnly(vm, job->data.commit.base->dataFileStore, asyncJob); } + job->data.commit.base->secAllowWrite = false; qemuDomainStorageSourceAccessAllow(driver, vm, job->data.commit.base, true, false, false); if (job->data.commit.topparent != job->disk->src) { if (job->data.commit.topparent->dataFileStore) { + job->data.commit.topparent->dataFileStore->secAllowWrite = false; qemuDomainStorageSourceAccessAllow(driver, vm, job->data.commit.topparent->dataFileStore, true, false, false); qemuBlockReopenReadWrite(vm, job->data.commit.topparent->dataFileStore, asyncJob); } + job->data.commit.topparent->secAllowWrite = false; qemuDomainStorageSourceAccessAllow(driver, vm, job->data.commit.topparent, true, false, true); } @@ -1177,6 +1181,10 @@ qemuBlockJobProcessEventCompletedActiveCommit(virQEMUDriver *driver, baseparent->backingStore = NULL; job->disk->src = job->data.commit.base; job->disk->src->readonly = job->data.commit.top->readonly; + job->disk->src->secAllowWrite = false; + if (job->disk->src->dataFileStore) { + job->disk->src->dataFileStore->secAllowWrite = false; + } if (qemuBlockJobProcessEventCompletedCommitBitmaps(vm, job, asyncJob) < 0) return; diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 6bb0f9170d..603feba87e 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -201,6 +201,13 @@ qemuSecurityMoveImageMetadata(virQEMUDriver *driver, if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; + if (dst) { + dst->secAllowWrite = src->secAllowWrite; + if (dst->backingStore && src->backingStore) { + dst->backingStore->secAllowWrite = src->backingStore->secAllowWrite; + } + } + return virSecurityManagerMoveImageMetadata(driver->securityManager, cfg->sharedFilesystems, pid, src, dst); diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 7920162cdc..fc2d4d9d41 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -833,11 +833,8 @@ add_file_path(virStorageSource *src, if (!src->path || !virStorageSourceIsLocalStorage(src)) return 0; - if (depth == 0) { - if (src->readonly) - ret = vah_add_file(buf, src->path, "Rk"); - else - ret = vah_add_file(buf, src->path, "rwk"); + if ((depth == 0 && !src->readonly) || src->secAllowWrite) { + ret = vah_add_file(buf, src->path, "rwk"); } else { ret = vah_add_file(buf, src->path, "Rk"); } -- 2.51.0