From: Hector Cao <hector.cao@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.cao@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)) {
+        /* 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);
+    } else
         pcontent = g_strdup_printf("%s%s", warning, included_files);
 
     plen = strlen(pcontent);
-- 
2.34.1

On 8/19/25 20:46, Hector CAO wrote:
> From: Hector Cao <hector.cao@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.cao@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

Hello Michal,

Thanks for the feedback !
Here is the v2 where I took into consideration your valuable feedback.
Please take a look !

https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/J5IC2LB2LZBWXGOKIO33UG7IU7SGXBG3/

Best,
Hector