Reorganise create and remove functionality into functions for later reuse in calls from multiple places. Signed-off-by: Ioanna Alifieraki <ioanna-maria.alifieraki(a)canonical.com&gt; --- src/security/virt-aa-helper.c | 55 ++++++++++++++++++++++------------- 1 file changed, 35 insertions(+), 20 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 77b2307594..269c372704 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -262,7 +262,7 @@ update_include_file(const char *include_file, const char *included_files, * Create a profile based on a template */ static int -create_profile(const char *profile, const char *profile_name, +_create_profile(const char *profile, const char *profile_name, const char *profile_files, int virtType) { g_autofree char *template = NULL; @@ -1437,6 +1437,36 @@ vahParseArgv(vahControl * ctl, int argc, char **argv) return 0; } +static int create_profile(vahControl *ctl, char *profile, char *include_file) +{ + + int rc = 0; + char *tmp = g_strdup_printf(" #include <libvirt/%s.files>\n", ctl->uuid); + if (ctl->dryrun) { + vah_info(profile); + vah_info(ctl->uuid); + vah_info(tmp); + } else if ((rc = _create_profile(profile, ctl->uuid, tmp, + ctl->def->virtType)) != 0) { + unlink(include_file); + rc = -1; + } + VIR_FREE(tmp); + + return rc; +} + +static int remove_profile(vahControl *ctl, char *include_file) +{ + int rc = 0; + + if ((rc = parserRemove(ctl->uuid)) != 0) + return rc; + if (ctl->cmd == 'D') + unlink(include_file); + + return rc; +} /* * virt-aa-helper -c -u UUID < file.xml @@ -1489,9 +1519,7 @@ main(int argc, char **argv) if (ctl->cmd == 'a') { rc = parserLoad(ctl->uuid); } else if (ctl->cmd == 'R' || ctl->cmd == 'D') { - rc = parserRemove(ctl->uuid); - if (ctl->cmd == 'D') - unlink(include_file); + rc = remove_profile(ctl, include_file); } else if (ctl->cmd == 'c' || ctl->cmd == 'r') { char *included_files = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; @@ -1538,22 +1566,9 @@ main(int argc, char **argv) /* create the profile from TEMPLATE */ - if (ctl->cmd == 'c') { - char *tmp = NULL; - tmp = g_strdup_printf(" #include <libvirt/%s.files>\n", ctl->uuid); - - if (ctl->dryrun) { - vah_info(profile); - vah_info(ctl->uuid); - vah_info(tmp); - rc = 0; - } else if ((rc = create_profile(profile, ctl->uuid, tmp, - ctl->def->virtType)) != 0) { - vah_error(ctl, 0, _("could not create profile")); - unlink(include_file); - } - VIR_FREE(tmp); - } + if (ctl->cmd == 'c') + if ((rc = create_profile(ctl, profile, include_file)) != 0) + vah_error(ctl, 0, _("could not create profile")); if (rc == 0 && !ctl->dryrun) { if (ctl->cmd == 'c') -- 2.17.1

This commit aims to address the bug reported in [1] and [2]. If the profile is corrupted (0-size) the VM cannot be launched. To overcome this check if the profile exists and if it has 0 size remove it and create it again. [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=890084 [2] https://bugs.launchpad.net/bugs/1927519 Signed-off-by: Ioanna Alifieraki <ioanna-maria.alifieraki(a)canonical.com&gt; --- src/security/virt-aa-helper.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5ec0fb8807..5e13b29053 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1489,6 +1489,7 @@ main(int argc, char **argv) int rc = -1; char *profile = NULL; char *include_file = NULL; + off_t size; if (virGettextInitialize() < 0 || virErrorInitialize() < 0) { @@ -1534,6 +1535,28 @@ main(int argc, char **argv) if (ctl->cmd == 'c' && virFileExists(profile)) vah_error(ctl, 1, _("profile exists")); + /* + * Rare cases can leave corrupted empty files behind breaking + * the guest. An empty file is never correct as virt-aa-helper + * would at least add the basic rules, therefore clean this up + * for a proper refresh. + */ + + if (virFileExists(profile)) { + size = virFileLength(profile, -1); + if (size == 0) { + char temp; + vah_warning(_("Profile of 0 size detected, will attempt to remove and re-create it")); + temp = ctl->cmd; + ctl->cmd = 'P'; + if ((rc = remove_profile(ctl, profile, include_file)) != 0) + vah_error(ctl, 1, _("could not remove corrupted profile")); + ctl->cmd = temp; + if ((rc = create_profile(ctl, profile, include_file)) != 0) + vah_error(ctl, 1, _("could not re-create profile")); + } + } + if (ctl->append && ctl->newfile) { if (vah_add_file(&buf, ctl->newfile, "rwk") != 0) goto cleanup; -- 2.17.1

On Thu, Oct 7, 2021 at 10:41 PM Ján Tomko <jtomko(a)redhat.com&gt; wrote: Thank you very much for the feedback provided. I'd like some clarifications. Do you suggest this for both remove_profile and create_profile helper functions ? Not passing 'ctl' to the helper functions would make it difficult to tell between the different options. For example, not passing ctl to remove_profile we cannot tell if it's called for D,R or P option. I guess we could overcome this by passing an extra 'cmd' argument and not the 'ctl'. Regarding create_profile 'ctl' is used to check the 'ctl->dryrun' case. Again, an alternative could be an extra argument for the dryrun. What do you think? In that case, I question whether patch 2/4 is needed after all. Adding the '-P' option targets (at least for the time being) that specific case. I'm a bit concerned here, as all tests in virt-aa-helper-test are run with the dryrun option set. Thanks, Jo

Create a corrupt profile and expect to be removed after the test is run. Signed-off-by: Ioanna Alifieraki <ioanna-maria.alifieraki(a)canonical.com&gt; --- tests/meson.build | 1 + tests/virt-aa-helper-test | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/tests/meson.build b/tests/meson.build index dfbc2c01e2..991cfc402d 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -40,6 +40,7 @@ tests_env = [ 'LC_ALL=C', 'LIBVIRT_AUTOSTART=0', 'G_DEBUG=fatal-warnings', + &#39;sysconfdir=@0(a)&#39;.format(get_option(&#39;prefix&#39;) / get_option('sysconfdir')), ] if use_expensive_tests diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test index 83f53acef6..135c4968b5 100755 --- a/tests/virt-aa-helper-test +++ b/tests/virt-aa-helper-test @@ -16,6 +16,7 @@ fi output="/dev/null" use_valgrind="" ld_library_path="$abs_top_builddir/src/" +profile_path="$sysconfdir/apparmor.d/libvirt/" if [ ! -z "$1" ] && [ "$1" = "-d" ]; then output="/dev/stdout" shift @@ -399,6 +400,34 @@ testme "0" "shmem doorbell" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/ sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<shmem name='shmem_server'><model type='ivshmem-doorbell'/><server path='/var/lib/libvirt/ivshmem_socket'/></shmem></devices>,g" "$template_xml" > "$test_xml" testme "0" "shmem doorbell serverpath" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/ivshmem_socket\"\s*rw,$" +# For the next test to run apparmor needs to be installed and enabled. +# In some environments (e.g. containers) even though apparmor is +# installed, it is not enabled because securityfs is not mounted. +# In those environments this test cannot run so skip it. +# This test also needs to be run as root. +if [ `eval id -u` = 0 ] && [ -x "$(command -v aa-enabled)" ] && [ `eval aa-enabled` = "Yes" ]; then + sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk2,g" "$template_xml" > "$test_xml" + # Running the tests does not require libvirt to be installed. As a + # result the appropriate directories have not been created. Create them + # now to run the test. + mkdir -p "$profile_path" + # create a corrupted profile + touch "$profile_path/$valid_uuid" + testme "0" "purge" "-r -u $valid_uuid" "$test_xml" + # All the tests are run with the --dry-run option this test is + # never going to fail because the profile is not going to be loaded. + # However, since we touch the profile if it's still here after the test + # it means that something went wrong, so make the test fail. + if [ -f "$profile_path/$valid_uuid" ]; then + echo "FAIL: failed to purge corrupted profile" >$output + echo " '$extra_args $args': " + errors=$(($errors + 1)) + # remove corrupted profile anyways not to interfere with + # subsequent runs of the tests. + rm "$profile_path/$valid_uuid" + fi +fi + testme "0" "help" "-h" echo "" >$output -- 2.17.1

On Thu, Oct 7, 2021 at 7:25 PM Ioanna Alifieraki <ioanna-maria.alifieraki(a)canonical.com&gt; wrote: This is great to be checked before causing a failure, but a question to the libvirt-CI experts, how doable (or not) would it be to get apparmor installed on those distro testbeds that support it? Are there any good pointers one would start to look at adapting those testbeds? -- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd