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

[Ján Tomko]

On a Wednesday in 2021, Ioanna Alifieraki wrote:
>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'.

I think the remove_profile function is not necessary - you can just
call parserRemove directly in main() and unlink the file there.

But passing the uuid and cmd as arguments instead of ctl would at least
remove the need to replace ctl->cmd temporarily.

>Regarding create_profile 'ctl' is used to check the 'ctl->dryrun' case.
>Again, an alternative could be an extra argument for the dryrun.

Passing dryrun in ctl is ok.

>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.
>

That depends on whether this should be available to the users of
virt-aa-helper, or it's just for internal use when creating the profile.

>>
>> >+                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.

Above, I suggested that creating the profile here does not need to check
for ctl->dryrun, because the remove_profile('P') call right above does
not check for it. That sounds wrong to me now - nothing should be purged
if the dryrun option was specified.

Also, is there a need to call create_profile here? For 'c' it would get
called twice. Adding a 'bool purged' variable and only creating the
profile
   if (ctl->cmd == 'c' || purged)
would get rid of the extra invocation.

Jano

>
>Thanks,
>Jo
>
>> Jano
>>
>> >+        }
>> >+
>> >         if (ctl->append && ctl->newfile) {
>> >             if (vah_add_file(&buf, ctl->newfile, "rwk") != 0)
>> >                 goto cleanup;
>> >--
>> >2.17.1
>> >
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20211021/def4cee9/attachment-0001.sig>