[PATCH 1/9] libxl: Remove redundant pidpath setting

[PATCH 1/9] libxl: Remove redundant pidpath setting

[George Dunlap]

This exact same line is duplicated further on without being used or
modified in between.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_dm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 9c47060473..5698fe8af3 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2348,7 +2348,6 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
     }
 
     const char *dom_path = libxl__xs_get_dompath(gc, domid);
-    spawn->pidpath = GCSPRINTF("%s/%s", dom_path, "image/device-model-pid");
 
     if (vnc && vnc->passwd) {
         /* This xenstore key will only be used by qemu-xen-traditionnal.
-- 
2.19.1


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

[PATCH 2/9] libxl: Move dm user determination logic into a helper function

[George Dunlap]

To reliably kill an untrusted devicemodel, we need to know not only
its pid, but its uid.  In preparation for this, move the userid
determination logic into a helper function.

Create a new field, `dm_runas`, in libxl__domain_build_state to store
the value during domain creation.

This change also removes unnecessary duplication of the argument
construction code.

No functional change intended.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Anthony Perard <anthony.perard@citrix.com>
---
 tools/libxl/libxl_dm.c       | 248 +++++++++++++++++++----------------
 tools/libxl/libxl_internal.h |   2 +
 2 files changed, 135 insertions(+), 115 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 5698fe8af3..8764a2b58b 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -65,6 +65,131 @@ static int libxl__create_qemu_logfile(libxl__gc *gc, char *name)
     return logfile_w;
 }
 
+/*
+ *  userlookup_helper_getpwnam(libxl__gc*, const char *user,
+ *                             struct passwd **pwd_r);
+ *
+ *  userlookup_helper_getpwuid(libxl__gc*, uid_t uid,
+ *                             struct passwd **pwd_r);
+ *
+ *  returns 1 if the user was found, 0 if it was not, -1 on error
+ */
+#define DEFINE_USERLOOKUP_HELPER(NAME,SPEC_TYPE,STRUCTNAME,SYSCONF)     \
+    static int userlookup_helper_##NAME(libxl__gc *gc,                  \
+                                        SPEC_TYPE spec,                 \
+                                        struct STRUCTNAME *resultbuf,   \
+                                        struct STRUCTNAME **out)        \
+    {                                                                   \
+        struct STRUCTNAME *resultp = NULL;                              \
+        char *buf = NULL;                                               \
+        long buf_size;                                                  \
+        int ret;                                                        \
+                                                                        \
+        buf_size = sysconf(SYSCONF);                                    \
+        if (buf_size < 0) {                                             \
+            buf_size = 2048;                                            \
+            LOG(DEBUG,                                                  \
+    "sysconf failed, setting the initial buffer size to %ld",           \
+                buf_size);                                              \
+        }                                                               \
+                                                                        \
+        while (1) {                                                     \
+            buf = libxl__realloc(gc, buf, buf_size);                    \
+            ret = NAME##_r(spec, resultbuf, buf, buf_size, &resultp);   \
+            if (ret == ERANGE) {                                        \
+                buf_size += 128;                                        \
+                continue;                                               \
+            }                                                           \
+            if (ret != 0)                                               \
+                return ERROR_FAIL;                                      \
+            if (resultp != NULL) {                                      \
+                if (out) *out = resultp;                                \
+                return 1;                                               \
+            }                                                           \
+            return 0;                                                   \
+        }                                                               \
+    }
+
+DEFINE_USERLOOKUP_HELPER(getpwnam, const char*, passwd, _SC_GETPW_R_SIZE_MAX);
+DEFINE_USERLOOKUP_HELPER(getpwuid, uid_t,       passwd, _SC_GETPW_R_SIZE_MAX);
+
+static int libxl__domain_get_device_model_uid(libxl__gc *gc,
+                                              libxl__dm_spawn_state *dmss)
+{
+    int guest_domid = dmss->guest_domid;
+    libxl__domain_build_state *const state = dmss->build_state;
+    const libxl_domain_build_info *b_info = &dmss->guest_config->b_info;
+    
+    struct passwd *user_base, user_pwbuf;
+    int ret;
+    char *user;
+
+    /* Only qemu-upstream can run as a different uid */
+    if (b_info->device_model_version != LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN)
+        return 0;
+    
+    user = b_info->device_model_user;
+    if (user)
+        goto end_search;
+
+    if (!libxl_defbool_val(b_info->dm_restrict)) {
+        LOGD(DEBUG, guest_domid,
+             "dm_restrict disabled, starting QEMU as root");
+        return 0;
+    }
+
+    user = GCSPRINTF("%s%d", LIBXL_QEMU_USER_BASE, guest_domid);
+    ret = userlookup_helper_getpwnam(gc, user, &user_pwbuf, 0);
+    if (ret < 0)
+        return ret;
+    if (ret > 0)
+        goto end_search;
+
+    ret = userlookup_helper_getpwnam(gc, LIBXL_QEMU_USER_RANGE_BASE,
+                                         &user_pwbuf, &user_base);
+    if (ret < 0)
+        return ret;
+    if (ret > 0) {
+        struct passwd *user_clash, user_clash_pwbuf;
+        uid_t intended_uid = user_base->pw_uid + guest_domid;
+        ret = userlookup_helper_getpwuid(gc, intended_uid,
+                                         &user_clash_pwbuf, &user_clash);
+        if (ret < 0)
+            return ret;
+        if (ret > 0) {
+            LOGD(ERROR, guest_domid,
+                 "wanted to use uid %ld (%s + %d) but that is user %s !",
+                 (long)intended_uid, LIBXL_QEMU_USER_RANGE_BASE,
+                 guest_domid, user_clash->pw_name);
+            return ERROR_FAIL;
+        }
+        LOGD(DEBUG, guest_domid, "using uid %ld", (long)intended_uid);
+        user = GCSPRINTF("%ld:%ld", (long)intended_uid,
+                         (long)user_base->pw_gid);
+        goto end_search;
+    }
+    
+    user = LIBXL_QEMU_USER_SHARED;
+    ret = userlookup_helper_getpwnam(gc, user, &user_pwbuf, 0);
+    if (ret < 0)
+        return ret;
+    if (ret > 0) {
+        LOGD(WARN, guest_domid, "Could not find user %s%d, falling back to %s",
+             LIBXL_QEMU_USER_BASE, guest_domid, LIBXL_QEMU_USER_SHARED);
+        goto end_search;
+    }
+    
+    LOGD(ERROR, guest_domid,
+         "Could not find user %s%d or %s or range base pseudo-user %s, cannot restrict",
+         LIBXL_QEMU_USER_BASE, guest_domid, LIBXL_QEMU_USER_SHARED,
+         LIBXL_QEMU_USER_RANGE_BASE);
+    return ERROR_INVAL;
+    
+end_search:
+    state->dm_runas = user;
+    return 0;
+}
+                                   
 const char *libxl__domain_device_model(libxl__gc *gc,
                                        const libxl_domain_build_info *info)
 {
@@ -737,54 +862,6 @@ libxl__detect_gfx_passthru_kind(libxl__gc *gc,
     return LIBXL_GFX_PASSTHRU_KIND_DEFAULT;
 }
 
-/*
- *  userlookup_helper_getpwnam(libxl__gc*, const char *user,
- *                             struct passwd **pwd_r);
- *
- *  userlookup_helper_getpwuid(libxl__gc*, uid_t uid,
- *                             struct passwd **pwd_r);
- *
- *  returns 1 if the user was found, 0 if it was not, -1 on error
- */
-#define DEFINE_USERLOOKUP_HELPER(NAME,SPEC_TYPE,STRUCTNAME,SYSCONF)     \
-    static int userlookup_helper_##NAME(libxl__gc *gc,                  \
-                                        SPEC_TYPE spec,                 \
-                                        struct STRUCTNAME *resultbuf,   \
-                                        struct STRUCTNAME **out)        \
-    {                                                                   \
-        struct STRUCTNAME *resultp = NULL;                              \
-        char *buf = NULL;                                               \
-        long buf_size;                                                  \
-        int ret;                                                        \
-                                                                        \
-        buf_size = sysconf(SYSCONF);                                    \
-        if (buf_size < 0) {                                             \
-            buf_size = 2048;                                            \
-            LOG(DEBUG,                                                  \
-    "sysconf failed, setting the initial buffer size to %ld",           \
-                buf_size);                                              \
-        }                                                               \
-                                                                        \
-        while (1) {                                                     \
-            buf = libxl__realloc(gc, buf, buf_size);                    \
-            ret = NAME##_r(spec, resultbuf, buf, buf_size, &resultp);   \
-            if (ret == ERANGE) {                                        \
-                buf_size += 128;                                        \
-                continue;                                               \
-            }                                                           \
-            if (ret != 0)                                               \
-                return ERROR_FAIL;                                      \
-            if (resultp != NULL) {                                      \
-                if (out) *out = resultp;                                \
-                return 1;                                               \
-            }                                                           \
-            return 0;                                                   \
-        }                                                               \
-    }
-
-DEFINE_USERLOOKUP_HELPER(getpwnam, const char*, passwd, _SC_GETPW_R_SIZE_MAX);
-DEFINE_USERLOOKUP_HELPER(getpwuid, uid_t,       passwd, _SC_GETPW_R_SIZE_MAX);
-
 /* colo mode */
 enum {
     LIBXL__COLO_NONE = 0,
@@ -928,11 +1005,9 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
     const char *keymap = dm_keymap(guest_config);
     char *machinearg;
     flexarray_t *dm_args, *dm_envs;
-    int i, connection, devid, ret;
+    int i, connection, devid;
     uint64_t ram_size;
     const char *path, *chardev;
-    char *user = NULL;
-    struct passwd *user_base, user_pwbuf;
 
     dm_args = flexarray_make(gc, 16, 1);
     dm_envs = flexarray_make(gc, 16, 1);
@@ -1685,71 +1760,9 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
             break;
         }
 
-        if (b_info->device_model_user) {
-            user = b_info->device_model_user;
-            goto end_search;
-        }
-
-        if (!libxl_defbool_val(b_info->dm_restrict)) {
-            LOGD(DEBUG, guest_domid,
-                 "dm_restrict disabled, starting QEMU as root");
-            goto end_search;
-        }
-
-        user = GCSPRINTF("%s%d", LIBXL_QEMU_USER_BASE, guest_domid);
-        ret = userlookup_helper_getpwnam(gc, user, &user_pwbuf, 0);
-        if (ret < 0)
-            return ret;
-        if (ret > 0)
-            goto end_search;
-
-        ret = userlookup_helper_getpwnam(gc, LIBXL_QEMU_USER_RANGE_BASE,
-                                         &user_pwbuf, &user_base);
-        if (ret < 0)
-            return ret;
-        if (ret > 0) {
-            struct passwd *user_clash, user_clash_pwbuf;
-            uid_t intended_uid = user_base->pw_uid + guest_domid;
-            ret = userlookup_helper_getpwuid(gc, intended_uid,
-                                             &user_clash_pwbuf, &user_clash);
-            if (ret < 0)
-                return ret;
-            if (ret > 0) {
-                LOGD(ERROR, guest_domid,
-                     "wanted to use uid %ld (%s + %d) but that is user %s !",
-                     (long)intended_uid, LIBXL_QEMU_USER_RANGE_BASE,
-                     guest_domid, user_clash->pw_name);
-                return ERROR_FAIL;
-            }
-            LOGD(DEBUG, guest_domid, "using uid %ld", (long)intended_uid);
+        if (state->dm_runas && strcmp(state->dm_runas, "root")) {
             flexarray_append(dm_args, "-runas");
-            flexarray_append(dm_args,
-                             GCSPRINTF("%ld:%ld", (long)intended_uid,
-                                       (long)user_base->pw_gid));
-            user = NULL; /* we have taken care of it */
-            goto end_search;
-        }
-
-        user = LIBXL_QEMU_USER_SHARED;
-        ret = userlookup_helper_getpwnam(gc, user, &user_pwbuf, 0);
-        if (ret < 0)
-            return ret;
-        if (ret > 0) {
-            LOGD(WARN, guest_domid, "Could not find user %s%d, falling back to %s",
-                    LIBXL_QEMU_USER_BASE, guest_domid, LIBXL_QEMU_USER_SHARED);
-            goto end_search;
-        }
-
-        LOGD(ERROR, guest_domid,
- "Could not find user %s%d or %s or range base pseudo-user %s, cannot restrict",
-             LIBXL_QEMU_USER_BASE, guest_domid, LIBXL_QEMU_USER_SHARED,
-             LIBXL_QEMU_USER_RANGE_BASE);
-        return ERROR_INVAL;
-
-end_search:
-        if (user != NULL && strcmp(user, "root")) {
-            flexarray_append(dm_args, "-runas");
-            flexarray_append(dm_args, user);
+            flexarray_append(dm_args, state->dm_runas);
         }
     }
     flexarray_append(dm_args, NULL);
@@ -2303,6 +2316,11 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
         rc = ERROR_FAIL;
         goto out;
     }
+
+    rc = libxl__domain_get_device_model_uid(gc, dmss);
+    if (rc)
+        goto out;
+    
     rc = libxl__build_device_model_args(gc, dm, domid, guest_config,
                                           &args, &envs, state,
                                           &dm_state_fd);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e498435e16..a370de54ed 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1135,6 +1135,8 @@ typedef struct {
     const char * shim_cmdline;
     const char * pv_cmdline;
 
+    char * dm_runas;
+
     xen_vmemrange_t *vmemranges;
     uint32_t num_vmemranges;
 
-- 
2.19.1


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

[PATCH 3/9] libxl: Get rid of support for QEMU_USER_BASE (xen-qemuuser-domidNN)

[George Dunlap]

QEMU_USER_BASE allows a user to specify the UID to use when running
the devicemodel for a specific domain number.  Unfortunately, this is
not really practical: It requires nearly 32,000 entries in
/etc/passwd.  QEMU_USER_RANGE_BASE is much more practical.

Remove support for QEMU_USER_BASE.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_dm.c       | 16 ++++------------
 tools/libxl/libxl_internal.h |  1 -
 2 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8764a2b58b..30038eb4e9 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -138,13 +138,6 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
         return 0;
     }
 
-    user = GCSPRINTF("%s%d", LIBXL_QEMU_USER_BASE, guest_domid);
-    ret = userlookup_helper_getpwnam(gc, user, &user_pwbuf, 0);
-    if (ret < 0)
-        return ret;
-    if (ret > 0)
-        goto end_search;
-
     ret = userlookup_helper_getpwnam(gc, LIBXL_QEMU_USER_RANGE_BASE,
                                          &user_pwbuf, &user_base);
     if (ret < 0)
@@ -174,15 +167,14 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
     if (ret < 0)
         return ret;
     if (ret > 0) {
-        LOGD(WARN, guest_domid, "Could not find user %s%d, falling back to %s",
-             LIBXL_QEMU_USER_BASE, guest_domid, LIBXL_QEMU_USER_SHARED);
+        LOGD(WARN, guest_domid, "Could not find user %s, falling back to %s",
+             LIBXL_QEMU_USER_RANGE_BASE, LIBXL_QEMU_USER_SHARED);
         goto end_search;
     }
     
     LOGD(ERROR, guest_domid,
-         "Could not find user %s%d or %s or range base pseudo-user %s, cannot restrict",
-         LIBXL_QEMU_USER_BASE, guest_domid, LIBXL_QEMU_USER_SHARED,
-         LIBXL_QEMU_USER_RANGE_BASE);
+         "Could not find user %s or range base pseudo-user %s, cannot restrict",
+         LIBXL_QEMU_USER_SHARED, LIBXL_QEMU_USER_RANGE_BASE);
     return ERROR_INVAL;
     
 end_search:
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a370de54ed..853a4f3d88 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4387,7 +4387,6 @@ _hidden int libxl__read_sysfs_file_contents(libxl__gc *gc,
                                             int *datalen_r);
 
 #define LIBXL_QEMU_USER_PREFIX "xen-qemuuser"
-#define LIBXL_QEMU_USER_BASE   LIBXL_QEMU_USER_PREFIX"-domid"
 #define LIBXL_QEMU_USER_SHARED LIBXL_QEMU_USER_PREFIX"-shared"
 #define LIBXL_QEMU_USER_RANGE_BASE LIBXL_QEMU_USER_PREFIX"-range-base"
 
-- 
2.19.1


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

[PATCH 4/9] dm_depriv: Describe expected usage of device_model_user parameter

[George Dunlap]

A number of subsequent patches rely on as-yet undefined behavior for
what the `device_model_user` parameter does.  Rather than implement it
incorrectly (or randomly), or remove the feature, describe an expected
usage for the feature.  Further patches will make decisions based on
this expected usage.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Anthony Perard <anthony.perard@citrix.com>
---
 docs/features/qemu-deprivilege.pandoc | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/docs/features/qemu-deprivilege.pandoc b/docs/features/qemu-deprivilege.pandoc
index f941525189..49b571980e 100644
--- a/docs/features/qemu-deprivilege.pandoc
+++ b/docs/features/qemu-deprivilege.pandoc
@@ -66,6 +66,23 @@ this, create a user named `xen-qemuuser-shared`; for example:
 
     adduser --no-create-home --system xen-qemuuser-shared
 
+A final way to set up a separate process for qemus is to allocate one
+UID per VM, and set the UID in the domain config file with the
+`device_model_user` argument.  For example, suppose you have a VM
+named `c6-01`.  You might do the following:
+
+    adduser --system --no-create-home --group xen-qemuuuser-c6-01
+
+And then in your config file, the following line:
+
+    device_model_user="xen-qemuuser-c6-01"
+
+NOTE: It is important when using `device_model_user` that EACH VM HAVE
+A SEPARATE UID, and that none of these UIDs map to root.  xl will
+throw an error a uid maps to zero, but not if multiple VMs have the
+same uid.  Multiple VMs with the same device model uid will cause
+problems.
+
 ## Domain config changes
 
 The core domain config change is to add the following line to the
-- 
2.19.1


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

[PATCH 5/9] libxl: Do root checks once in libxl__domain_get_device_model_uid

[George Dunlap]

At the moment, we check for equivalence to literal "root" before
deciding whether to add the `runas` command-line option to QEMU.  This
is unsatisfactory for several reasons.

First, just because the string doesn't match "root" doesn't mean the
final uid won't end up being zero; in particular, the range_base
calculations may end up producing "0:NNN", which would be root in any
case.

Secondly, it's almost certainly a configuration error if the resulting
uid ends up to be zero; rather than silently do what was specified but
probably not intended, throw an error.

To fix this, check for root once in
libxl__domain_get_device_model_uid.  If the result is root, return an
error; if appropriate, set the user.

After that, assume that the presence of state->dm_runas implies that a
`runas` argument should be constructed.

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

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 30038eb4e9..3cc6bc0f1d 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -129,8 +129,18 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
         return 0;
     
     user = b_info->device_model_user;
-    if (user)
-        goto end_search;
+    if (user) {
+        ret = userlookup_helper_getpwnam(gc, user, &user_pwbuf, &user_base);
+        if (ret < 0)
+            return ret;
+        if (!ret) {
+            LOGD(ERROR, guest_domid,
+                 "Couldn't find device_model_user %s",
+                 user);
+            return -EINVAL;
+        }
+        goto root_check;
+    }
 
     if (!libxl_defbool_val(b_info->dm_restrict)) {
         LOGD(DEBUG, guest_domid,
@@ -156,6 +166,12 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
                  guest_domid, user_clash->pw_name);
             return ERROR_FAIL;
         }
+
+        if (intended_uid == 0) {
+            LOGD(ERROR, guest_domid, "intended_uid is 0 (root)!");
+            return ERROR_INVAL;
+        }
+        
         LOGD(DEBUG, guest_domid, "using uid %ld", (long)intended_uid);
         user = GCSPRINTF("%ld:%ld", (long)intended_uid,
                          (long)user_base->pw_gid);
@@ -163,19 +179,26 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
     }
     
     user = LIBXL_QEMU_USER_SHARED;
-    ret = userlookup_helper_getpwnam(gc, user, &user_pwbuf, 0);
+    ret = userlookup_helper_getpwnam(gc, user, &user_pwbuf, &user_base);
     if (ret < 0)
         return ret;
     if (ret > 0) {
         LOGD(WARN, guest_domid, "Could not find user %s, falling back to %s",
              LIBXL_QEMU_USER_RANGE_BASE, LIBXL_QEMU_USER_SHARED);
-        goto end_search;
+        goto root_check;
     }
     
     LOGD(ERROR, guest_domid,
          "Could not find user %s or range base pseudo-user %s, cannot restrict",
          LIBXL_QEMU_USER_SHARED, LIBXL_QEMU_USER_RANGE_BASE);
     return ERROR_INVAL;
+
+root_check:
+    /* Make sure that the user doesn't map to root. */
+    if (user_base->pw_uid == 0) {
+        LOGD(ERROR, guest_domid, "User %s maps to uid 0 (root)!", user);
+        return ERROR_INVAL;
+    }
     
 end_search:
     state->dm_runas = user;
@@ -1752,7 +1775,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
             break;
         }
 
-        if (state->dm_runas && strcmp(state->dm_runas, "root")) {
+        if (state->dm_runas) {
             flexarray_append(dm_args, "-runas");
             flexarray_append(dm_args, state->dm_runas);
         }
-- 
2.19.1


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

[PATCH 6/9] libxl: Move qmp cleanup into devicemodel destroy function

[George Dunlap]

Removing the qmp connection is logically part of the device model
destruction; having the caller destroy it is a mild layering
violation.

Move libxl__qmp_cleanup() into libxl__destroy_device_model().  This
will make it easier when we make devicemodel destruction asynchronous.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_dm.c     | 9 +++++++--
 tools/libxl/libxl_domain.c | 2 --
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 3cc6bc0f1d..3e7d273812 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2653,12 +2653,17 @@ out:
 
 int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
 {
+    int rc;
     char *path = DEVICE_MODEL_XS_PATH(gc, LIBXL_TOOLSTACK_DOMID, domid, "");
     if (!xs_rm(CTX->xsh, XBT_NULL, path))
         LOGD(ERROR, domid, "xs_rm failed for %s", path);
     /* We should try to destroy the device model anyway. */
-    return kill_device_model(gc,
-                GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
+    rc = kill_device_model(gc,
+              GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
+    
+    libxl__qmp_cleanup(gc, domid);
+
+    return rc;
 }
 
 /* Return 0 if no dm needed, 1 if needed and <0 if error. */
diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
index 3377bba994..d46b97dedf 100644
--- a/tools/libxl/libxl_domain.c
+++ b/tools/libxl/libxl_domain.c
@@ -1069,8 +1069,6 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
     if (dm_present) {
         if (libxl__destroy_device_model(gc, domid) < 0)
             LOGD(ERROR, domid, "libxl__destroy_device_model failed");
-
-        libxl__qmp_cleanup(gc, domid);
     }
     dis->drs.ao = ao;
     dis->drs.domid = domid;
-- 
2.19.1


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

[PATCH 7/9] libxl: Make killing of device model asynchronous

[George Dunlap]

Or at least, give it an asynchronous interface so that we can make it
actually asynchronous in subsequent patches.

Create state structures and callback function signatures.  Add the
state structure to libxl__destroy_domid_state.  Break
libxl__destroy_domid down into two functions.

No functional change intended.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_dm.c       |  9 ++++++--
 tools/libxl/libxl_domain.c   | 40 ++++++++++++++++++++++++++++--------
 tools/libxl/libxl_internal.h | 20 ++++++++++++++++--
 3 files changed, 57 insertions(+), 12 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 3e7d273812..8f7a1d7524 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2651,19 +2651,24 @@ out:
     return rc;
 }
 
-int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
+void libxl__destroy_device_model(libxl__egc *egc,
+                                 libxl__destroy_devicemodel_state *ddms)
 {
+    STATE_AO_GC(ddms->ao);
     int rc;
+    int domid = ddms->domid;
     char *path = DEVICE_MODEL_XS_PATH(gc, LIBXL_TOOLSTACK_DOMID, domid, "");
+    
     if (!xs_rm(CTX->xsh, XBT_NULL, path))
         LOGD(ERROR, domid, "xs_rm failed for %s", path);
+    
     /* We should try to destroy the device model anyway. */
     rc = kill_device_model(gc,
               GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
     
     libxl__qmp_cleanup(gc, domid);
 
-    return rc;
+    ddms->callback(egc, ddms, rc);
 }
 
 /* Return 0 if no dm needed, 1 if needed and <0 if error. */
diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
index d46b97dedf..0433042393 100644
--- a/tools/libxl/libxl_domain.c
+++ b/tools/libxl/libxl_domain.c
@@ -1008,6 +1008,10 @@ static void destroy_finish_check(libxl__egc *egc,
 }
 
 /* Callbacks for libxl__destroy_domid */
+static void dm_destroy_cb(libxl__egc *egc,
+                          libxl__destroy_devicemodel_state *ddms,
+                          int rc);
+
 static void devices_destroy_cb(libxl__egc *egc,
                                libxl__devices_remove_state *drs,
                                int rc);
@@ -1066,16 +1070,18 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
     if (rc < 0) {
         LOGEVD(ERROR, rc, domid, "xc_domain_pause failed");
     }
+
     if (dm_present) {
-        if (libxl__destroy_device_model(gc, domid) < 0)
-            LOGD(ERROR, domid, "libxl__destroy_device_model failed");
+        dis->ddms.ao = ao;
+        dis->ddms.domid = domid;
+        dis->ddms.callback = dm_destroy_cb;
+        
+        libxl__destroy_device_model(egc, &dis->ddms);
+        return;
+    } else {
+        dm_destroy_cb(egc, &dis->ddms, 0);
+        return;
     }
-    dis->drs.ao = ao;
-    dis->drs.domid = domid;
-    dis->drs.callback = devices_destroy_cb;
-    dis->drs.force = 1;
-    libxl__devices_destroy(egc, &dis->drs);
-    return;
 
 out:
     assert(rc);
@@ -1083,6 +1089,24 @@ out:
     return;
 }
 
+static void dm_destroy_cb(libxl__egc *egc,
+                          libxl__destroy_devicemodel_state *ddms,
+                          int rc)
+{
+    libxl__destroy_domid_state *dis = CONTAINER_OF(ddms, *dis, ddms);
+    STATE_AO_GC(dis->ao);
+    uint32_t domid = dis->domid;
+
+    if (rc < 0)
+        LOGD(ERROR, domid, "libxl__destroy_device_model failed");
+
+    dis->drs.ao = ao;
+    dis->drs.domid = domid;
+    dis->drs.callback = devices_destroy_cb;
+    dis->drs.force = 1;
+    libxl__devices_destroy(egc, &dis->drs);
+}
+
 static void devices_destroy_cb(libxl__egc *egc,
                                libxl__devices_remove_state *drs,
                                int rc)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 853a4f3d88..899a86e84b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1705,8 +1705,6 @@ _hidden int libxl__wait_for_device_model_deprecated(libxl__gc *gc,
                                                       void *userdata),
                                 void *check_callback_userdata);
 
-_hidden int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid);
-
 _hidden const libxl_vnc_info *libxl__dm_vnc(const libxl_domain_config *g_cfg);
 
 _hidden char *libxl__abs_path(libxl__gc *gc, const char *s, const char *path);
@@ -3672,6 +3670,7 @@ extern const struct libxl_device_type *device_type_tbl[];
 
 typedef struct libxl__domain_destroy_state libxl__domain_destroy_state;
 typedef struct libxl__destroy_domid_state libxl__destroy_domid_state;
+typedef struct libxl__destroy_devicemodel_state libxl__destroy_devicemodel_state;
 typedef struct libxl__devices_remove_state libxl__devices_remove_state;
 
 typedef void libxl__domain_destroy_cb(libxl__egc *egc,
@@ -3682,6 +3681,10 @@ typedef void libxl__domid_destroy_cb(libxl__egc *egc,
                                      libxl__destroy_domid_state *dis,
                                      int rc);
 
+typedef void libxl__devicemodel_destroy_cb(libxl__egc *egc,
+                                     libxl__destroy_devicemodel_state *ddms,
+                                     int rc);
+
 typedef void libxl__devices_remove_callback(libxl__egc *egc,
                                             libxl__devices_remove_state *drs,
                                             int rc);
@@ -3697,6 +3700,14 @@ struct libxl__devices_remove_state {
     int num_devices;
 };
 
+struct libxl__destroy_devicemodel_state {
+    /* filled in by user */
+    libxl__ao *ao;
+    uint32_t domid;
+    libxl__devicemodel_destroy_cb *callback;
+    /* private to implementation */
+};
+
 struct libxl__destroy_domid_state {
     /* filled in by user */
     libxl__ao *ao;
@@ -3704,6 +3715,7 @@ struct libxl__destroy_domid_state {
     libxl__domid_destroy_cb *callback;
     /* private to implementation */
     libxl__devices_remove_state drs;
+    libxl__destroy_devicemodel_state ddms;
     libxl__ev_child destroyer;
     bool soft_reset;
 };
@@ -3735,6 +3747,10 @@ _hidden void libxl__domain_destroy(libxl__egc *egc,
 _hidden void libxl__destroy_domid(libxl__egc *egc,
                                   libxl__destroy_domid_state *dis);
 
+/* Used to detroy the device model */
+_hidden void libxl__destroy_device_model(libxl__egc *egc,
+                                         libxl__destroy_devicemodel_state *ddms);
+
 /* Entry point for devices destruction */
 _hidden void libxl__devices_destroy(libxl__egc *egc,
                                     libxl__devices_remove_state *drs);
-- 
2.19.1


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

[PATCH 8/9] libxl: Kill QEMU by uid when possible

[George Dunlap]

The privcmd fd that a dm_restrict'ed QEMU has gives it permission to
one specific domain ID.  This domain ID will probably eventually be
used again.  It is therefore necessary to make absolutely sure that a
rogue QEMU process cannot hang around after its domain has exited.

Killing QEMU by pid is insufficient in this situation, because QEMU
may be able to fork() to escape killing.  It is surprisingly tricky to
kill a process which can call fork() without races; the only reliable
way is to use kill(-1) to kill all processes with a given uid.

We can use this method only when we're sure that there's only one QEMU
instance per uid.  Add a dm_uid into the domain_build_state struct,
and set it in libxl__domain_get_device_model_uid() when it's safe to
kill by UID.  Store this in xenstore next to device-model-pid.

On domain destroy, check to see if device-model-uid is present in
xenstore.  If so, fork off a reaper process, setuid to that uid, and
do kill(-9) to kill all uids of that type.  Otherwise, carry on
destroying by pid.

NOTE that this is not yet completely safe: with ruid == dm_uid, the
device model may be able to kill(-9) the 'reaper' process before the
reaper process can kill it.  Further patches will address this.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---

RFC: This patch _either_ kills by pid, _or_ kills by uid.  Another
approach would be to always kill by pid, then kill by uid if
available.  It wouldn't help the situation if QEMU can manage to call
fork().  But if we can use RLIMIT (or -sandbox or something) to
prevent QEMU from calling fork(), then always killing by pid first
might be safer when we don't have a separate "reaper" uid.

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Anthony Perard <anthony.perard@citrix.com>
---
 tools/libxl/libxl_dm.c       | 118 +++++++++++++++++++++++++++++++++--
 tools/libxl/libxl_internal.h |   3 +-
 2 files changed, 115 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8f7a1d7524..0997865773 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -122,7 +122,7 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
     
     struct passwd *user_base, user_pwbuf;
     int ret;
-    char *user;
+    char *user = NULL, *uid = NULL;
 
     /* Only qemu-upstream can run as a different uid */
     if (b_info->device_model_version != LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN)
@@ -139,6 +139,7 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
                  user);
             return -EINVAL;
         }
+        uid = GCSPRINTF("%ld", (long)user_base->pw_uid);
         goto root_check;
     }
 
@@ -175,9 +176,14 @@ static int libxl__domain_get_device_model_uid(libxl__gc *gc,
         LOGD(DEBUG, guest_domid, "using uid %ld", (long)intended_uid);
         user = GCSPRINTF("%ld:%ld", (long)intended_uid,
                          (long)user_base->pw_gid);
+        uid = GCSPRINTF("%ld", (long)intended_uid);
         goto end_search;
     }
     
+    /* 
+     * NB for QEMU_USER_SHARED, all QEMU will run as the same UID, we
+     * can't kill by uid; therefore don't set uid.
+     */    
     user = LIBXL_QEMU_USER_SHARED;
     ret = userlookup_helper_getpwnam(gc, user, &user_pwbuf, &user_base);
     if (ret < 0)
@@ -202,6 +208,7 @@ root_check:
     
 end_search:
     state->dm_runas = user;
+    state->dm_uid = uid;
     return 0;
 }
                                    
@@ -2382,6 +2389,15 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
 
     const char *dom_path = libxl__xs_get_dompath(gc, domid);
 
+    /* 
+     * If we're stating the dm with a non-root UID, save the UID so
+     * that we can reliably kill it and any subprocesses
+     */
+    if (state->dm_uid)
+        libxl__xs_printf(gc, XBT_NULL,
+                         GCSPRINTF("%s/image/device-model-uid", dom_path),
+                         "%s", state->dm_uid);
+
     if (vnc && vnc->passwd) {
         /* This xenstore key will only be used by qemu-xen-traditionnal.
          * The code to supply vncpasswd to qemu-xen is later. */
@@ -2651,6 +2667,10 @@ out:
     return rc;
 }
 
+static void kill_device_model_uid_cb(libxl__egc *egc,
+                                     libxl__ev_child *destroyer,
+                                     pid_t pid, int status);
+
 void libxl__destroy_device_model(libxl__egc *egc,
                                  libxl__destroy_devicemodel_state *ddms)
 {
@@ -2658,15 +2678,103 @@ void libxl__destroy_device_model(libxl__egc *egc,
     int rc;
     int domid = ddms->domid;
     char *path = DEVICE_MODEL_XS_PATH(gc, LIBXL_TOOLSTACK_DOMID, domid, "");
+    const char * dm_uid_str;
+    uid_t dm_uid;
+    int reaper_pid;
+    int ret;
     
     if (!xs_rm(CTX->xsh, XBT_NULL, path))
         LOGD(ERROR, domid, "xs_rm failed for %s", path);
     
-    /* We should try to destroy the device model anyway. */
-    rc = kill_device_model(gc,
-              GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
+    /* 
+     * We should try to destroy the device model anyway.  Check to see
+     * if we can kill by UID
+     */
+    ret = libxl__xs_read_checked(gc, XBT_NULL,
+                                GCSPRINTF("/local/domain/%d/image/device-model-uid",
+                                           domid),
+                                 &dm_uid_str);
+    if (ret || !dm_uid_str) {
+        /* No uid in xenstore; just kill the pid we have */
+        LOGD(DEBUG, domid, "Didn't find dm UID; destroying by pid");
+        
+        rc = kill_device_model(gc,
+                               GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
+    
+        libxl__qmp_cleanup(gc, domid);
+
+        ddms->callback(egc, ddms, rc);
+        return;
+    }
+
+    /* QEMU has its own uid; kill all processes with that UID */
+    LOGD(DEBUG, domid, "Found DM uid %s, destroying by uid", dm_uid_str);
+    
+    dm_uid = atoi(dm_uid_str);
     
-    libxl__qmp_cleanup(gc, domid);
+    reaper_pid = libxl__ev_child_fork(gc, &ddms->destroyer,
+                                      kill_device_model_uid_cb);
+    if (reaper_pid < 0)
+        ddms->callback(egc, ddms, ERROR_FAIL);
+    
+    if (!reaper_pid) { /* child */
+        const char * call;
+
+        /* 
+         * FIXME: the second uid needs to be distinct to avoid being
+         * killed by a potential rogue process
+         */
+        ret = setresuid(dm_uid, dm_uid, 0);
+        if (ret) {
+            call = "setresuid";
+            goto badchild;
+        }
+
+        /* And kill everyone but me */
+        ret = kill(-1, 9);
+        if (ret) {
+            call = "kill";
+            goto badchild;
+        }
+        _exit(0);
+        
+    badchild:
+        if (errno > 0  && errno < 126) {
+            _exit(errno);
+        } else {
+            LOGED(ERROR, domid,
+                  "Call %s failed (with difficult errno value %d)",
+                  call, errno);
+            _exit(-1);
+        }
+    }
+
+    return;
+}
+
+static void kill_device_model_uid_cb(libxl__egc *egc,
+                                    libxl__ev_child *destroyer,
+                                    pid_t pid, int status)
+{
+    libxl__destroy_devicemodel_state *ddms = CONTAINER_OF(destroyer, *ddms, destroyer);
+    STATE_AO_GC(ddms->ao);
+    int rc;
+
+    if (status) {
+        if (WIFEXITED(status) && WEXITSTATUS(status)<126) {
+            LOGEVD(ERROR, WEXITSTATUS(status), ddms->domid,
+                   "uid-kill failed");
+        } else {
+            libxl_report_child_exitstatus(CTX, XTL_ERROR,
+                                          "async domain destroy", pid, status);
+        }
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    rc = 0;
+
+out:
+    libxl__qmp_cleanup(gc, ddms->domid);
 
     ddms->callback(egc, ddms, rc);
 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 899a86e84b..59eac0662a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1135,7 +1135,7 @@ typedef struct {
     const char * shim_cmdline;
     const char * pv_cmdline;
 
-    char * dm_runas;
+    char * dm_runas,  *dm_uid;
 
     xen_vmemrange_t *vmemranges;
     uint32_t num_vmemranges;
@@ -3706,6 +3706,7 @@ struct libxl__destroy_devicemodel_state {
     uint32_t domid;
     libxl__devicemodel_destroy_cb *callback;
     /* private to implementation */
+    libxl__ev_child destroyer;
 };
 
 struct libxl__destroy_domid_state {
-- 
2.19.1


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

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

[George Dunlap]

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@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 8/9] libxl: Kill QEMU by uid when possible

[George Dunlap]

On 11/23/18 5:15 PM, George Dunlap wrote:
> The privcmd fd that a dm_restrict'ed QEMU has gives it permission to
> one specific domain ID.  This domain ID will probably eventually be
> used again.  It is therefore necessary to make absolutely sure that a
> rogue QEMU process cannot hang around after its domain has exited.
> 
> Killing QEMU by pid is insufficient in this situation, because QEMU
> may be able to fork() to escape killing.  It is surprisingly tricky to
> kill a process which can call fork() without races; the only reliable
> way is to use kill(-1) to kill all processes with a given uid.
> 
> We can use this method only when we're sure that there's only one QEMU
> instance per uid.  Add a dm_uid into the domain_build_state struct,
> and set it in libxl__domain_get_device_model_uid() when it's safe to
> kill by UID.  Store this in xenstore next to device-model-pid.
> 
> On domain destroy, check to see if device-model-uid is present in
> xenstore.  If so, fork off a reaper process, setuid to that uid, and
> do kill(-9) to kill all uids of that type.  Otherwise, carry on
> destroying by pid.
> 
> NOTE that this is not yet completely safe: with ruid == dm_uid, the
> device model may be able to kill(-9) the 'reaper' process before the
> reaper process can kill it.  Further patches will address this.
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

Also...

> +    if (ret || !dm_uid_str) {
> +        /* No uid in xenstore; just kill the pid we have */
> +        LOGD(DEBUG, domid, "Didn't find dm UID; destroying by pid");
> +        
> +        rc = kill_device_model(gc,
> +                               GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
> +    
> +        libxl__qmp_cleanup(gc, domid);
> +
> +        ddms->callback(egc, ddms, rc);
> +        return;

[snip]

> +static void kill_device_model_uid_cb(libxl__egc *egc,
> +                                    libxl__ev_child *destroyer,
> +                                    pid_t pid, int status)
> +{
> +    libxl__destroy_devicemodel_state *ddms = CONTAINER_OF(destroyer, *ddms, destroyer);
> +    STATE_AO_GC(ddms->ao);
> +    int rc;
> +
> +    if (status) {
> +        if (WIFEXITED(status) && WEXITSTATUS(status)<126) {
> +            LOGEVD(ERROR, WEXITSTATUS(status), ddms->domid,
> +                   "uid-kill failed");
> +        } else {
> +            libxl_report_child_exitstatus(CTX, XTL_ERROR,
> +                                          "async domain destroy", pid, status);
> +        }
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +    rc = 0;
> +
> +out:
> +    libxl__qmp_cleanup(gc, ddms->domid);

Does libxl__qmp_cleanup() need to be called after the kill() happens?
If not, we could put this before the kill() and avoid having two call sites.

 -George

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

Re: [PATCH 1/9] libxl: Remove redundant pidpath setting

[Wei Liu]

On Fri, Nov 23, 2018 at 05:14:54PM +0000, George Dunlap wrote:
> This exact same line is duplicated further on without being used or
> modified in between.
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 2/9] libxl: Move dm user determination logic into a helper function

[Wei Liu]

On Fri, Nov 23, 2018 at 05:14:55PM +0000, George Dunlap wrote:
> To reliably kill an untrusted devicemodel, we need to know not only
> its pid, but its uid.  In preparation for this, move the userid
> determination logic into a helper function.
> 
> Create a new field, `dm_runas`, in libxl__domain_build_state to store
> the value during domain creation.
> 
> This change also removes unnecessary duplication of the argument
> construction code.
> 
> No functional change intended.
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

As far as I can tell the code is correct:

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 3/9] libxl: Get rid of support for QEMU_USER_BASE (xen-qemuuser-domidNN)

[Wei Liu]

On Fri, Nov 23, 2018 at 05:14:56PM +0000, George Dunlap wrote:
> QEMU_USER_BASE allows a user to specify the UID to use when running
> the devicemodel for a specific domain number.  Unfortunately, this is
> not really practical: It requires nearly 32,000 entries in
> /etc/passwd.  QEMU_USER_RANGE_BASE is much more practical.
> 
> Remove support for QEMU_USER_BASE.
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

I have no problem with you removing this, but you forgot to change
xl.cfg.pod.5.in.

Wei.

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

Re: [PATCH 4/9] dm_depriv: Describe expected usage of device_model_user parameter

[Wei Liu]

On Fri, Nov 23, 2018 at 05:14:57PM +0000, George Dunlap wrote:
> A number of subsequent patches rely on as-yet undefined behavior for
> what the `device_model_user` parameter does.  Rather than implement it
> incorrectly (or randomly), or remove the feature, describe an expected
> usage for the feature.  Further patches will make decisions based on
> this expected usage.
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Anthony Perard <anthony.perard@citrix.com>
> ---
>  docs/features/qemu-deprivilege.pandoc | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/docs/features/qemu-deprivilege.pandoc b/docs/features/qemu-deprivilege.pandoc
> index f941525189..49b571980e 100644
> --- a/docs/features/qemu-deprivilege.pandoc
> +++ b/docs/features/qemu-deprivilege.pandoc
> @@ -66,6 +66,23 @@ this, create a user named `xen-qemuuser-shared`; for example:
>  
>      adduser --no-create-home --system xen-qemuuser-shared
>  
> +A final way to set up a separate process for qemus is to allocate one
> +UID per VM, and set the UID in the domain config file with the
> +`device_model_user` argument.  For example, suppose you have a VM
> +named `c6-01`.  You might do the following:
> +
> +    adduser --system --no-create-home --group xen-qemuuuser-c6-01
> +
> +And then in your config file, the following line:
> +
> +    device_model_user="xen-qemuuser-c6-01"
> +
> +NOTE: It is important when using `device_model_user` that EACH VM HAVE
> +A SEPARATE UID, and that none of these UIDs map to root.  xl will
> +throw an error a uid maps to zero, but not if multiple VMs have the
> +same uid.  Multiple VMs with the same device model uid will cause
> +problems.
> +

This sounds plausible but I haven't been following the design discussion
closely so I will leave this to Ian and Anthony.

Wei.

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

Re: [PATCH 8/9] libxl: Kill QEMU by uid when possible

[Anthony PERARD]

On Fri, Nov 23, 2018 at 05:18:59PM +0000, George Dunlap wrote:
> On 11/23/18 5:15 PM, George Dunlap wrote:
> Does libxl__qmp_cleanup() need to be called after the kill() happens?
> If not, we could put this before the kill() and avoid having two call sites.

QEMU is supposed to create monitor sockets before the guest is running,
even before it drops priviledge, so I don't think it matter when we `rm`
those qmp sockets. There are only useful to libxl anyway, once libxl
don't needs them they can be removed.

So, before kill() should be fine.

-- 
Anthony PERARD

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

Re: [PATCH 1/9] libxl: Remove redundant pidpath setting

[Ian Jackson]

George Dunlap writes ("[PATCH 1/9] libxl: Remove redundant pidpath setting"):
> This exact same line is duplicated further on without being used or
> modified in between.

I'll take your word for that :-)

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

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

Re: [PATCH 2/9] libxl: Move dm user determination logic into a helper function

[Ian Jackson]

George Dunlap writes ("[PATCH 2/9] libxl: Move dm user determination logic into a helper function"):
> To reliably kill an untrusted devicemodel, we need to know not only
> its pid, but its uid.  In preparation for this, move the userid
> determination logic into a helper function.
> 
> Create a new field, `dm_runas`, in libxl__domain_build_state to store
> the value during domain creation.
> 
> This change also removes unnecessary duplication of the argument
> construction code.
> 
> No functional change intended.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

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

Re: [PATCH 3/9] libxl: Get rid of support for QEMU_USER_BASE (xen-qemuuser-domidNN)

[Ian Jackson]

George Dunlap writes ("[PATCH 3/9] libxl: Get rid of support for QEMU_USER_BASE (xen-qemuuser-domidNN)"):
> QEMU_USER_BASE allows a user to specify the UID to use when running
> the devicemodel for a specific domain number.  Unfortunately, this is
> not really practical: It requires nearly 32,000 entries in
> /etc/passwd.  QEMU_USER_RANGE_BASE is much more practical.

Is the presence of this code causing a problem ?  I am happy to
declare it unsupported.

I provided it because the behaviour of choosing a uid which has *no*
passwd entry might reasonably be regarded as anomalous and
undesirable.

I have used systems with O(32k) password file entries for real
users...

Ian.

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

Re: [PATCH 4/9] dm_depriv: Describe expected usage of device_model_user parameter

[Ian Jackson]

George Dunlap writes ("[PATCH 4/9] dm_depriv: Describe expected usage of device_model_user parameter"):
> A number of subsequent patches rely on as-yet undefined behavior for
> what the `device_model_user` parameter does.  Rather than implement it
> incorrectly (or randomly), or remove the feature, describe an expected
> usage for the feature.  Further patches will make decisions based on
> this expected usage.

This document seems to document a fine feature.  I don't mind the docs
patch being separate but I don't know why you wouldn't add it in the
same patch as you implement it.

So, I guess,

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>


Ian.

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

Re: [PATCH 5/9] libxl: Do root checks once in libxl__domain_get_device_model_uid

[Ian Jackson]

George Dunlap writes ("[PATCH 5/9] libxl: Do root checks once in libxl__domain_get_device_model_uid"):
> At the moment, we check for equivalence to literal "root" before
> deciding whether to add the `runas` command-line option to QEMU.  This
> is unsatisfactory for several reasons.

I was in two minds about the gotos in the earlier version of this
patch.  But here they are getting quite out of hand.

I know that in the hypervisor this kind of thing is tolerated (wrongly
IMO) but can we please not have it here.

This may mean splitting stuff out into a sub-function.  That could be
done some time between "Move dm user determination logic into a helper
function" and this patch I guess.

Sorry,
Ian.

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

Re: [PATCH 6/9] libxl: Move qmp cleanup into devicemodel destroy function

[Ian Jackson]

George Dunlap writes ("[PATCH 6/9] libxl: Move qmp cleanup into devicemodel destroy function"):
> Removing the qmp connection is logically part of the device model
> destruction; having the caller destroy it is a mild layering
> violation.
> 
> Move libxl__qmp_cleanup() into libxl__destroy_device_model().  This
> will make it easier when we make devicemodel destruction asynchronous.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

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

Re: [PATCH 7/9] libxl: Make killing of device model asynchronous

[Ian Jackson]

George Dunlap writes ("[PATCH 7/9] libxl: Make killing of device model asynchronous"):
> Or at least, give it an asynchronous interface so that we can make it
> actually asynchronous in subsequent patches.
> 
> Create state structures and callback function signatures.  Add the
> state structure to libxl__destroy_domid_state.  Break
> libxl__destroy_domid down into two functions.
...
> +/* Used to detroy the device model */
> +_hidden void libxl__destroy_device_model(libxl__egc *egc,
> +                                         libxl__destroy_devicemodel_state *ddms);

I think that comment is rather pointless (but I won't object if you
really want to keep it).  Conversely it would be nice to say somewhere
that ddms->callback may be called reentrantly.

Everything else LGTM.

Thanks,
Ian.

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

Re: [PATCH 8/9] libxl: Kill QEMU by uid when possible

[Ian Jackson]

George Dunlap writes ("[PATCH 8/9] libxl: Kill QEMU by uid when possible"):
> The privcmd fd that a dm_restrict'ed QEMU has gives it permission to
> one specific domain ID.  This domain ID will probably eventually be
> used again.  It is therefore necessary to make absolutely sure that a
> rogue QEMU process cannot hang around after its domain has exited.

Thanks.  This looks roughly right but I have some coding style
quibbles, and I am not convinced the error handling is right.
                                  
> @@ -2382,6 +2389,15 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
>  
>      const char *dom_path = libxl__xs_get_dompath(gc, domid);
>  
> +    /* 
> +     * If we're stating the dm with a non-root UID, save the UID so

Typo for `starting'.

> +     * that we can reliably kill it and any subprocesses
> +     */
...
> +static void kill_device_model_uid_cb(libxl__egc *egc,
> +                                     libxl__ev_child *destroyer,
> +                                     pid_t pid, int status);
> +
>  void libxl__destroy_device_model(libxl__egc *egc,
>                                   libxl__destroy_devicemodel_state *ddms)
>  {
> @@ -2658,15 +2678,103 @@ void libxl__destroy_device_model(libxl__egc *egc,
>      int rc;
>      int domid = ddms->domid;
>      char *path = DEVICE_MODEL_XS_PATH(gc, LIBXL_TOOLSTACK_DOMID, domid, "");
> +    const char * dm_uid_str;
> +    uid_t dm_uid;
> +    int reaper_pid;
> +    int ret;
>      
>      if (!xs_rm(CTX->xsh, XBT_NULL, path))
>          LOGD(ERROR, domid, "xs_rm failed for %s", path);
>      
> -    /* We should try to destroy the device model anyway. */
> -    rc = kill_device_model(gc,
> -              GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
> +    /* 
> +     * We should try to destroy the device model anyway.  Check to see
> +     * if we can kill by UID
> +     */
> +    ret = libxl__xs_read_checked(gc, XBT_NULL,
> +                                GCSPRINTF("/local/domain/%d/image/device-model-uid",
> +                                           domid),
> +                                 &dm_uid_str);

I know this function is bad in its use of `rc' for syscall return but
please don't make it worse by introducing `ret' for what should be
`rc'.  Would you mind adding a pre-patch to change `rc' to `r' and
then you can use `rc' ?

> +    if (ret || !dm_uid_str) {

Shouldn't we fail if libxl__xs_read_checked fails ?
Otherwise we risk leaving fragments of domain behind even if we fail.
(Possibly we should carry on, but accumulate errors in rc.)

> +        /* No uid in xenstore; just kill the pid we have */
> +        LOGD(DEBUG, domid, "Didn't find dm UID; destroying by pid");
> +        
> +        rc = kill_device_model(gc,
> +                               GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
> +    
> +        libxl__qmp_cleanup(gc, domid);
> +
> +        ddms->callback(egc, ddms, rc);
> +        return;

Can you please break out this little exit code fragment

  +        libxl__qmp_cleanup(gc, domid);
  +        ddms->callback(egc, ddms, rc);

into a sub-function ?  It occurs twice and it is easier to reason
about things if the operation has a single exit path.

> +    /* QEMU has its own uid; kill all processes with that UID */
> +    LOGD(DEBUG, domid, "Found DM uid %s, destroying by uid", dm_uid_str);
> +    
> +    dm_uid = atoi(dm_uid_str);

I am tempted to suggest a more robust use of strtoul here but since
this came from our own xenstore node, fine.

> +    reaper_pid = libxl__ev_child_fork(gc, &ddms->destroyer,
> +                                      kill_device_model_uid_cb);
> +    if (reaper_pid < 0)
> +        ddms->callback(egc, ddms, ERROR_FAIL);
> +    }
> +

And, voila!  Didn't you mean to call libxl__qmp_cleanup ?  You're
missing the exit path.  And maybe the call to it should be an `out'
block in this function.

Also, amazingly, you don't return or anything here.  This is only not
a bug because the rest is all the child process.

> +    if (!reaper_pid) { /* child */
> +        const char * call;
> +
> +        /* 
> +         * FIXME: the second uid needs to be distinct to avoid being
> +         * killed by a potential rogue process
> +         */

This is a bit of a funny way of introducing this but fine.

> +        ret = setresuid(dm_uid, dm_uid, 0);
> +        if (ret) {
> +            call = "setresuid";
> +            goto badchild;
> +        }
> +
> +        /* And kill everyone but me */
> +        ret = kill(-1, 9);
> +        if (ret) {
> +            call = "kill";
> +            goto badchild;
> +        }
> +        _exit(0);
> +        
> +    badchild:
> +        if (errno > 0  && errno < 126) {
> +            _exit(errno);
> +        } else {
> +            LOGED(ERROR, domid,
> +                  "Call %s failed (with difficult errno value %d)",
> +                  call, errno);
> +            _exit(-1);
> +        }
> +    }

I don't much like these gotos either.  Maybe a subfunction is called
for.

Furthermore, once again please see CODING_STYLE re conventional
variable names `r' and `rc'·

> +out:
> +    libxl__qmp_cleanup(gc, ddms->domid);

>      ddms->callback(egc, ddms, rc);

Here's that finish fragment again.

> index 899a86e84b..59eac0662a 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1135,7 +1135,7 @@ typedef struct {
>      const char * shim_cmdline;
>      const char * pv_cmdline;
>  
> -    char * dm_runas;
> +    char * dm_runas,  *dm_uid;

It would be nice to drop the spurious space before dm_runas while you
are here, and then presumably the double space before *dm_uid would
not be needed.

Thanks,
Ian.

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

Re: [PATCH 9/9] libxl: Kill QEMU with "reaper" ruid

[Ian Jackson]

George Dunlap writes ("[PATCH 9/9] libxl: Kill QEMU with "reaper" ruid"):
> 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().
...
> +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);

ret should be r I think.

Also I think you need to handle errors properly ?  Ie set and check
errno.

>  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,
...
> -        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;

More gotos!  I doubt this error handling is right.

I'm also not convinced that it is sensible to handle the case where we
have multiple per-domain uids but no reaper uid.  This turns host
configuration errors into systems that are less secure than they
should be in a really obscure way.

> +        /* Try to lock the file */
> +        while (flock(fd, LOCK_EX)) {

CODING_STYLE, no call inside the condition please.


Overall I think this stuff needs a different error handling approach:

 * We should distinguish expected and reasonable configurations, where
   we fall back to less secure methods, from other unexpected
   situations.

 * In other unexpected situations (whether bad host configuration or
   syscall errors or whatever) we should make a best effort to
   destroy as much as we can.

 * But crucially in such situations (i) overall destroy ao should
   return a failure error code (ii) the domain itself should not be
   destroyed in Xen.  This means that `xl destroy' fails, and can be
   repeated after the problem is corrected.

Thanks,
Ian.

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

Re: [PATCH 3/9] libxl: Get rid of support for QEMU_USER_BASE (xen-qemuuser-domidNN)

[George Dunlap]

> On Nov 28, 2018, at 4:34 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
> 
> George Dunlap writes ("[PATCH 3/9] libxl: Get rid of support for QEMU_USER_BASE (xen-qemuuser-domidNN)"):
>> QEMU_USER_BASE allows a user to specify the UID to use when running
>> the devicemodel for a specific domain number.  Unfortunately, this is
>> not really practical: It requires nearly 32,000 entries in
>> /etc/passwd.  QEMU_USER_RANGE_BASE is much more practical.
> 
> Is the presence of this code causing a problem ?  I am happy to
> declare it unsupported.
> 
> I provided it because the behaviour of choosing a uid which has *no*
> passwd entry might reasonably be regarded as anomalous and
> undesirable.
> 
> I have used systems with O(32k) password file entries for real
> users…

With QEMU_USER_BASE, there are 5 qemu uid cases to consider (root, device_model_user, QEMU_USER_RANGE_BASE, QEMU_USER_BASE, and QEMU_USER_SHARED).  Having one less just simplifies the thinking and the logic.  Since I considered QEMU_USER_BASE to be impractical, I thought rather than spend time reasoning about it, I’d just delete it.

I’d personally just as soon leave it out (and add it back in if someone asks for it), but if you think it has value I can leave it in and do the work of thinking about the logic.

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

Re: [PATCH 4/9] dm_depriv: Describe expected usage of device_model_user parameter

[George Dunlap]

> On Nov 28, 2018, at 4:36 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
> 
> George Dunlap writes ("[PATCH 4/9] dm_depriv: Describe expected usage of device_model_user parameter"):
>> A number of subsequent patches rely on as-yet undefined behavior for
>> what the `device_model_user` parameter does.  Rather than implement it
>> incorrectly (or randomly), or remove the feature, describe an expected
>> usage for the feature.  Further patches will make decisions based on
>> this expected usage.
> 
> This document seems to document a fine feature.  I don't mind the docs
> patch being separate but I don't know why you wouldn't add it in the
> same patch as you implement it.

Because the feature is already implemented and working correctly according to the pre-series semantics (AFAICT), but not documented (other than a comment in libxl_types.idl saying, “is not ready for use yet” (which I suppose I should remove while I’m at it)).

When I came to implement kill-by-uid, I came upon the questions: If device_model_user is specified, should I attempt to kill by uid or not?  And, is device_model_user allowed to be “root”?

If we expect device_model_user not to be shared with any other domains, then we can (and probably should) kill by uid; and of course the uid must not be root.  If we expect device_model_user to be shared with other domains, then we cannot kill by uid, and we might consider allowing the uid to be root.

I found no guidance in the code one way or the other; so this change is proposal of what I think is the most useful behavior: Namely, that we make the semantics “must be unique per domain”, rather than “may be shared”.

Given that...

> So, I guess,
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Does the Ack still stand?

 -George

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

Re: [PATCH 5/9] libxl: Do root checks once in libxl__domain_get_device_model_uid

[George Dunlap]

> On Nov 28, 2018, at 4:39 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
> 
> George Dunlap writes ("[PATCH 5/9] libxl: Do root checks once in libxl__domain_get_device_model_uid"):
>> At the moment, we check for equivalence to literal "root" before
>> deciding whether to add the `runas` command-line option to QEMU.  This
>> is unsatisfactory for several reasons.
> 
> I was in two minds about the gotos in the earlier version of this
> patch.  But here they are getting quite out of hand.
> 
> I know that in the hypervisor this kind of thing is tolerated (wrongly
> IMO) but can we please not have it here.

It is a bit strange having to work with one maintianer who thinks a handful of simple gotos is an issue, and another maintainer who thinks having switch case statements appear in the middle of if() { } blocks is perfectly normal. :-)

> This may mean splitting stuff out into a sub-function.  That could be
> done some time between "Move dm user determination logic into a helper
> function" and this patch I guess.

I’m afraid you’re going to have to give me a bit more guidance here: It’s not clear to me what would be split into a sub-function, and how that would make the code easier to follow while avoiding unnecessary code duplication.

Do you propose replacing “goto root_check;” with “root_check(); goto out;” in all locations?  Or something else?

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

Re: [PATCH 8/9] libxl: Kill QEMU by uid when possible

[George Dunlap]

> On Nov 28, 2018, at 4:56 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
> 
> George Dunlap writes ("[PATCH 8/9] libxl: Kill QEMU by uid when possible"):
>> The privcmd fd that a dm_restrict'ed QEMU has gives it permission to
>> one specific domain ID.  This domain ID will probably eventually be
>> used again.  It is therefore necessary to make absolutely sure that a
>> rogue QEMU process cannot hang around after its domain has exited.
> 
> Thanks.  This looks roughly right but I have some coding style
> quibbles, and I am not convinced the error handling is right.
> 
>> @@ -2382,6 +2389,15 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
>> 
>>     const char *dom_path = libxl__xs_get_dompath(gc, domid);
>> 
>> +    /* 
>> +     * If we're stating the dm with a non-root UID, save the UID so
> 
> Typo for `starting'.
> 
>> +     * that we can reliably kill it and any subprocesses
>> +     */
> ...
>> +static void kill_device_model_uid_cb(libxl__egc *egc,
>> +                                     libxl__ev_child *destroyer,
>> +                                     pid_t pid, int status);
>> +
>> void libxl__destroy_device_model(libxl__egc *egc,
>>                                  libxl__destroy_devicemodel_state *ddms)
>> {
>> @@ -2658,15 +2678,103 @@ void libxl__destroy_device_model(libxl__egc *egc,
>>     int rc;
>>     int domid = ddms->domid;
>>     char *path = DEVICE_MODEL_XS_PATH(gc, LIBXL_TOOLSTACK_DOMID, domid, "");
>> +    const char * dm_uid_str;
>> +    uid_t dm_uid;
>> +    int reaper_pid;
>> +    int ret;
>> 
>>     if (!xs_rm(CTX->xsh, XBT_NULL, path))
>>         LOGD(ERROR, domid, "xs_rm failed for %s", path);
>> 
>> -    /* We should try to destroy the device model anyway. */
>> -    rc = kill_device_model(gc,
>> -              GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
>> +    /* 
>> +     * We should try to destroy the device model anyway.  Check to see
>> +     * if we can kill by UID
>> +     */
>> +    ret = libxl__xs_read_checked(gc, XBT_NULL,
>> +                                GCSPRINTF("/local/domain/%d/image/device-model-uid",
>> +                                           domid),
>> +                                 &dm_uid_str);
> 
> I know this function is bad in its use of `rc' for syscall return but
> please don't make it worse by introducing `ret' for what should be
> `rc'.  Would you mind adding a pre-patch to change `rc' to `r' and
> then you can use `rc' ?
> 
>> +    if (ret || !dm_uid_str) {
> 
> Shouldn't we fail if libxl__xs_read_checked fails ?
> Otherwise we risk leaving fragments of domain behind even if we fail.
> (Possibly we should carry on, but accumulate errors in rc.)

Right, I didn’t notice that read_checked filtered out ENOENT (thus a non-zero value for ret indicates a different error).

Not really sure what the best thing would be to do in that case; maybe returning an error immediately is the better option.

> 
>> +        /* No uid in xenstore; just kill the pid we have */
>> +        LOGD(DEBUG, domid, "Didn't find dm UID; destroying by pid");
>> +        
>> +        rc = kill_device_model(gc,
>> +                               GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
>> +    
>> +        libxl__qmp_cleanup(gc, domid);
>> +
>> +        ddms->callback(egc, ddms, rc);
>> +        return;
> 
> Can you please break out this little exit code fragment
> 
>  +        libxl__qmp_cleanup(gc, domid);
>  +        ddms->callback(egc, ddms, rc);
> 
> into a sub-function ?  It occurs twice and it is easier to reason
> about things if the operation has a single exit path.

Actually, I think I’m going to call libxl__qmp_cleanup() before doing the kill()[s] instead.

> 
>> +    /* QEMU has its own uid; kill all processes with that UID */
>> +    LOGD(DEBUG, domid, "Found DM uid %s, destroying by uid", dm_uid_str);
>> +    
>> +    dm_uid = atoi(dm_uid_str);
> 
> I am tempted to suggest a more robust use of strtoul here but since
> this came from our own xenstore node, fine.
> 
>> +    reaper_pid = libxl__ev_child_fork(gc, &ddms->destroyer,
>> +                                      kill_device_model_uid_cb);
>> +    if (reaper_pid < 0)
>> +        ddms->callback(egc, ddms, ERROR_FAIL);
>> +    }
>> +
> 
> And, voila!  Didn't you mean to call libxl__qmp_cleanup ?

Not necessarily — if the fork doesn’t work, the qemu process won’t be killed.  This code was written with the assumption that we wanted qemu dead before removing the socket.

But Anthony tells me that’s not the case, so I’m leaning more towards reorganizing this function to look like the following:

1. libxl__qmp_cleanup()
2. kill_by_pid(pid);
3. if (uid) kill_by_uid(uid);

That is, always kill by pid, and kill by uid if it’s available; but remove the qmp socket first.

>  You're
> missing the exit path.  And maybe the call to it should be an `out'
> block in this function.
> 
> Also, amazingly, you don't return or anything here.  This is only not
> a bug because the rest is all the child process.

Not sure the distinction between these two paragraphs.  But I do think putting an `out:` label that also makes an `assert(rc)` would make this more robust.

>> +    if (!reaper_pid) { /* child */
>> +        const char * call;
>> +
>> +        /* 
>> +         * FIXME: the second uid needs to be distinct to avoid being
>> +         * killed by a potential rogue process
>> +         */
> 
> This is a bit of a funny way of introducing this but fine.

I don’t disagree, but I thought it was better to have a warning than not. I’m open to alternate suggestions.

> 
>> +        ret = setresuid(dm_uid, dm_uid, 0);
>> +        if (ret) {
>> +            call = "setresuid";
>> +            goto badchild;
>> +        }
>> +
>> +        /* And kill everyone but me */
>> +        ret = kill(-1, 9);
>> +        if (ret) {
>> +            call = "kill";
>> +            goto badchild;
>> +        }
>> +        _exit(0);
>> +        
>> +    badchild:
>> +        if (errno > 0  && errno < 126) {
>> +            _exit(errno);
>> +        } else {
>> +            LOGED(ERROR, domid,
>> +                  "Call %s failed (with difficult errno value %d)",
>> +                  call, errno);
>> +            _exit(-1);
>> +        }
>> +    }
> 
> I don't much like these gotos either.  Maybe a subfunction is called
> for.
> 
> Furthermore, once again please see CODING_STYLE re conventional
> variable names `r' and `rc'·
> 
>> +out:
>> +    libxl__qmp_cleanup(gc, ddms->domid);
> 
>>     ddms->callback(egc, ddms, rc);
> 
> Here's that finish fragment again.
> 
>> index 899a86e84b..59eac0662a 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -1135,7 +1135,7 @@ typedef struct {
>>     const char * shim_cmdline;
>>     const char * pv_cmdline;
>> 
>> -    char * dm_runas;
>> +    char * dm_runas,  *dm_uid;
> 
> It would be nice to drop the spurious space before dm_runas while you
> are here, and then presumably the double space before *dm_uid would
> not be needed.

Well presumably it would be better to fix it back in patch 2 instead. :-)

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

Re: [PATCH 9/9] libxl: Kill QEMU with "reaper" ruid

[George Dunlap]

> On Nov 28, 2018, at 5:02 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
> 
> George Dunlap writes ("[PATCH 9/9] libxl: Kill QEMU with "reaper" ruid"):
>> 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().
> ...
>> +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);
> 
> ret should be r I think.
> 
> Also I think you need to handle errors properly ?  Ie set and check
> errno.

Don’t I want to pass up the errno values set by the getpwnam functions?

Although I suppose I should also make sure that the uid I return is not zero...

> 
>> 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,
> ...
>> -        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;
> 
> More gotos!  I doubt this error handling is right.
> 
> I'm also not convinced that it is sensible to handle the case where we
> have multiple per-domain uids but no reaper uid.  This turns host
> configuration errors into systems that are less secure than they
> should be in a really obscure way.

At this point, we have a target_uid but no way of getting a lock for reaper_uid.  We have three options:

1. Don’t kill(-1) at all.
2. Try to  kill(-1) with setresuid(target_uid, target_uid, 0)
3. kill(-1) with setresuid(reaper_uid, target_uid, 0) without holding the lock.

#1 means that a rogue qemu will not be destroyed.

#2 means that there’s a race, whereby sometimes the rogue qemu is destroyed, and sometimes the reaper process is destroyed by the rogue qemu first.

#3 means there’s a race, whereby sometimes everything works fine, sometimes both the rogue qemu and *the reaper process from another domain* is destroyed, and sometimes this reaper process is killed by the reaper process from another domain (leaving the rogue qemu alive).

I think #1 is obviously the best option.

> 
>> +        /* Try to lock the file */
>> +        while (flock(fd, LOCK_EX)) {
> 
> CODING_STYLE, no call inside the condition please.

I c&p’d this from libxl_internal.c:libxl__lock_domain_userdata().

I take it you’re referring to the “ERROR HANDLING” section of CODING_STYLE?  It wasn’t obvious to me that refers to while() loops.

I take it you’d prefer "while(true) { rc = flock(); if(!rc) break; …}” instead?

And if I may make a minor suggestion: This is the second time in this series you’ve said “don’t do X” for fairly common code idioms without giving me guidance as to what you’d like to see instead. 

> Overall I think this stuff needs a different error handling approach:
> 
> * We should distinguish expected and reasonable configurations, where
>   we fall back to less secure methods, from other unexpected
>   situations.
> 
> * In other unexpected situations (whether bad host configuration or
>   syscall errors or whatever) we should make a best effort to
>   destroy as much as we can.

Which is what the code does.

> 
> * But crucially in such situations (i) overall destroy ao should
>   return a failure error code (ii) the domain itself should not be
>   destroyed in Xen.  This means that `xl destroy' fails, and can be
>   repeated after the problem is corrected.

This means that in such a situation, we might successfully kill the devicemodel, but leave a zombie domain lying around.

I suppose that might be the least-bad option, as 1) would be more likely to alert the administrator to fix the configuration error, and 2) the domain would hold the domid, such that any unkilled qemu processes wouldn’t be able to cause mischief on other domains.

Probably some of these principles should be in a comment somewhere.

 -George


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

Re: [PATCH 9/9] libxl: Kill QEMU with "reaper" ruid

[George Dunlap]

> On Nov 28, 2018, at 6:33 PM, George Dunlap <george.dunlap@citrix.com> wrote:
> 
>>> -        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;
>> 
>> More gotos!  I doubt this error handling is right.
>> 
>> I'm also not convinced that it is sensible to handle the case where we
>> have multiple per-domain uids but no reaper uid.  This turns host
>> configuration errors into systems that are less secure than they
>> should be in a really obscure way.
> 
> At this point, we have a target_uid but no way of getting a lock for reaper_uid.  We have three options:
> 
> 1. Don’t kill(-1) at all.
> 2. Try to  kill(-1) with setresuid(target_uid, target_uid, 0)
> 3. kill(-1) with setresuid(reaper_uid, target_uid, 0) without holding the lock.
> 
> #1 means that a rogue qemu will not be destroyed.
> 
> #2 means that there’s a race, whereby sometimes the rogue qemu is destroyed, and sometimes the reaper process is destroyed by the rogue qemu first.
> 
> #3 means there’s a race, whereby sometimes everything works fine, sometimes both the rogue qemu and *the reaper process from another domain* is destroyed, and sometimes this reaper process is killed by the reaper process from another domain (leaving the rogue qemu alive).
> 
> I think #1 is obviously the best option.

Sorry, this should say, ‘I think #2 is the best option’.

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

Re: [PATCH 8/9] libxl: Kill QEMU by uid when possible

[Wei Liu]

On Wed, Nov 28, 2018 at 03:57:58PM +0000, Anthony PERARD wrote:
> On Fri, Nov 23, 2018 at 05:18:59PM +0000, George Dunlap wrote:
> > On 11/23/18 5:15 PM, George Dunlap wrote:
> > Does libxl__qmp_cleanup() need to be called after the kill() happens?
> > If not, we could put this before the kill() and avoid having two call sites.
> 
> QEMU is supposed to create monitor sockets before the guest is running,
> even before it drops priviledge, so I don't think it matter when we `rm`
> those qmp sockets. There are only useful to libxl anyway, once libxl
> don't needs them they can be removed.
> 
> So, before kill() should be fine.

With this scheme, my question is supposedly there is a rogue QEMU, will
it be able to recreate these sockets again by forking so we may end up
having some garbage lying around after it has been killed?

Wei.

> 
> -- 
> Anthony PERARD

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

Re: [PATCH 8/9] libxl: Kill QEMU by uid when possible

[George Dunlap]

> On Nov 29, 2018, at 11:55 AM, Wei Liu <wei.liu2@citrix.com> wrote:
> 
> On Wed, Nov 28, 2018 at 03:57:58PM +0000, Anthony PERARD wrote:
>> On Fri, Nov 23, 2018 at 05:18:59PM +0000, George Dunlap wrote:
>>> On 11/23/18 5:15 PM, George Dunlap wrote:
>>> Does libxl__qmp_cleanup() need to be called after the kill() happens?
>>> If not, we could put this before the kill() and avoid having two call sites.
>> 
>> QEMU is supposed to create monitor sockets before the guest is running,
>> even before it drops priviledge, so I don't think it matter when we `rm`
>> those qmp sockets. There are only useful to libxl anyway, once libxl
>> don't needs them they can be removed.
>> 
>> So, before kill() should be fine.
> 
> With this scheme, my question is supposedly there is a rogue QEMU, will
> it be able to recreate these sockets again by forking so we may end up
> having some garbage lying around after it has been killed?

No; it should at that point be deprivileged and chrooted into a directory owned by root; so it shouldn’t be able to create any new sockets.

It wouldn’t be terribly hard to have a common “exit” to both the kill-by-pid and kill-by-uid paths that did it once, but it would involve adding Yet Another Function; and each additional function makes the code a little bit more difficult to follow.

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

Re: [PATCH 3/9] libxl: Get rid of support for QEMU_USER_BASE (xen-qemuuser-domidNN)

[Ian Jackson]

George Dunlap writes ("Re: [PATCH 3/9] libxl: Get rid of support for QEMU_USER_BASE (xen-qemuuser-domidNN)"):
> I’d personally just as soon leave it out (and add it back in if someone asks for it), but if you think it has value I can leave it in and do the work of thinking about the logic.

OK, fine, I'm sold.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

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

Re: [PATCH 4/9] dm_depriv: Describe expected usage of device_model_user parameter

[Ian Jackson]

George Dunlap writes ("Re: [PATCH 4/9] dm_depriv: Describe expected usage of device_model_user parameter"):
> Because the feature is already implemented and working correctly according to the pre-series semantics (AFAICT), but not documented (other than a comment in libxl_types.idl saying, “is not ready for use yet” (which I suppose I should remove while I’m at it)).

Oh!   Yes, please do remove that comment then.

> Given that...
> 
> > So, I guess,
> > 
> > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Does the Ack still stand?

Yes.

ian.

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

Re: [PATCH 8/9] libxl: Kill QEMU by uid when possible

[Ian Jackson]

George Dunlap writes ("Re: [PATCH 8/9] libxl: Kill QEMU by uid when possible"):
> It wouldn’t be terribly hard to have a common “exit” to both the
> kill-by-pid and kill-by-uid paths that did it once, but it would
> involve adding Yet Another Function; and each additional function
> makes the code a little bit more difficult to follow.

I'm afraid I disagree on this point.

I agree that additional functions should be avoided where they are
needless.  But a single exit path is more important.

Without a single exit path, someone who modifies this code in the
fututure (to add new state, say) will risk altering only one of the
exit paths.

Ian.

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

Re: [PATCH 8/9] libxl: Kill QEMU by uid when possible

[George Dunlap]

> On Nov 29, 2018, at 12:26 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
> 
> George Dunlap writes ("Re: [PATCH 8/9] libxl: Kill QEMU by uid when possible"):
>> It wouldn’t be terribly hard to have a common “exit” to both the
>> kill-by-pid and kill-by-uid paths that did it once, but it would
>> involve adding Yet Another Function; and each additional function
>> makes the code a little bit more difficult to follow.
> 
> I'm afraid I disagree on this point.
> 
> I agree that additional functions should be avoided where they are
> needless.  But a single exit path is more important.
> 
> Without a single exit path, someone who modifies this code in the
> fututure (to add new state, say) will risk altering only one of the
> exit paths.

“Creating an extra function to avoid moving qmp_cleanup earlier” doesn’t sound like a good reason to me. “Creating an extra function to make sure future modifications have only one exit path” sounds like a good reason, though.

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

Re: [PATCH 8/9] libxl: Kill QEMU by uid when possible

[Ian Jackson]

George Dunlap writes ("Re: [PATCH 8/9] libxl: Kill QEMU by uid when possible"):
> Right, I didn’t notice that read_checked filtered out ENOENT (thus a non-zero value for ret indicates a different error).
> 
> Not really sure what the best thing would be to do in that case; maybe returning an error immediately is the better option.

I don't mind whether we do some kind of best effort thing or bail
right away, but in either case this should be regarded as an error.

> But Anthony tells me that’s not the case, so I’m leaning more towards reorganizing this function to look like the following:
> 
> 1. libxl__qmp_cleanup()
> 2. kill_by_pid(pid);
> 3. if (uid) kill_by_uid(uid);
> 
> That is, always kill by pid, and kill by uid if it’s available; but remove the qmp socket first.

I don't have a strong opinion about this, but putting the
libxl__qmp_cleanup after qemu has been killed seems more
straightforward to me (and I've already said there should be a single
exit path so it can go there...)

> > It would be nice to drop the spurious space before dm_runas while you
> > are here, and then presumably the double space before *dm_uid would
> > not be needed.
> 
> Well presumably it would be better to fix it back in patch 2 instead. :-)

Err, yes :-).

Ian.

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

Re: [PATCH 5/9] libxl: Do root checks once in libxl__domain_get_device_model_uid

[Ian Jackson]

George Dunlap writes ("Re: [PATCH 5/9] libxl: Do root checks once in libxl__domain_get_device_model_uid"):
> > On Nov 28, 2018, at 4:39 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
> > I know that in the hypervisor this kind of thing is tolerated (wrongly
> > IMO) but can we please not have it here.
> 
> It is a bit strange having to work with one maintianer who thinks a handful of simple gotos is an issue, and another maintainer who thinks having switch case statements appear in the middle of if() { } blocks is perfectly normal. :-)

Sorry, I missed that.

I think maybe indeed a CODING_STYLE update is called for.

> > This may mean splitting stuff out into a sub-function.  That could be
> > done some time between "Move dm user determination logic into a helper
> > function" and this patch I guess.
> 
> I’m afraid you’re going to have to give me a bit more guidance here: It’s not clear to me what would be split into a sub-function, and how that would make the code easier to follow while avoiding unnecessary code duplication.
> 
> Do you propose replacing “goto root_check;” with “root_check(); goto out;” in all locations?  Or something else?

I am afraid that I have not followed all of the strands of code here.
I am very tempted to reply simply with a reference to Dijkstra [1968],
but:

I think some of your gotos could be refactored into early exits from a
new subfunction.  In any case, I would like the code to be made out of
block structured pieces such as for and while loops (perhaps with
early exits like break and continue), if statements, switch
statements, conditionals, and so on.

The `goto out' paradigm (which should be encouraged) is IMO an
exception to this.  But then there should be only one `out' label,
or maybe exceptionally both `out' and `err'.

(I also deprecate the `retry' labels found in some places to deal with
xenstore.)

HTH.

Thanks,
Ian.

References:
  Dijkstra [1968]   _Go To Statement Considered Harmful_
                    March 1968
                    Comm. ACM, 11 (3), 147-148
                    doi:10.1145/362929.362947

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

Re: [PATCH 9/9] libxl: Kill QEMU with "reaper" ruid

[Ian Jackson]

George Dunlap writes ("Re: [PATCH 9/9] libxl: Kill QEMU with "reaper" ruid"):
> On Nov 28, 2018, at 5:02 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
> > Also I think you need to handle errors properly ?  Ie set and check
> > errno.
> 
> Don’t I want to pass up the errno values set by the getpwnam functions?

By `set' I meant zero beforehand.  See the manpage for getpw*.

> Although I suppose I should also make sure that the uid I return is not zero...

Yes.

> > I'm also not convinced that it is sensible to handle the case where we
> > have multiple per-domain uids but no reaper uid.  This turns host
> > configuration errors into systems that are less secure than they
> > should be in a really obscure way.
> 
> At this point, we have a target_uid but no way of getting a lock for reaper_uid.  We have three options:
> 
> 1. Don’t kill(-1) at all.
> 2. Try to  kill(-1) with setresuid(target_uid, target_uid, 0)
> 3. kill(-1) with setresuid(reaper_uid, target_uid, 0) without holding the lock.
> 
> #1 means that a rogue qemu will not be destroyed.
> 
> #2 means that there’s a race, whereby sometimes the rogue qemu is destroyed, and sometimes the reaper process is destroyed by the rogue qemu first.
> 
> #3 means there’s a race, whereby sometimes everything works fine, sometimes both the rogue qemu and *the reaper process from another domain* is destroyed, and sometimes this reaper process is killed by the reaper process from another domain (leaving the rogue qemu alive).
> 
> I think [#2] is obviously the best option.

Yes, but it still needs to count as an error.

> > CODING_STYLE, no call inside the condition please.
> 
> I c&p’d this from libxl_internal.c:libxl__lock_domain_userdata().
> I take it you’re referring to the “ERROR HANDLING” section of CODING_STYLE?  It wasn’t obvious to me that refers to while() loops.
> I take it you’d prefer "while(true) { rc = flock(); if(!rc) break; …}” instead?

Except that the return value from flock() is `r' according to
CODING_STYLE.  (And I'm not sure `true' is in scope and anyway
personally I prefer `for (;;)' but that's a matter of taste
so whatever.)

> And if I may make a minor suggestion: This is the second time in this series you’ve said “don’t do X” for fairly common code idioms without giving me guidance as to what you’d like to see instead. 

I'm sorry that `no call inside the condition please' was not clear
enough.  I mean that the flock call should be outside any condition,
because it is a call that might fail, and that consequently the loop
termination will have to be done with an early loop exit using `break'
rather than a condition.  (Since saving the return value so that it
could be used in a while() would be perverse.)

> > * But crucially in such situations (i) overall destroy ao should
> >   return a failure error code (ii) the domain itself should not be
> >   destroyed in Xen.  This means that `xl destroy' fails, and can be
> >   repeated after the problem is corrected.
> 
> This means that in such a situation, we might successfully kill the
> devicemodel, but leave a zombie domain lying around.

Yes, precisely.

> I suppose that might be the least-bad option, as 1) would be more
> likely to alert the administrator to fix the configuration error,
> and 2) the domain would hold the domid, such that any unkilled qemu
> processes wouldn’t be able to cause mischief on other domains.

Precisely.

In more formal terms, they prevent the system getting into a
completely uncontrolled and forbidden state, namely a state where
there is possibly a qemu and maybe other stuff hanging about, which is
not visible in xl and may cause future mischief.

Since we cannot destroy everything at once, our system model must
include a `half destroyed' state.  In that state the domain must still
show up in `xl'.  We have chosen to `hang everything off' the Xen list
of active domains, which means that we mustn't remove a domain from
Xen until we have *successfully* destroyed all its other stuff.

> Probably some of these principles should be in a comment somewhere.

Yes.  I think they apply to all of domain destruction in libxl.

Ian.

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

Re: [PATCH 8/9] libxl: Kill QEMU by uid when possible

[Ian Jackson]

George Dunlap writes ("Re: [PATCH 8/9] libxl: Kill QEMU by uid when possible"):
> > On Nov 29, 2018, at 12:26 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
> > George Dunlap writes ("Re: [PATCH 8/9] libxl: Kill QEMU by uid when possible"):
> >> It wouldn’t be terribly hard to have a common “exit” to both the
> >> kill-by-pid and kill-by-uid paths that did it once, but it would
> >> involve adding Yet Another Function; and each additional function
> >> makes the code a little bit more difficult to follow.
> > 
> > I'm afraid I disagree on this point.
> > 
> > I agree that additional functions should be avoided where they are
> > needless.  But a single exit path is more important.
> > 
> > Without a single exit path, someone who modifies this code in the
> > fututure (to add new state, say) will risk altering only one of the
> > exit paths.
> 
> “Creating an extra function to avoid moving qmp_cleanup earlier”
> doesn’t sound like a good reason to me. “Creating an extra function
> to make sure future modifications have only one exit path” sounds
> like a good reason, though.

OK, good, I think we are in agreement on this point.

Ian.

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

Re: [PATCH 5/9] libxl: Do root checks once in libxl__domain_get_device_model_uid

[George Dunlap]

> On Nov 29, 2018, at 5:09 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
> 
> George Dunlap writes ("Re: [PATCH 5/9] libxl: Do root checks once in libxl__domain_get_device_model_uid"):
>>> On Nov 28, 2018, at 4:39 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
>>> I know that in the hypervisor this kind of thing is tolerated (wrongly
>>> IMO) but can we please not have it here.
>> 
>> It is a bit strange having to work with one maintianer who thinks a handful of simple gotos is an issue, and another maintainer who thinks having switch case statements appear in the middle of if() { } blocks is perfectly normal. :-)
> 
> Sorry, I missed that.
> 
> I think maybe indeed a CODING_STYLE update is called for.

…not sure what you mean; I’m referring to a hypervisor maintainer who regularly generates code like:

switch() {
case A:
  if(blah) {
case B:
     foo();
  }
}

I’m not complaining, I’m just slightly amused at the difference. :-)

> 
>>> This may mean splitting stuff out into a sub-function.  That could be
>>> done some time between "Move dm user determination logic into a helper
>>> function" and this patch I guess.
>> 
>> I’m afraid you’re going to have to give me a bit more guidance here: It’s not clear to me what would be split into a sub-function, and how that would make the code easier to follow while avoiding unnecessary code duplication.
>> 
>> Do you propose replacing “goto root_check;” with “root_check(); goto out;” in all locations?  Or something else?
> 
> I am afraid that I have not followed all of the strands of code here.
> I am very tempted to reply simply with a reference to Dijkstra [1968],
> but:

I read that paper; it was nearly two decades ago now, but my memory was that most of his objections are made moot by languages such as C, particularly if your compiler is configured to throw an error if a label is unused.

> 
> I think some of your gotos could be refactored into early exits from a
> new subfunction.  In any case, I would like the code to be made out of
> block structured pieces such as for and while loops (perhaps with
> early exits like break and continue), if statements, switch
> statements, conditionals, and so on.

So the situation is, we have three “found a UID” cases (device_model_uid set, QEMU_USER_RANGE_BASE uid present, QEMU_USER_SHARED uid present).  In all three cases, we want to make sure the final UID is not root, but in two of them (device_model_uid and QEMU_USER_SHARED) we need to check user_base->pw_uid, and in the other one (QEMU_USER_RANGE_BASE), we need to check our calculated uid value (intended_uid).

Also, at some point in the development of the series, it wasn’t clear whether device_model_uid was allowed to be root (which is how patch 4 came about).

So it sounds like instead of this:

8<---
int a() {
  if (c1) {
    /* do something */
    goto check_1;
  }

  if (c2) {
    /* do something */
   goto out;
  }

  if (c3) {
    /* do something */
   goto check;
  }

  /* Fail */

check:
  /* do check, maybe fail */

out:
  /* Do common “out” things */
}
—>8

You want this:
8<—

int a() {
  if (c1) {
    /* do something */
    if(check())
      return error;
    goto out;
  }

  if (c2) {
    /* do something */
   goto out;
  } 

  if (c3) {
    /* do something */
    if(check())
      return error;
  }

  /* Fail */

out:
  /* Do common “out” things */
}

int check() {
  /* do check */
}
—>8

All I can say is, the second one seems less clear and more repetitive to me.  It contains many more control flow changes for each check(), rather than one (since a function call is a type of goto), and you have to duplicate handling the error condition in each of the return sites.

But anyway, I’ll see what I can do about re-arranging things to have only a single `out:` label (getting rid of the style-non-conformant `return`s all over the function as well).  I think it should be possible without an extra function.

 -George

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

Re: [PATCH 2/9] libxl: Move dm user determination logic into a helper function

[George Dunlap]

> On Nov 23, 2018, at 5:14 PM, George Dunlap <george.dunlap@citrix.com> wrote:
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index e498435e16..a370de54ed 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1135,6 +1135,8 @@ typedef struct {
>     const char * shim_cmdline;
>     const char * pv_cmdline;
> 
> +    char * dm_runas;

I’ve removed the space both here, and in the above char * elements. I’ll retain both of your Acks unless I hear otherwise.

 -George

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

Re: [PATCH 3/9] libxl: Get rid of support for QEMU_USER_BASE (xen-qemuuser-domidNN)

[George Dunlap]

> On Nov 28, 2018, at 3:32 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> 
> On Fri, Nov 23, 2018 at 05:14:56PM +0000, George Dunlap wrote:
>> QEMU_USER_BASE allows a user to specify the UID to use when running
>> the devicemodel for a specific domain number.  Unfortunately, this is
>> not really practical: It requires nearly 32,000 entries in
>> /etc/passwd.  QEMU_USER_RANGE_BASE is much more practical.
>> 
>> Remove support for QEMU_USER_BASE.
>> 
>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> 
> I have no problem with you removing this, but you forgot to change
> xl.cfg.pod.5.in.

Hmm… it seems what I *really* forgot to do was remove that entire section and point to the feature doc instead.  It’s quite out of date now (e.g., cdrom insert should work I think, qemu is chrooted so can’t read world-readable files, &c).  With your permission I’ll put fixing that up properly on a to-do list (to be done before 4.12 release at a minimum).

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

Re: [PATCH 7/9] libxl: Make killing of device model asynchronous

[George Dunlap]

> On Nov 28, 2018, at 4:43 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
> 
> George Dunlap writes ("[PATCH 7/9] libxl: Make killing of device model asynchronous"):
>> Or at least, give it an asynchronous interface so that we can make it
>> actually asynchronous in subsequent patches.
>> 
>> Create state structures and callback function signatures.  Add the
>> state structure to libxl__destroy_domid_state.  Break
>> libxl__destroy_domid down into two functions.
> ...
>> +/* Used to detroy the device model */
>> +_hidden void libxl__destroy_device_model(libxl__egc *egc,
>> +                                         libxl__destroy_devicemodel_state *ddms);
> 
> I think that comment is rather pointless (but I won't object if you
> really want to keep it).  

Yes; that comment looks rather vestigal.

> Conversely it would be nice to say somewhere
> that ddms->callback may be called reentrantly.

What do you mean by reentrantly?  That libxl__destroy_device_model() may end up calling it directly (on this execution stack), rather than arranging for it to be called by someone else after returning?

 -George


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

Re: [PATCH 7/9] libxl: Make killing of device model asynchronous

[George Dunlap]

> On Nov 30, 2018, at 4:12 PM, George Dunlap <george.dunlap@citrix.com> wrote:
> 
> 
> 
>> On Nov 28, 2018, at 4:43 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
>> 
>> George Dunlap writes ("[PATCH 7/9] libxl: Make killing of device model asynchronous"):
>>> Or at least, give it an asynchronous interface so that we can make it
>>> actually asynchronous in subsequent patches.
>>> 
>>> Create state structures and callback function signatures.  Add the
>>> state structure to libxl__destroy_domid_state.  Break
>>> libxl__destroy_domid down into two functions.
>> ...
>>> +/* Used to detroy the device model */
>>> +_hidden void libxl__destroy_device_model(libxl__egc *egc,
>>> +                                         libxl__destroy_devicemodel_state *ddms);
>> 
>> I think that comment is rather pointless (but I won't object if you
>> really want to keep it).  
> 
> Yes; that comment looks rather vestigal.

Oh, I see; I was following suit with the code around it:

/* Used to destroy a domain with the passed id (it doesn't check for stubs) */
_hidden void libxl__destroy_domid(libxl__egc *egc,
                                  libxl__destroy_domid_state *dis);

/* Used to detroy the device model */
_hidden void libxl__destroy_device_model(libxl__egc *egc,
                                         libxl__destroy_devicemodel_state *ddms);

/* Entry point for devices destruction */
_hidden void libxl__devices_destroy(libxl__egc *egc,
                                    libxl__devices_remove_state *drs);

It looks cleaner to me to have *something* there than not, just to visually make it clear that it has nothing to do with the previous function.

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

Re: [PATCH 7/9] libxl: Make killing of device model asynchronous

[Ian Jackson]

George Dunlap writes ("Re: [PATCH 7/9] libxl: Make killing of device model asynchronous"):
> On Nov 28, 2018, at 4:43 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
> > Conversely it would be nice to say somewhere
> > that ddms->callback may be called reentrantly.
> 
> What do you mean by reentrantly?  That libxl__destroy_device_model() may end up calling it directly (on this execution stack), rather than arranging for it to be called by someone else after returning?

Precisely.

This kind of callback can, in some situations, be a hazard for the
caller, because it might result in data structures that the caller is
about to rely on vanishing.

It is conventional in libxl to return after setting up callbacks so
this generally doesn't arise, but adding a note to functions with this
property is a good idea.

Ian.

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

Re: [PATCH 7/9] libxl: Make killing of device model asynchronous

[Ian Jackson]

George Dunlap writes ("Re: [PATCH 7/9] libxl: Make killing of device model asynchronous"):
> It looks cleaner to me to have *something* there than not, just to visually make it clear that it has nothing to do with the previous function.

That's OK by me.

Ian.

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

Re: [PATCH 8/9] libxl: Kill QEMU by uid when possible

[George Dunlap]

> On Nov 28, 2018, at 4:56 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
> 
>>     if (!xs_rm(CTX->xsh, XBT_NULL, path))
>>         LOGD(ERROR, domid, "xs_rm failed for %s", path);
>> 
>> -    /* We should try to destroy the device model anyway. */
>> -    rc = kill_device_model(gc,
>> -              GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
>> +    /* 
>> +     * We should try to destroy the device model anyway.  Check to see
>> +     * if we can kill by UID
>> +     */
>> +    ret = libxl__xs_read_checked(gc, XBT_NULL,
>> +                                GCSPRINTF("/local/domain/%d/image/device-model-uid",
>> +                                           domid),
>> +                                 &dm_uid_str);
> 
> I know this function is bad in its use of `rc' for syscall return but
> please don't make it worse by introducing `ret' for what should be
> `rc'.  Would you mind adding a pre-patch to change `rc' to `r' and
> then you can use `rc’ ?

Actually, it looks like kill_device_model() returns a libxl error value.  So we should use the existing rc for both kill_device_model() and xs_read_checked(), and introduce `r` to use for setresuid() &c

 -George

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

Re: [PATCH 3/9] libxl: Get rid of support for QEMU_USER_BASE (xen-qemuuser-domidNN)

[Wei Liu]

On Fri, Nov 30, 2018 at 12:53:20PM +0000, George Dunlap wrote:
> 
> 
> > On Nov 28, 2018, at 3:32 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> > 
> > On Fri, Nov 23, 2018 at 05:14:56PM +0000, George Dunlap wrote:
> >> QEMU_USER_BASE allows a user to specify the UID to use when running
> >> the devicemodel for a specific domain number.  Unfortunately, this is
> >> not really practical: It requires nearly 32,000 entries in
> >> /etc/passwd.  QEMU_USER_RANGE_BASE is much more practical.
> >> 
> >> Remove support for QEMU_USER_BASE.
> >> 
> >> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> > 
> > I have no problem with you removing this, but you forgot to change
> > xl.cfg.pod.5.in.
> 
> Hmm… it seems what I *really* forgot to do was remove that entire
> section and point to the feature doc instead.  It’s quite out of date
> now (e.g., cdrom insert should work I think, qemu is chrooted so can’t
> read world-readable files, &c).  With your permission I’ll put fixing
> that up properly on a to-do list (to be done before 4.12 release at a
> minimum).

Sure.

Wei.

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