|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] [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.
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@xxxxxxxxxx>
---
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@xxxxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
CC: Anthony Perard <anthony.perard@xxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |