[PATCH 3/4] virt-aa-helper: Purge profile if corrupted

[Ioanna Alifieraki]

On Thu, Oct 7, 2021 at 10:41 PM Ján Tomko <jtomko at redhat.com> wrote:
>
> On a Thursday in 2021, Ioanna Alifieraki wrote:
> >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 at canonical.com>
> >---
> > 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;
>

Thank you very much for the feedback provided.
I'd like some clarifications.

> Please do not pass 'ctl' to the helper functions. It makes it more
> diffuclt to see what's going on.

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?

>
> >+                ctl->cmd = 'P';
> >+                if ((rc = remove_profile(ctl, profile, include_file)) != 0)
> >+                    vah_error(ctl, 1, _("could not remove corrupted profile"));
>
> Easier to read as:
>      if (parserRemove(ctl->uuid) < 0)
>          goto cleanup;
>      unlink(profile);

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.

>
> >+                ctl->cmd = temp;
> >+                if ((rc = create_profile(ctl, profile, include_file)) != 0)
> >+                    vah_error(ctl, 1, _("could not re-create profile"));
> >+            }
>
> Since we did not honour ctl->dryrun in the previous step,
> it should be safe to call create_profile directly, without the helper
> introduced in 1/4.
>

I'm a bit concerned here, as all tests in virt-aa-helper-test are run with the
dryrun option set.

Thanks,
Jo

> Jano
>
> >+        }
> >+
> >         if (ctl->append && ctl->newfile) {
> >             if (vah_add_file(&buf, ctl->newfile, "rwk") != 0)
> >                 goto cleanup;
> >--
> >2.17.1
> >