Re: [PATCH 3/4] virt-aa-helper: Purge profile if corrupted
On Thu, Oct 7, 2021 at 10:41 PM Ján Tomko <jtomko(a)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(a)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
>