On Thu, Oct 7, 2021 at 10:41 PM Ján Tomko <jtomko@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@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