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