|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] [PATCH v2 05/10] 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.
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.
One side effect of this is to check whether device_model_user exists
before passing it to qemu, resulting in better error reporting.
While we're here:
- Refactor the function to use the "goto out" idiom in most cases
- Use 'rc' rather than 'ret', in line with CODING_STYLE
- Change the error returned in the "uid collision" case to
ERROR_DEVICE_EXISTS
Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
---
v2:
- Refactor to use `out` rather than multiple labels
- Only check for root once
- Use 'out' rather than direct returns for errors (only use direct returns
for early `succeed-without-setting-runas` paths)
- Use `rc` rather than `ret` to more closely align with CODING_STYLE
- Fill out comments about the cases we're handling
- Return ERROR_DEVICE_EXISTS rather than ERROR_FAIL if there's another
username that maps to our calculated uid
- Report an error if the specified device_model_user doesn't exist
CC: Ian Jackson <ian.jackson@xxxxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
---
tools/libxl/libxl_dm.c | 86 +++++++++++++++++++++++++++++++-----------
1 file changed, 65 insertions(+), 21 deletions(-)
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 959fa0f46c..7ff3e3160a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -126,65 +126,109 @@ static int libxl__domain_get_device_model_uid(libxl__gc
*gc,
const libxl_domain_build_info *b_info = &dmss->guest_config->b_info;
struct passwd *user_base, user_pwbuf;
- int ret;
+ int rc;
char *user;
+ uid_t intended_uid;
/* Only qemu-upstream can run as a different uid */
if (b_info->device_model_version != LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN)
return 0;
+ /*
+ * If device_model_user is present, set `-runas` even if
+ * dm_restrict isn't in use
+ */
user = b_info->device_model_user;
- if (user)
- goto end_search;
+ if (user) {
+ rc = userlookup_helper_getpwnam(gc, user, &user_pwbuf, &user_base);
+ if (rc)
+ goto out;
+
+ if (!user_base) {
+ LOGD(ERROR, guest_domid, "Couldn't find device_model_user %s",
+ user);
+ rc = ERROR_INVAL;
+ } else
+ intended_uid = user_base->pw_uid;
+ goto out;
+ }
+
+ /*
+ * If dm_restrict isn't set, and we don't have a specified user, don't
+ * bother setting a `-runas` parameter.
+ */
if (!libxl_defbool_val(b_info->dm_restrict)) {
LOGD(DEBUG, guest_domid,
"dm_restrict disabled, starting QEMU as root");
return 0;
}
- ret = userlookup_helper_getpwnam(gc, LIBXL_QEMU_USER_RANGE_BASE,
+ /*
+ * dm_restrict is set, but device_model_user isn't set; look for
+ * QEMU_USER_BASE_RANGE
+ */
+ rc = userlookup_helper_getpwnam(gc, LIBXL_QEMU_USER_RANGE_BASE,
&user_pwbuf, &user_base);
- if (ret < 0)
- return ret;
+ if (rc)
+ goto out;
if (user_base) {
struct passwd *user_clash, user_clash_pwbuf;
- uid_t intended_uid = user_base->pw_uid + guest_domid;
- ret = userlookup_helper_getpwuid(gc, intended_uid,
+
+ intended_uid = user_base->pw_uid + guest_domid;
+ rc = userlookup_helper_getpwuid(gc, intended_uid,
&user_clash_pwbuf, &user_clash);
- if (ret < 0)
- return ret;
+ if (rc < 0)
+ goto out;
if (user_clash) {
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;
+ rc = ERROR_DEVICE_EXISTS;
+ goto out;
}
+
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;
+ goto out;
}
+ /*
+ * We couldn't find QEMU_USER_BASE_RANGE; look for QEMU_USER_SHARED
+ */
user = LIBXL_QEMU_USER_SHARED;
- ret = userlookup_helper_getpwnam(gc, user, &user_pwbuf, &user_base);
- if (ret < 0)
- return ret;
+ rc = userlookup_helper_getpwnam(gc, user, &user_pwbuf, &user_base);
+ if (rc < 0)
+ goto out;
if (user_base) {
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;
+ intended_uid = user_base->pw_uid;
+ goto out;
}
+ /*
+ * dm_depriv is set, but we can't find a non-root uid to run as;
+ * fail domain creation
+ */
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;
+ rc = ERROR_INVAL;
-end_search:
- state->dm_runas = user;
- return 0;
+out:
+ if (!rc) {
+ if (intended_uid == 0) {
+ LOGD(ERROR, guest_domid, "intended_uid is 0 (root)!");
+ return ERROR_INVAL;
+ }
+
+ state->dm_runas = user;
+ }
+
+ return rc;
}
const char *libxl__domain_device_model(libxl__gc *gc,
@@ -1757,7 +1801,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.2
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |