[Xen-devel] [PATCH 9/9] libxl: Kill QEMU with "reaper" ruid

[George Dunlap <george.dunlap@xxxxxxxxxx>]

Using kill(-1) to killing an untrusted dm process with the real uid
equal to the dm_uid isn't guaranteed to succeed: the process in
question may be able to kill the reaper process after the setresuid()
and before the kill().

Instead, set the real uid to the QEMU user for domain 0
(QEMU_USER_RANGE_BASE + 0).  The reaper process will still be able to
kill the dm process, but not vice versa.

This, in turn, requires locking to make sure that only one reaper
process is using that uid at a time; otherwise one reaper process may
kill the other reaper process.

Create a lockfile in RUNDIR/dm-reaper-lock, and grab the lock before
executing kill.

In the event that we can't get the lock for some reason, go ahead with
the kill using dm_uid for both real and effective UIDs.  This isn't
guaranteed to work, but it's no worse than not trying to kill the
process at all.

Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
---
CC: Ian Jackson <ian.jackson@xxxxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
---
 tools/libxl/libxl_dm.c | 67 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 63 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 0997865773..846d23bddd 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -211,7 +211,16 @@ end_search:
     state->dm_uid = uid;
     return 0;
 }
-                                   
+
+static int libxl__get_reaper_uid(libxl__gc *gc)
+{
+    struct passwd *user_base, user_pwbuf;
+    int ret;
+    ret = userlookup_helper_getpwnam(gc, LIBXL_QEMU_USER_RANGE_BASE,
+                                         &user_pwbuf, &user_base);
+    return (ret < 0) ? ret : user_base->pw_uid;
+}
+
 const char *libxl__domain_device_model(libxl__gc *gc,
                                        const libxl_domain_build_info *info)
 {
@@ -2719,12 +2728,62 @@ void libxl__destroy_device_model(libxl__egc *egc,
     
     if (!reaper_pid) { /* child */
         const char * call;
+        const char * lockfile;
+        int fd;
+        uid_t reaper_uid = dm_uid;
+
+        /* 
+         * Try to kill the devicemodel by uid.  The safest way to do
+         * this is to set euid == dm_uid, but the ruid to something
+         * else.  If we can't get an ruid, carry on trying to kill the
+         * process anyway using dm_uid for the ruid.
+         */
+
+        /* Try to lock the "reaper uid" */
+        lockfile = GCSPRINTF("%s/dm-reaper-lock", libxl__run_dir_path());
 
         /* 
-         * FIXME: the second uid needs to be distinct to avoid being
-         * killed by a potential rogue process
+         * NB that since we've just forked, we can't have any
+         * threads; so we don't need the libxl__carefd
+         * infrastructure here.
          */
-        ret = setresuid(dm_uid, dm_uid, 0);
+        fd = open(lockfile, O_RDWR|O_CREAT, 0666);
+        if (fd < 0) {
+            /* All other errno: EBADF, EINVAL, ENOLCK, EWOULDBLOCK */
+            LOGED(ERROR, domid,
+                  "unexpected error while trying to open lockfile %s, 
errno=%d",
+                  lockfile, errno);
+            goto kill;
+        }
+
+        /* Try to lock the file */
+        while (flock(fd, LOCK_EX)) {
+            switch (errno) {
+            case EINTR:
+                /* Signal received, retry */
+                continue;
+            default:
+                /* All other errno: EBADF, EINVAL, ENOLCK, EWOULDBLOCK */
+                LOGED(ERROR, domid,
+                      "unexpected error while trying to lock %s, fd=%d, 
errno=%d",
+                      lockfile, fd, errno);
+                goto kill;
+            }
+        }
+
+        /* Get reaper_uid */
+        reaper_uid = libxl__get_reaper_uid(gc);
+        if (reaper_uid < 0) {
+            LOGE(ERROR, "Couldn't get reaper UID");
+            reaper_uid = dm_uid;
+        }
+        
+    kill:
+        if (reaper_uid == dm_uid)
+            LOG(WARN, "Couldn't get separate reaper uid;"
+                "carrying on with unsafe kill");
+        
+        ret = setresuid(reaper_uid, dm_uid, 0);
         if (ret) {
             call = "setresuid";
             goto badchild;
-- 
2.19.1


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel