There are some network file systems that do support XATTRs (e.g.
gluster via FUSE). And they appear to support SELinux too.
However, not really. Problem is, that it is impossible to change
SELinux label of a file stored there, and yet we claim success
(rightfully - hypervisor succeeds in opening the file). But this
creates a problem for us - from XATTR bookkeeping POV, we haven't
changed the label and thus if we remembered any label, we must
roll back and remove it.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1740506

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/security/security_selinux.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 855eaafdda..4d0c7a46ae 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1384,12 +1384,22 @@ virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr,
         }
     }
 
-    if (virSecuritySELinuxSetFileconImpl(path, tcon, privileged) < 0)
+    if ((rc = virSecuritySELinuxSetFileconImpl(path, tcon, privileged)) < 0)
         goto cleanup;
 
+    /* At this point, we can claim success. However,
+     * virSecuritySELinuxSetFileconImpl() could returned 0
+     * (SELinux label changed) or 1 (SELinux label NOT changed in
+     * a non-critical fashion). If the label was NOT changed, we
+     * must remove remembered label then - there's nothing to
+     * remember, is there? But of the label was changed, don't
+     * remove the remembered label. It's valid. */
+    if (rc == 0)
+        rollback = false;
+
     ret = 0;
  cleanup:
-    if (ret < 0 && rollback) {
+    if (rollback) {
         virErrorPtr origerr;
 
         virErrorPreserveLast(&origerr);
-- 
2.21.0

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

On Thu, Aug 22, 2019 at 05:19:09PM +0200, Michal Privoznik wrote:
>There are some network file systems that do support XATTRs (e.g.
>gluster via FUSE). And they appear to support SELinux too.
>However, not really. Problem is, that it is impossible to change
>SELinux label of a file stored there, and yet we claim success
>(rightfully - hypervisor succeeds in opening the file). But this
>creates a problem for us - from XATTR bookkeeping POV, we haven't
>changed the label and thus if we remembered any label, we must
>roll back and remove it.
>
>Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1740506
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/security/security_selinux.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
>diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
>index 855eaafdda..4d0c7a46ae 100644
>--- a/src/security/security_selinux.c
>+++ b/src/security/security_selinux.c
>@@ -1384,12 +1384,22 @@ virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr,
>         }
>     }
>
>-    if (virSecuritySELinuxSetFileconImpl(path, tcon, privileged) < 0)
>+    if ((rc = virSecuritySELinuxSetFileconImpl(path, tcon, privileged)) < 0)

I wonder why so many people try to stuff as much as possible into the condition
instead of doing:

    rc = func();
    if (rc < 0)

But anyway, this is not related to this commit, just a place to rent.

>         goto cleanup;
>
>+    /* At this point, we can claim success. However,
>+     * virSecuritySELinuxSetFileconImpl() could returned 0
>+     * (SELinux label changed) or 1 (SELinux label NOT changed in
>+     * a non-critical fashion). If the label was NOT changed, we
>+     * must remove remembered label then - there's nothing to
>+     * remember, is there? But of the label was changed, don't

s/of/if/, but I think it is overcomplicated.  Why don't you just:

    /* Do not try restoring the label if it was not changed
     * (setting it failed in a non-critical fashion) */

Either way:

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list