[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
On 1/5/26 21:27, Wesley Hershberger via Devel wrote:
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
We like full URL here so that it's clickable even from good old 'git log' ran from a terminal.
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)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> and merged. The rest was already reviewed by Peter. Michal
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
On Mon, Jan 05, 2026 at 14:27:22 -0600, Wesley Hershberger via Devel wrote:
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); + }
'g_free' is a no-op if argument is NULL so no check is needed similarly to all other calls above.
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]);
So this resolves to /proc/PID_OF_VIRTQEMUD_PROCESS/fd/NNN. virtqemud will close the FD as soon as we pass it to qemu and additionally virtqemud might restart where the path stored in the status XML (which is mainly used to re-load after restart) would point to the old PID. Thus this makes no sense to be stored in status XML. You might want to store the path once it's passed to qemu inside the qemu processe's fd list, but I'd argue that qemu ought to have access to everything in there anyways since libvirt actively passed it there.
+ + if (virFileResolveLink(procpath, &net->tapfdpath) < 0) { + /* it's a deleted file, presumably. Ignore? */
'deleted file' ? These werent files originally even when they come from e.g. opening /dev/net/tun.
+ VIR_WARN("could not find path for descriptor %s, skipping", procpath);
Please avoid VIR_WARN.
+ } }
netpriv->tapfds = g_slist_reverse(netpriv->tapfds);
So with other security drivers the one-time setup of seclabels passed to qemu is done in qemuSecuritySetTapFDLabel() in this function. Other dirvers can do it one-time because it applies on the FDs passed to qemu and doesn't need to be changed. If apparmor requires to be able to reference the FD paths at any later time it will need to refer to the FDs under the qemu pid (which is not know at this point since the qemu process did not start) rather than the pid of the process the qemu driver runs in because that one can re-start and also will necessarily close the FDs once qemu starts.
On Thu, Jan 15, 2026 at 3:50 AM Peter Krempa <pkrempa@redhat.com> wrote: Thanks very much for the review!
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]);
So this resolves to /proc/PID_OF_VIRTQEMUD_PROCESS/fd/NNN. virtqemud will close the FD as soon as we pass it to qemu and additionally virtqemud might restart where the path stored in the status XML (which is mainly used to re-load after restart) would point to the old PID.
Maybe I misunderstood, but this code is almost an exact copy from AppArmorSetFDLabel, where it resolves to "/dev/tapXX" (virFileResolveLink ends up in g_canonicalize_path, which must call realpath(3)). XX in that path _should_ be the index from ioctl SIOCGIFINDEX which IIUC isn't specific to the virtqemud process. All that said, it's super gross to use virFileResolveLink here, especially because virNetDevMacVLanTapOpen already has the tapname. I'm wondering if it's feasible to just pass the tapname through from qemuInterfaceDirectConnect and use qemuSecuritySetPathLabel instead of qemuSecuritySetFDLabel for macvtap devices. I have basically no experience with SELinux; to your knowledge would this work for that driver as I've described it?
Thus this makes no sense to be stored in status XML.
Would the above change your position on this? Thanks! ~Wesley
On Mon, Jan 19, 2026 at 11:38:05 -0600, Wesley Hershberger wrote:
On Thu, Jan 15, 2026 at 3:50 AM Peter Krempa <pkrempa@redhat.com> wrote:
Thanks very much for the review!
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]);
So this resolves to /proc/PID_OF_VIRTQEMUD_PROCESS/fd/NNN. virtqemud will close the FD as soon as we pass it to qemu and additionally virtqemud might restart where the path stored in the status XML (which is mainly used to re-load after restart) would point to the old PID.
Maybe I misunderstood, but this code is almost an exact copy from AppArmorSetFDLabel, where it resolves to "/dev/tapXX" (virFileResolveLink ends up in g_canonicalize_path, which must call realpath(3)). XX in that path _should_ be the index from ioctl SIOCGIFINDEX which IIUC isn't specific to the virtqemud process.
Ah so if this resolves to /dev/tap??? then that information may make sense to be stored. But it definitely is not obvious what happens in the code above so you'll need to comment it and explain what happens.
All that said, it's super gross to use virFileResolveLink here, especially because virNetDevMacVLanTapOpen already has the tapname. I'm wondering if it's feasible to just pass the tapname through from qemuInterfaceDirectConnect and use
Feel free to plumb it through; qemuInterfaceDirectConnect has only one place where it's called so that should be easy.
qemuSecuritySetPathLabel instead of qemuSecuritySetFDLabel for macvtap devices. I have basically no experience with SELinux; to your knowledge would this work for that driver as I've described it?
AFAIU you need to label the specific FD here for selinux to work properly. Since it's being passed to qemu we still need the FD. You could introduce an API which takes both FD and path to label and apparmor would label the latter, but you can't avoid the FD wit selinux.
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
On Mon, Jan 05, 2026 at 14:27:23 -0600, Wesley Hershberger via Devel wrote:
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/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;
We internally track the writable state in the block job data (by knowing which images are undergoing a block commit operation). This would just duplicate the data and requiring us to keep it in sync just to be able to smuggle it into the helper process. I currently don't remember if you are getting the whole status XML, with also the qemu driver private data, where you could do the same inference, or just the normal XML with some status bits which are inside the domain XML. Either way I don't like adding another flag which is not the primary source of information. If the full status XML is fed to the helper process you'll need to look in the 'blockjobs' section (see tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml as example) and lookup the images based on job type and nodename. If the whole status XML is not fed to the helper thne I'd suggest to pass another block of private data to the helper process rather than sprinkling them into the runtime definitions and requiring to keep them in sync, so that it's tightly coupled what's required by the helper function and where we provide it from.
On Thu, Jan 15, 2026 at 3:07 AM Peter Krempa <pkrempa@redhat.com> wrote:
On Mon, Jan 05, 2026 at 14:27:23 -0600, Wesley Hershberger via Devel wrote:
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/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;
We internally track the writable state in the block job data (by knowing which images are undergoing a block commit operation). This would just duplicate the data and requiring us to keep it in sync just to be able to smuggle it into the helper process.
I currently don't remember if you are getting the whole status XML, with also the qemu driver private data, where you could do the same inference, or just the normal XML with some status bits which are inside the domain XML.
Either way I don't like adding another flag which is not the primary source of information.
If the full status XML is fed to the helper process you'll need to look in the 'blockjobs' section (see tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml as example) and lookup the images based on job type and nodename.
If the whole status XML is not fed to the helper thne I'd suggest to pass another block of private data to the helper process rather than sprinkling them into the runtime definitions and requiring to keep them in sync, so that it's tightly coupled what's required by the helper function and where we provide it from.
Thanks for the pointer on domstatus->blockjobs. As Michal noted, the domstatus XML with the blockjob data isn't fed into the virt-aa-helper today. That's the solution I'd prefer here, but... - The qemu driver privateData is a field of virDomainObj - The secdriver is only passed a virDomainDef I'm not sure if/how it's possible to go from virDomainDef to virDomainObj. There are the virDomainObjListFindBy* functions, but they require a reference to the domain driver which I don't think we have in the secdriver either. Here's a little additional context on these patches; I should have linked this in the cover letter: https://www.mail-archive.com/devel@lists.libvirt.org/msg13340.html That's why I went with changes to the XML rather than trying to plumb the info through separately. Thanks for your help, I really appreciate it. ~Wesley
On Mon, Jan 19, 2026 at 11:38:43 -0600, Wesley Hershberger wrote:
On Thu, Jan 15, 2026 at 3:07 AM Peter Krempa <pkrempa@redhat.com> wrote:
[...]
If the full status XML is fed to the helper process you'll need to look in the 'blockjobs' section (see tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml as example) and lookup the images based on job type and nodename.
If the whole status XML is not fed to the helper thne I'd suggest to pass another block of private data to the helper process rather than sprinkling them into the runtime definitions and requiring to keep them in sync, so that it's tightly coupled what's required by the helper function and where we provide it from.
Thanks for the pointer on domstatus->blockjobs. As Michal noted, the domstatus XML with the blockjob data isn't fed into the virt-aa-helper today. That's the solution I'd prefer here, but...
- The qemu driver privateData is a field of virDomainObj - The secdriver is only passed a virDomainDef
I'm not sure if/how it's possible to go from virDomainDef to virDomainObj. There are the virDomainObjListFindBy* functions, but they require a reference to the domain driver which I don't think we have in the secdriver either.
Hmm, yeah that's a pitty. I think the only viable solution would be via a callback function and a private data pointer. I wonder how we can address the disconnect between how apparmor works and how selinux/dac work. Specifically the part where qemu driver code needs to be able to reconstruct the state which the other drivers keep in seclabels. Obviously for the disk images you could create an specific XATTR labels noting the current state akin to the selinux label.
Here's a little additional context on these patches; I should have linked this in the cover letter: https://www.mail-archive.com/devel@lists.libvirt.org/msg13340.html That's why I went with changes to the XML rather than trying to plumb the info through separately.
I understand that this feels simpler but this specific field serves no purpose other than duplicate state we store elsewhere which comes with extra cost to keep in sync and that should be avoided. The parser and formatter are complex-enough as they stand currently.
participants (3)
-
Michal Prívozník -
Peter Krempa -
Wesley Hershberger