[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Xen-devel] [PATCH v4 01/11] 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.

While here, clean up some minor CODING_STYLE infractions (space
between * and variable name).

No functional change intended.

While here, delete some trailing whitespace.

Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
---
v2:
- Remove unnecessary space between * and dm_runas
- Additional code clean-up
- Delete trailing whitespace

CC: Ian Jackson <ian.jackson@xxxxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
CC: Anthony Perard <anthony.perard@xxxxxxxxxx>
---
 tools/libxl/libxl_dm.c       | 260 +++++++++++++++++++----------------
 tools/libxl/libxl_internal.h |  22 +--
 2 files changed, 151 insertions(+), 131 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 5698fe8af3..bbcbc94b6c 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);
@@ -1414,10 +1489,10 @@ static int libxl__build_device_model_args_new(libxl__gc 
*gc,
         char *chroot_dir = GCSPRINTF("%s/qemu-root-%d",
                                       libxl__run_dir_path(), guest_domid);
         int r;
-        
+
         flexarray_append(dm_args, "-xen-domid-restrict");
 
-        /* 
+        /*
          * Run QEMU in a chroot at XEN_RUN_DIR/qemu-root-<domid>
          *
          * There is no library function to do the equivalent of `rm
@@ -1425,7 +1500,7 @@ static int libxl__build_device_model_args_new(libxl__gc 
*gc,
          * able to write any files, as the chroot would be owned by
          * root, but it would be running as an unprivileged process.
          * So in theory, old chroots should always be empty.
-         * 
+         *
          * rmdir the directory before attempting to create
          * it; if it returns anything other than ENOENT, fail domain
          * creation.
@@ -1436,7 +1511,7 @@ static int libxl__build_device_model_args_new(libxl__gc 
*gc,
                   "failed to remove existing chroot dir %s", chroot_dir);
             return ERROR_FAIL;
         }
-        
+
         for (;;) {
             r = mkdir(chroot_dir, 0000);
             if (!r)
@@ -1538,7 +1613,7 @@ static int libxl__build_device_model_args_new(libxl__gc 
*gc,
                 continue;
             }
 
-            /* 
+            /*
              * If qemu isn't doing the interpreting, the parameter is
              * always raw
              */
@@ -1563,7 +1638,7 @@ static int libxl__build_device_model_args_new(libxl__gc 
*gc,
                     continue;
                 }
 
-                /* 
+                /*
                  * We can't call libxl__blktap_devpath from
                  * libxl__device_disk_find_local_path for now because
                  * the bootloader is called before the disks are set
@@ -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..c4a43bd0b7 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -332,7 +332,7 @@ struct libxl__ev_evtchn {
 typedef struct libxl__ev_watch_slot {
     LIBXL_SLIST_ENTRY(struct libxl__ev_watch_slot) empty;
 } libxl__ev_watch_slot;
-    
+
 _hidden libxl__ev_xswatch *libxl__watch_slot_contents(libxl__gc *gc,
                                                       int slotnum);
 
@@ -484,7 +484,7 @@ struct libxl__ctx {
         death_list /* sorted by domid */,
         death_reported;
     libxl__ev_xswatch death_watch;
-    
+
     LIBXL_LIST_HEAD(, libxl_evgen_disk_eject) disk_eject_evgens;
 
     const libxl_childproc_hooks *childproc_hooks;
@@ -1131,9 +1131,11 @@ typedef struct {
 
     libxl__file_reference pv_kernel;
     libxl__file_reference pv_ramdisk;
-    const char * shim_path;
-    const char * shim_cmdline;
-    const char * pv_cmdline;
+    const char *shim_path;
+    const char *shim_cmdline;
+    const char *pv_cmdline;
+
+    char *dm_runas;
 
     xen_vmemrange_t *vmemranges;
     uint32_t num_vmemranges;
@@ -1471,7 +1473,7 @@ _hidden void libxl__spawn_init(libxl__spawn_state*);
  *
  * what: string describing the spawned process, used for logging
  *
- * Logs errors.  A copy of "what" is taken. 
+ * Logs errors.  A copy of "what" is taken.
  * Return values:
  *  < 0   error, *spawn is now Idle and need not be detached
  *   +1   caller is the parent, *spawn is Attached and must be detached
@@ -2750,10 +2752,10 @@ static inline void 
libxl__device_disk_local_init(libxl__disk_local_state *dls)
     dls->rc = 0;
 }
 
-/* 
+/*
  * See if we can find a way to access a disk locally
  */
-_hidden char * libxl__device_disk_find_local_path(libxl__gc *gc, 
+_hidden char * libxl__device_disk_find_local_path(libxl__gc *gc,
                                                   libxl_domid guest_domid,
                                                   const libxl_device_disk 
*disk,
                                                   bool qdisk_direct);
@@ -3774,7 +3776,7 @@ struct libxl__dm_spawn_state {
 
 _hidden void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state*);
 
-/* 
+/*
  * Called after forking but before executing the local devicemodel.
  */
 _hidden int libxl__local_dm_preexec_restrict(libxl__gc *gc);
@@ -3963,7 +3965,7 @@ _hidden void libxl__remus_restore_setup(libxl__egc *egc,
  */
 #define GCNEW_ARRAY(var, nmemb)                                 \
     ((var) = libxl__calloc((gc), (nmemb), sizeof(*(var))))
-    
+
 /*
  * Expression statement  <type> *GCREALLOC_ARRAY(<type> *var, size_t nmemb);
  * Uses                  libxl__gc *gc;
-- 
2.19.2


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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.