[PATCH 0/3] Add info used by AppArmor to domstatus XML
This series has two independent changes following from a thread back in November (#692) [1][2]. Broadly speaking I agree that regenerating the apparmor profile from scratch feels fragile. That said, this issue has been on my back burner for a while; it's out of scope for me to take on that (much larger) effort. I'm including the first patch for completeness' sake, as all blockcommit operations fail without it when using the AppArmor driver (#806 [3]). It was rejected in 2017 but is still carried in Ubuntu [4]. Feel free not to pull it - the solution to that issue is separate and not my primary concern. I can send a new version of patch 3 that applies without it. My understanding is that the domstatus XML is only used by libvirt internally (stored in /var/run to persist runtime info over libvirtd restarts). Since this is the case, I haven't included documentation for the new items here; please let me know if I missed where they should be documented. I'm happy to consider this a first draft; feedback is welcome. I've opened a MR to libvirt-tck with test cases that demonstrate the bugs that this fixes [5]. Those tests pass with the series applied. Thanks for your consideration. ~Wesley [1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/QUJIT... [2] https://gitlab.com/libvirt/libvirt/-/issues/692 [3] https://gitlab.com/libvirt/libvirt/-/issues/806 [4] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/3WIDP... [5] https://gitlab.com/libvirt/libvirt-tck/-/merge_requests/73 --- Serge Hallyn (1): virt-aa-helper: Ask for no deny rule for readonly disk elements Wesley Hershberger (2): qemu: Store tapfd path in domstatus XML qemu: Store blockcommit permissions in domstatus XML src/conf/domain_conf.c | 17 +++++++++++++++++ src/conf/domain_conf.h | 1 + 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_command.c | 9 +++++++++ src/qemu/qemu_security.c | 7 +++++++ src/security/security_apparmor.c | 1 + src/security/virt-aa-helper.c | 14 ++++++++------ 10 files changed, 82 insertions(+), 6 deletions(-) --- base-commit: 16804acf14616d7357ad6a336f2ffd6d255a8d63 change-id: 20260105-apparmor-races-d03238ee4d93 Best regards, -- Wesley Hershberger <wesley.hershberger@canonical.com>
From: Serge Hallyn <serge.hallyn@ubuntu.com> Just because a disk element only requests read access doesn't mean there may not be another readwrite request. Using 'R' when creating the apparmor rule will prevent an implicit write-deny rule to be created alongside. This does not mean write is allowed but it would cause a denial message and probably more relevant, allows to add write access later. Resolves: #622 Resolves: #806 Bug-Ubuntu: https://bugs.launchpad.net/bugs/1554031 Bug-Ubuntu: https://bugs.launchpad.net/bugs/1692441 Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> Signed-off-by: Wesley Hershberger <wesley.hershberger@canonical.com> --- src/security/virt-aa-helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index de0a826063..9598b95432 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -835,11 +835,11 @@ add_file_path(virStorageSource *src, if (depth == 0) { if (src->readonly) - ret = vah_add_file(buf, src->path, "rk"); + ret = vah_add_file(buf, src->path, "Rk"); else ret = vah_add_file(buf, src->path, "rwk"); } else { - ret = vah_add_file(buf, src->path, "rk"); + ret = vah_add_file(buf, src->path, "Rk"); } if (ret != 0) -- 2.51.0
Introduce a read-only `tapfd` element for interfaces of type network, bridge, direct, and ethernet, which contains the path in `/dev` to the backing tapfd for that interface. The element is only included when the domain is being formatted for internal consumption (VIR_DOMAIN_DEF_FORMAT_STATUS) and is not accepted in user-provided XML (!VIR_DOMAIN_DEF_PARSE_INACTIVE). 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 | 10 ++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 9 +++++++++ src/security/security_apparmor.c | 1 + src/security/virt-aa-helper.c | 5 +++++ 5 files changed, 26 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 541dad5bdc..bfeed3dc96 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2936,6 +2936,9 @@ virDomainNetDefFree(virDomainNetDef *def) g_free(def->virtio); g_free(def->coalesce); g_free(def->sourceDev); + if (def->tapfdpath) { + g_free(def->tapfdpath); + } virNetDevIPInfoClear(&def->guestIP); virNetDevIPInfoClear(&def->hostIP); @@ -10427,6 +10430,10 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt, return NULL; } + if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) { + def->tapfdpath = virXPathString("string(./tapfd/@path)", ctxt); + } + if (virNetworkPortOptionsParseXML(ctxt, &def->isolatedPort) < 0) return NULL; @@ -25596,6 +25603,9 @@ virDomainNetDefFormat(virBuffer *buf, if (def->mtu) virBufferAsprintf(buf, "<mtu size='%u'/>\n", def->mtu); + if (def->tapfdpath && (flags & VIR_DOMAIN_DEF_FORMAT_STATUS)) + virBufferAsprintf(buf, "<tapfd path='%s'/>\n", def->tapfdpath); + virDomainNetDefCoalesceFormatXML(buf, def->coalesce); virDomainDeviceInfoFormat(buf, &def->info, flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cb35ff06bd..cadbc3e36d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1201,6 +1201,7 @@ struct _virDomainNetDef { char *downscript; char *domain_name; /* backend domain name */ char *ifname; /* interface name on the host (<target dev='x'/>) */ + char *tapfdpath; /* Path for the device's tapfd in /dev on the host */ virTristateBool managed_tap; virNetDevIPInfo hostIP; char *ifname_guest_actual; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 98229d7cf9..96352821d7 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8880,9 +8880,18 @@ qemuBuildInterfaceConnect(virDomainObj *vm, for (i = 0; i < tapfdSize; i++) { g_autofree char *name = g_strdup_printf("tapfd-%s%zu", net->info.alias, i); + g_autofree char *procpath = NULL; int fd = tapfd[i]; /* we want to keep the array intact for security labeling*/ netpriv->tapfds = g_slist_prepend(netpriv->tapfds, qemuFDPassDirectNew(name, &fd)); + + /* Include tapfd path in the domstatus XML */ + procpath = g_strdup_printf("/proc/self/fd/%d", tapfd[i]); + + if (virFileResolveLink(procpath, &net->tapfdpath) < 0) { + /* it's a deleted file, presumably. Ignore? */ + VIR_WARN("could not find path for descriptor %s, skipping", procpath); + } } netpriv->tapfds = g_slist_reverse(netpriv->tapfds); diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 68ac39611f..13dbce67c3 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -156,6 +156,7 @@ load_profile(virSecurityManager *mgr G_GNUC_UNUSED, if (virDomainDefFormatInternal(def, NULL, &buf, VIR_DOMAIN_DEF_FORMAT_SECURE | + VIR_DOMAIN_DEF_FORMAT_STATUS | VIR_DOMAIN_DEF_FORMAT_VOLUME_TRANSLATED) < 0) return -1; diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 9598b95432..7920162cdc 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1174,6 +1174,11 @@ get_files(vahControl * ctl) vhu->type) != 0) return -1; } + + if (net->tapfdpath) { + if (vah_add_file(&buf, net->tapfdpath, "rwk") != 0) + return -1; + } } for (i = 0; i < ctl->def->nmems; i++) { -- 2.51.0
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
participants (1)
-
Wesley Hershberger