[PATCH 4/4] security: Always spawn process for transactions

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

There is this latent bug which can result in virtlockd killing
libvirtd. The problem is when the following steps occur:

   Parent                                             |  Child
------------------------------------------------------+----------------
1) virSecurityManagerMetadataLock(path);              |
   This results in connection FD to virtlockd to be   |
   dup()-ed and saved for later use.                  |
                                                      |
2) Another thread calling fork();                     |
   This results in the FD from step 1) to be cloned   |
                                                      |
3) virSecurityManagerMetadataUnlock(path);            |
   Here, the FD is closed, but the connection is      |
   still kept open because child process has          |
   duplicated FD                                      |
                                                      |
4) virSecurityManagerMetadataLock(differentPath);     |
   Again, new connection is opened, FD is dup()-ed    |
                                                      |
5)                                                    | exec() or exit()

Step 5) results in closing the connection from 1) to be closed,
finally. However, at this point virtlockd looks at its internal
records if PID from 1) does not still own any resources. Well it
does - in step 4) it locked differentPath. This results in
virtlockd killing libvirtd.

Note that this happens because we do some relabelling even before
fork() at which point vm->pid is still -1. When this value is
passed down to transaction code it simply runs the transaction
in parent process (=libvirtd), which means the owner of metadata
locks is the parent process.

Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>

Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
 src/security/security_dac.c     | 12 ++++++------
 src/security/security_selinux.c | 12 ++++++------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 5aea386e7c..876cca0f2f 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -561,12 +561,12 @@ virSecurityDACTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
         goto cleanup;
     }
 
-    if ((pid == -1 &&
-         virSecurityDACTransactionRun(pid, list) < 0) ||
-        (pid != -1 &&
-         virProcessRunInMountNamespace(pid,
-                                       virSecurityDACTransactionRun,
-                                       list) < 0))
+    if (pid == -1)
+        pid = getpid();
+
+    if (virProcessRunInMountNamespace(pid,
+                                      virSecurityDACTransactionRun,
+                                      list) < 0)
         goto cleanup;
 
     ret = 0;
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 31e42afee7..1e23d776ff 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1102,12 +1102,12 @@ virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
         goto cleanup;
     }
 
-    if ((pid == -1 &&
-         virSecuritySELinuxTransactionRun(pid, list) < 0) ||
-        (pid != -1 &&
-         virProcessRunInMountNamespace(pid,
-                                       virSecuritySELinuxTransactionRun,
-                                       list) < 0))
+    if (pid == -1)
+        pid = getpid();
+
+    if (virProcessRunInMountNamespace(pid,
+                                      virSecuritySELinuxTransactionRun,
+                                      list) < 0)
         goto cleanup;
 
     ret = 0;
-- 
2.16.4

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