On 8/19/25 20:46, Hector CAO wrote:
> From: Hector Cao <hector....@canonical.com>
>
> when a device is dynamically attached to a VM, and it needs a special
> system access for apparmor, libvirt calls virt-aa-helper (with argument -F)
> to append a new rule to the apparmor profile of the VM. virt-aa-helper does
> not check for duplicate and blindly appends the rule to the profile. since
> there is no rule removal when a device is detached, this can make the profile
> grow in size if a big number of attach/detach operations are done and the
> profile might hit the size limit and futur attach operations might dysfunction
> because no rule can be added into the apparmor profile.
>
> this patch tries to mitigate this issue by doing a duplicate check
> when rules are appended into the profile. this fix does not guarantee
> the absence of duplicates but should be enough to prevent the profile
> to grow significantly in size and reach its size limit.
>
> Signed-off-by: Hector CAO <hector....@canonical.com>
> ---
> src/security/virt-aa-helper.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index b662d971cb..a003c6dd9e 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -208,9 +208,20 @@ update_include_file(const char *include_file, const char
> *included_files,
> return -1;
> }
>
> - if (append && virFileExists(include_file))
> - pcontent = g_strdup_printf("%s%s", existing, included_files);
> - else
> + if (append && virFileExists(include_file)) {
pre existing, but I'd rather see this condition as: 'append &&
existing'. Might use this chance to change it.
> + /* duplicate check: include_files might contain multiple rules
> + * the best is to check for each rule (separated by \n) but
> + * it might be overkilled, just do the check for the whole
> + * include_files.
> + * most of the time, include_files contains only one rule
> + * so this check is ok to avoid the overflow of the profile
> + * duplicates might still exist though.
> + */
> + if (!strstr(existing, included_files)) {
> + pcontent = g_strdup_printf("%s%s", existing, included_files);
> + } else
> + pcontent = g_strdup(existing);
Why not exit early in this case? We are not going to change the contents
of the file anyway.
> + } else
> pcontent = g_strdup_printf("%s%s", warning, included_files);
>
> plen = strlen(pcontent);
And overall, our coding style says, that if one branch of if() statement
is wrapped in curly braces, the other branch must be wrapped too.
Michal
