[libvirt] [PATCH 0/7] security: apparmor: prep for qcow2 data_file
This series does some preparation cleanup and refactoring to simplify adding qcow2 data_file support to the apparmor driver. More info on the qcow2 feature and libvirt work here: https://www.redhat.com/archives/libvir-list/2019-October/msg00303.html Cole Robinson (7): conf: Move -virDomainDiskDefForeachPath to virt-aa-helper security: apparmor: Remove unused ignoreOpenFailure security: apparmor: Drop disk_foreach_iterator security: apparmor: Pass virStorageSource to add_file_path security: apparmor: Push virStorageSource checks to add_file_path security: apparmor: Use only virStorageSource for disk paths security: apparmor: Make storage_source_add_files recursively callable src/conf/domain_conf.c | 42 ---------------------------------- src/conf/domain_conf.h | 10 -------- src/libvirt_private.syms | 1 - src/security/virt-aa-helper.c | 43 ++++++++++++++++++++++++----------- 4 files changed, 30 insertions(+), 66 deletions(-) -- 2.23.0
It is the only user. Rename it to match the local style Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 42 ----------------------------- src/conf/domain_conf.h | 10 ------- src/libvirt_private.syms | 1 - src/security/virt-aa-helper.c | 50 ++++++++++++++++++++++++++++++++++- 4 files changed, 49 insertions(+), 54 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a53cd6a725..5fe03ea866 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -29486,48 +29486,6 @@ virDomainUSBDeviceDefForeach(virDomainDefPtr def, } -/* Call iter(disk, name, depth, opaque) for each element of disk and - * its backing chain in the pre-populated disk->src.backingStore. - * ignoreOpenFailure determines whether to warn about a chain that - * mentions a backing file without also having metadata on that - * file. */ -int -virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, - bool ignoreOpenFailure, - virDomainDiskDefPathIterator iter, - void *opaque) -{ - size_t depth = 0; - virStorageSourcePtr tmp; - VIR_AUTOFREE(char *) brokenRaw = NULL; - - if (!ignoreOpenFailure) { - if (virStorageFileChainGetBroken(disk->src, &brokenRaw) < 0) - return -1; - - if (brokenRaw) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unable to visit backing chain file %s"), - brokenRaw); - return -1; - } - } - - for (tmp = disk->src; virStorageSourceIsBacking(tmp); tmp = tmp->backingStore) { - /* execute the callback only for local storage */ - if (virStorageSourceIsLocalStorage(tmp) && - tmp->path) { - if (iter(disk, tmp->path, depth, opaque) < 0) - return -1; - } - - depth++; - } - - return 0; -} - - /* Copy src into a new definition; with the quality of the copy * depending on the migratable flag (false for transitions between * persistent and active, true for transitions across save files or diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 2884af49d8..653dcaf2bc 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3327,11 +3327,6 @@ int virDomainChrDefForeach(virDomainDefPtr def, virDomainChrDefIterator iter, void *opaque); -typedef int (*virDomainDiskDefPathIterator)(virDomainDiskDefPtr disk, - const char *path, - size_t depth, - void *opaque); - typedef int (*virDomainUSBDeviceDefIterator)(virDomainDeviceInfoPtr info, void *opaque); int virDomainUSBDeviceDefForeach(virDomainDefPtr def, @@ -3339,11 +3334,6 @@ int virDomainUSBDeviceDefForeach(virDomainDefPtr def, void *opaque, bool skipHubs); -int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, - bool ignoreOpenFailure, - virDomainDiskDefPathIterator iter, - void *opaque); - void virDomainObjSetState(virDomainObjPtr obj, virDomainState state, int reason) ATTRIBUTE_NONNULL(1); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c818bc807a..5949cba08d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -334,7 +334,6 @@ virDomainDiskCacheTypeFromString; virDomainDiskCacheTypeToString; virDomainDiskDefAssignAddress; virDomainDiskDefCheckDuplicateInfo; -virDomainDiskDefForeachPath; virDomainDiskDefFree; virDomainDiskDefNew; virDomainDiskDefParse; diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5853ad985f..6e358ff5b6 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -934,6 +934,54 @@ add_file_path(virDomainDiskDefPtr disk, return ret; } + +typedef int (*disk_foreach_iterator)(virDomainDiskDefPtr disk, + const char *path, + size_t depth, + void *opaque); + + +/* Call iter(disk, name, depth, opaque) for each element of disk and + * its backing chain in the pre-populated disk->src.backingStore. + * ignoreOpenFailure determines whether to warn about a chain that + * mentions a backing file without also having metadata on that + * file. */ +static int +disk_foreach_path(virDomainDiskDefPtr disk, + bool ignoreOpenFailure, + disk_foreach_iterator iter, + void *opaque) +{ + size_t depth = 0; + virStorageSourcePtr tmp; + VIR_AUTOFREE(char *) brokenRaw = NULL; + + if (!ignoreOpenFailure) { + if (virStorageFileChainGetBroken(disk->src, &brokenRaw) < 0) + return -1; + + if (brokenRaw) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unable to visit backing chain file %s"), + brokenRaw); + return -1; + } + } + + for (tmp = disk->src; virStorageSourceIsBacking(tmp); tmp = tmp->backingStore) { + /* execute the callback only for local storage */ + if (virStorageSourceIsLocalStorage(tmp) && + tmp->path) { + if (iter(disk, tmp->path, depth, opaque) < 0) + return -1; + } + + depth++; + } + + return 0; +} + static int get_files(vahControl * ctl) { @@ -977,7 +1025,7 @@ get_files(vahControl * ctl) * be passing ignoreOpenFailure = false and handle open errors more * careful than just ignoring them. */ - if (virDomainDiskDefForeachPath(disk, true, add_file_path, &buf) < 0) + if (disk_foreach_path(disk, true, add_file_path, &buf) < 0) goto cleanup; } -- 2.23.0
On 10/8/19 6:22 PM, Cole Robinson wrote:
It is the only user. Rename it to match the local style
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 42 ----------------------------- src/conf/domain_conf.h | 10 ------- src/libvirt_private.syms | 1 - src/security/virt-aa-helper.c | 50 ++++++++++++++++++++++++++++++++++- 4 files changed, 49 insertions(+), 54 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5853ad985f..6e358ff5b6 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -934,6 +934,54 @@ add_file_path(virDomainDiskDefPtr disk, return ret; }
+ +typedef int (*disk_foreach_iterator)(virDomainDiskDefPtr disk, + const char *path, + size_t depth, + void *opaque); + + +/* Call iter(disk, name, depth, opaque) for each element of disk and + * its backing chain in the pre-populated disk->src.backingStore. + * ignoreOpenFailure determines whether to warn about a chain that + * mentions a backing file without also having metadata on that + * file. */ +static int +disk_foreach_path(virDomainDiskDefPtr disk, + bool ignoreOpenFailure, + disk_foreach_iterator iter, + void *opaque) +{ + size_t depth = 0; + virStorageSourcePtr tmp; + VIR_AUTOFREE(char *) brokenRaw = NULL; + + if (!ignoreOpenFailure) { + if (virStorageFileChainGetBroken(disk->src, &brokenRaw) < 0) + return -1; + + if (brokenRaw) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unable to visit backing chain file %s"), + brokenRaw); + return -1; + } + } + + for (tmp = disk->src; virStorageSourceIsBacking(tmp); tmp = tmp->backingStore) { + /* execute the callback only for local storage */ + if (virStorageSourceIsLocalStorage(tmp) && + tmp->path) { + if (iter(disk, tmp->path, depth, opaque) < 0) + return -1; + } + + depth++; + } + + return 0; +} + static int get_files(vahControl * ctl) { @@ -977,7 +1025,7 @@ get_files(vahControl * ctl) * be passing ignoreOpenFailure = false and handle open errors more * careful than just ignoring them.
There's one occurrence of virDomainDiskDefForeachPath in this comment ^^^
*/ - if (virDomainDiskDefForeachPath(disk, true, add_file_path, &buf) < 0) + if (disk_foreach_path(disk, true, add_file_path, &buf) < 0) goto cleanup; }
Michal
true is always passed here, so delete the unused code path and adjust the associated comment Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/virt-aa-helper.c | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 6e358ff5b6..511443dd3e 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -943,30 +943,14 @@ typedef int (*disk_foreach_iterator)(virDomainDiskDefPtr disk, /* Call iter(disk, name, depth, opaque) for each element of disk and * its backing chain in the pre-populated disk->src.backingStore. - * ignoreOpenFailure determines whether to warn about a chain that - * mentions a backing file without also having metadata on that - * file. */ + */ static int disk_foreach_path(virDomainDiskDefPtr disk, - bool ignoreOpenFailure, disk_foreach_iterator iter, void *opaque) { size_t depth = 0; virStorageSourcePtr tmp; - VIR_AUTOFREE(char *) brokenRaw = NULL; - - if (!ignoreOpenFailure) { - if (virStorageFileChainGetBroken(disk->src, &brokenRaw) < 0) - return -1; - - if (brokenRaw) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unable to visit backing chain file %s"), - brokenRaw); - return -1; - } - } for (tmp = disk->src; virStorageSourceIsBacking(tmp); tmp = tmp->backingStore) { /* execute the callback only for local storage */ @@ -1020,12 +1004,9 @@ get_files(vahControl * ctl) if (!virStorageSourceHasBacking(disk->src)) virStorageFileGetMetadata(disk->src, -1, -1, false); - /* XXX passing ignoreOpenFailure = true to get back to the behavior - * from before using virDomainDiskDefForeachPath. actually we should - * be passing ignoreOpenFailure = false and handle open errors more - * careful than just ignoring them. + /* XXX should handle open errors more careful than just ignoring them. */ - if (disk_foreach_path(disk, true, add_file_path, &buf) < 0) + if (disk_foreach_path(disk, add_file_path, &buf) < 0) goto cleanup; } -- 2.23.0
There's only one caller, so open code the file_add_path behavior Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/virt-aa-helper.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 511443dd3e..7148e3c760 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -914,9 +914,8 @@ static int add_file_path(virDomainDiskDefPtr disk, const char *path, size_t depth, - void *opaque) + virBufferPtr buf) { - virBufferPtr buf = opaque; int ret; if (depth == 0) { @@ -935,19 +934,9 @@ add_file_path(virDomainDiskDefPtr disk, } -typedef int (*disk_foreach_iterator)(virDomainDiskDefPtr disk, - const char *path, - size_t depth, - void *opaque); - - -/* Call iter(disk, name, depth, opaque) for each element of disk and - * its backing chain in the pre-populated disk->src.backingStore. - */ static int -disk_foreach_path(virDomainDiskDefPtr disk, - disk_foreach_iterator iter, - void *opaque) +disk_add_files(virDomainDiskDefPtr disk, + virBufferPtr buf) { size_t depth = 0; virStorageSourcePtr tmp; @@ -956,7 +945,7 @@ disk_foreach_path(virDomainDiskDefPtr disk, /* execute the callback only for local storage */ if (virStorageSourceIsLocalStorage(tmp) && tmp->path) { - if (iter(disk, tmp->path, depth, opaque) < 0) + if (add_file_path(disk, tmp->path, depth, buf) < 0) return -1; } @@ -1006,7 +995,7 @@ get_files(vahControl * ctl) /* XXX should handle open errors more careful than just ignoring them. */ - if (disk_foreach_path(disk, add_file_path, &buf) < 0) + if (disk_add_files(disk, &buf) < 0) goto cleanup; } -- 2.23.0
The virStorageSource must have everything it needs Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/virt-aa-helper.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 7148e3c760..9f39eb2e2b 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -911,20 +911,19 @@ file_iterate_pci_cb(virPCIDevicePtr dev ATTRIBUTE_UNUSED, } static int -add_file_path(virDomainDiskDefPtr disk, - const char *path, +add_file_path(virStorageSourcePtr src, size_t depth, virBufferPtr buf) { int ret; if (depth == 0) { - if (disk->src->readonly) - ret = vah_add_file(buf, path, "rk"); + if (src->readonly) + ret = vah_add_file(buf, src->path, "rk"); else - ret = vah_add_file(buf, path, "rwk"); + ret = vah_add_file(buf, src->path, "rwk"); } else { - ret = vah_add_file(buf, path, "rk"); + ret = vah_add_file(buf, src->path, "rk"); } if (ret != 0) @@ -945,7 +944,7 @@ disk_add_files(virDomainDiskDefPtr disk, /* execute the callback only for local storage */ if (virStorageSourceIsLocalStorage(tmp) && tmp->path) { - if (add_file_path(disk, tmp->path, depth, buf) < 0) + if (add_file_path(tmp, depth, buf) < 0) return -1; } -- 2.23.0
This mirrors the code layout in security_selinux.c. It will also make it easier to share the checks for qcow2 external data_file support eventually Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/virt-aa-helper.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 9f39eb2e2b..20281c38b7 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -917,6 +917,10 @@ add_file_path(virStorageSourcePtr src, { int ret; + /* execute the callback only for local storage */ + if (!src->path || !virStorageSourceIsLocalStorage(src)) + return 0; + if (depth == 0) { if (src->readonly) ret = vah_add_file(buf, src->path, "rk"); @@ -941,12 +945,8 @@ disk_add_files(virDomainDiskDefPtr disk, virStorageSourcePtr tmp; for (tmp = disk->src; virStorageSourceIsBacking(tmp); tmp = tmp->backingStore) { - /* execute the callback only for local storage */ - if (virStorageSourceIsLocalStorage(tmp) && - tmp->path) { - if (add_file_path(tmp, depth, buf) < 0) - return -1; - } + if (add_file_path(tmp, depth, buf) < 0) + return -1; depth++; } -- 2.23.0
This is closer to what security_selinux.c does, and will help add support for qcow2 external data_files Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/virt-aa-helper.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 20281c38b7..b675572144 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -938,13 +938,13 @@ add_file_path(virStorageSourcePtr src, static int -disk_add_files(virDomainDiskDefPtr disk, - virBufferPtr buf) +storage_source_add_files(virStorageSourcePtr src, + virBufferPtr buf) { size_t depth = 0; virStorageSourcePtr tmp; - for (tmp = disk->src; virStorageSourceIsBacking(tmp); tmp = tmp->backingStore) { + for (tmp = src; virStorageSourceIsBacking(tmp); tmp = tmp->backingStore) { if (add_file_path(tmp, depth, buf) < 0) return -1; @@ -994,7 +994,7 @@ get_files(vahControl * ctl) /* XXX should handle open errors more careful than just ignoring them. */ - if (disk_add_files(disk, &buf) < 0) + if (storage_source_add_files(disk->src, &buf) < 0) goto cleanup; } -- 2.23.0
This will simplify adding support for qcow2 external data_file Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/virt-aa-helper.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index b675572144..d9f6b5638b 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -939,9 +939,9 @@ add_file_path(virStorageSourcePtr src, static int storage_source_add_files(virStorageSourcePtr src, - virBufferPtr buf) + virBufferPtr buf, + size_t depth) { - size_t depth = 0; virStorageSourcePtr tmp; for (tmp = src; virStorageSourceIsBacking(tmp); tmp = tmp->backingStore) { @@ -994,7 +994,7 @@ get_files(vahControl * ctl) /* XXX should handle open errors more careful than just ignoring them. */ - if (storage_source_add_files(disk->src, &buf) < 0) + if (storage_source_add_files(disk->src, &buf, 0) < 0) goto cleanup; } -- 2.23.0
On 10/8/19 12:22 PM, Cole Robinson wrote:
This series does some preparation cleanup and refactoring to simplify adding qcow2 data_file support to the apparmor driver. More info on the qcow2 feature and libvirt work here: https://www.redhat.com/archives/libvir-list/2019-October/msg00303.html
Should have mentioned here: I found apparmor libs/devel packages for fedora, so this is compile tested but not runtime tested. Help with that appreciated. virt-aa-helper-test doesn't seem to regress, but it is failing for me on master before these patches: ./virt-aa-helper-test ls: cannot access '/boot/initrd*': No such file or directory Skipping /boot/initrd* tests. Could not find /boot/initrd* FAIL: exited with '1' OVMF (new path): '--dryrun -r -u libvirt-00000000-0000-0000-0000-0123456789ab': FAIL: exited with '1' AAVMF: '--dryrun -r -u libvirt-00000000-0000-0000-0000-0123456789ab': FAIL: exited with '1' AAVMF32: '--dryrun -r -u libvirt-00000000-0000-0000-0000-0123456789ab': - Cole
On 10/8/19 6:22 PM, Cole Robinson wrote:
This series does some preparation cleanup and refactoring to simplify adding qcow2 data_file support to the apparmor driver. More info on the qcow2 feature and libvirt work here: https://www.redhat.com/archives/libvir-list/2019-October/msg00303.html
Cole Robinson (7): conf: Move -virDomainDiskDefForeachPath to virt-aa-helper security: apparmor: Remove unused ignoreOpenFailure security: apparmor: Drop disk_foreach_iterator security: apparmor: Pass virStorageSource to add_file_path security: apparmor: Push virStorageSource checks to add_file_path security: apparmor: Use only virStorageSource for disk paths security: apparmor: Make storage_source_add_files recursively callable
src/conf/domain_conf.c | 42 ---------------------------------- src/conf/domain_conf.h | 10 -------- src/libvirt_private.syms | 1 - src/security/virt-aa-helper.c | 43 ++++++++++++++++++++++++----------- 4 files changed, 30 insertions(+), 66 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Cole Robinson -
Michal Privoznik