[libvirt] [PATCH] apparmor: avoid copying empty profile name

Jim Fehlig posted 1 patch 1 week ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190909162620.12249-1-jfehlig@suse.com
src/security/security_apparmor.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)

[libvirt] [PATCH] apparmor: avoid copying empty profile name

Posted by Jim Fehlig 1 week ago
AppArmorGetSecurityProcessLabel copies the VM's profile name to the
label member of virSecurityLabel struct. If the profile is not loaded,
the name is set empty before calling virStrcpy to copy it. However,
virStrcpy will fail if src is empty (0 length), causing
AppArmorGetSecurityProcessLabel to needlessly fail. Simple operations
that report security driver information will subsequently fail

virsh dominfo test
Id:             248
Name:           test
...
Security model: apparmor
Security DOI:   0
error: internal error: error copying profile name

Avoid copying an empty profile name when the profile is not loaded.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 src/security/security_apparmor.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 6d16b15c65..77eee9410c 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -525,14 +525,13 @@ AppArmorGetSecurityProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
                        "%s", _("error getting profile status"));
         goto cleanup;
     } else if (status == -1) {
-        profile_name[0] = '\0';
-    }
-
-    if (virStrcpy(sec->label, profile_name,
-        VIR_SECURITY_LABEL_BUFLEN) < 0) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       "%s", _("error copying profile name"));
-        goto cleanup;
+        sec->label[0] = '\0';
+    } else {
+        if (virStrcpy(sec->label, profile_name, VIR_SECURITY_LABEL_BUFLEN) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           "%s", _("error copying profile name"));
+            goto cleanup;
+        }
     }
 
     sec->enforcing = status == 1;
-- 
2.23.0


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] apparmor: avoid copying empty profile name

Posted by Michal Privoznik 1 week ago
On 9/9/19 6:26 PM, Jim Fehlig wrote:
> AppArmorGetSecurityProcessLabel copies the VM's profile name to the
> label member of virSecurityLabel struct. If the profile is not loaded,
> the name is set empty before calling virStrcpy to copy it. However,
> virStrcpy will fail if src is empty (0 length), causing
> AppArmorGetSecurityProcessLabel to needlessly fail. Simple operations
> that report security driver information will subsequently fail
> 
> virsh dominfo test
> Id:             248
> Name:           test
> ...
> Security model: apparmor
> Security DOI:   0
> error: internal error: error copying profile name
> 
> Avoid copying an empty profile name when the profile is not loaded.
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>   src/security/security_apparmor.c | 15 +++++++--------
>   1 file changed, 7 insertions(+), 8 deletions(-)

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] apparmor: avoid copying empty profile name

Posted by Jim Fehlig 1 week ago
On 9/9/19 10:26 AM, Jim Fehlig wrote:
> AppArmorGetSecurityProcessLabel copies the VM's profile name to the
> label member of virSecurityLabel struct. If the profile is not loaded,
> the name is set empty before calling virStrcpy to copy it. However,
> virStrcpy will fail if src is empty (0 length), causing
> AppArmorGetSecurityProcessLabel to needlessly fail. Simple operations
> that report security driver information will subsequently fail
> 
> virsh dominfo test
> Id:             248
> Name:           test
> ...
> Security model: apparmor
> Security DOI:   0
> error: internal error: error copying profile name
> 
> Avoid copying an empty profile name when the profile is not loaded.

Any comments on this patch? There are a lot of ways to skin the cat, but I think 
we can agree that copying an empty profile name should be avoided.

Regards,
Jim

> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>   src/security/security_apparmor.c | 15 +++++++--------
>   1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
> index 6d16b15c65..77eee9410c 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -525,14 +525,13 @@ AppArmorGetSecurityProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
>                          "%s", _("error getting profile status"));
>           goto cleanup;
>       } else if (status == -1) {
> -        profile_name[0] = '\0';
> -    }
> -
> -    if (virStrcpy(sec->label, profile_name,
> -        VIR_SECURITY_LABEL_BUFLEN) < 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       "%s", _("error copying profile name"));
> -        goto cleanup;
> +        sec->label[0] = '\0';
> +    } else {
> +        if (virStrcpy(sec->label, profile_name, VIR_SECURITY_LABEL_BUFLEN) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           "%s", _("error copying profile name"));
> +            goto cleanup;
> +        }
>       }
>   
>       sec->enforcing = status == 1;
> 


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list