|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 2/3] libxl: do not assume Dom0 backend while listing disks and nics
On 08/05/13 05:39, Marek Marczykowski wrote:
> One more place where code assumed that all backends are in dom0. List
> devices in domain device/ tree, instead of backend/ of dom0.
> Additionally fix libxl_devid_to_device_{nic,disk} to fill backend_domid
> properly.
>
> Changes in v2:
> - restructure *_from_xs_be to *_from_xs_fe, which take care of getting
> backend path and backend domid (with libxl__get_backend_from_xs_fe
> helper)
> - sanity checks on frontend-controlled entries
>
> Signed-off-by: Marek Marczykowski <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> ---
> tools/libxl/libxl.c | 162
> ++++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 127 insertions(+), 35 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 5b9a3df..cb4a2a8 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1739,6 +1739,83 @@ static int libxl__resolve_domid(libxl__gc *gc, const
> char *name,
> return libxl_domain_qualifier_to_domid(CTX, name, domid);
> }
>
> +/* Get backend path from frontend, with some sanity checks:
> + * - if both points to each other
> + * - if backend is owned by backend domid
> + *
> + * Returns backend domid or -1 on error. If no error reported, be_path will
> be
> + * filled with backend path.
> + */
> +static int libxl__get_backend_from_xs_fe(libxl__gc *gc, const char *fe_path,
> + const char **be_path_out)
> +{
> + int be_domid;
> + const char *be_domid_str;
> + const char *be_path;
> + const char *fe_path_from_be;
> + char *be_dompath;
> + char *be_base_path;
> + struct xs_permissions *be_perms;
> + unsigned int be_perms_count;
> +
> + if (libxl__xs_read_checked(gc, XBT_NULL,
> + GCSPRINTF("%s/backend-id", fe_path), &be_domid_str)) {
> + LOG(ERROR, "Missing xenstore node %s/backend-id", fe_path);
> + return -1;
It will be better to propagate the error returned from
libxl__xs_read_checked:
rc = libxl__xs_read_checked(gc, XBT_NULL,
GCSPRINTF("%s/backend-id", fe_path), &be_domid_str);
if (rc) {
LOG(ERROR, "Missing xenstore node %s/backend-id", fe_path);
return rc;
}
Or better, have a fail label that could be used when an error is detected:
rc = libxl__xs_read_checked(gc, XBT_NULL,
GCSPRINTF("%s/backend-id", fe_path), &be_domid_str);
if (rc) {
LOG(ERROR, "Missing xenstore node %s/backend-id", fe_path);
goto fail;
}
...
return be_domid;
fail:
assert(rc);
return rc;
Also, the -1s below should be replaced with ERROR_FAIL or some more
appropriate error value.
> + }
> + be_domid = strtoul(be_domid_str, NULL, 10);
> +
> + if (libxl__xs_read_checked(gc, XBT_NULL,
> + GCSPRINTF("%s/backend", fe_path), &be_path)) {
> + LOG(ERROR, "Missing xenstore node %s/backend", fe_path);
> + return -1;
> + }
> +
> + if (!(be_dompath = libxl__xs_get_dompath(gc, be_domid))) {
> + LOG(ERROR, "Failed to get dompath for domain %d", be_domid);
> + return -1;
> + }
> +
> + be_base_path = GCSPRINTF("%s/backend/", be_dompath);
> + if (strncmp(be_path, be_base_path, strlen(be_base_path))) {
> + /* possible malicious frontend action */
> + LOG(ERROR, "Backend xenstore path %s not withing %s",
> + be_path,
> + be_base_path);
> + return -1;
> + }
> +
> + be_perms = libxl__xs_get_permissions(gc, XBT_NULL, be_path,
> &be_perms_count);
> + if (!be_perms) {
> + LOG(ERROR, "Failed to get %s path permissions", be_path);
> + return -1;
> + }
> +
> + if (be_perms[0].id != be_domid) {
> + /* possible malicious frontend action */
> + /* perhaps LIBXL_TOOLSTACK_DOMID should be also allowed? */
> + LOG(ERROR, "Backend path %s not owned by backend domain (owner:
> %d)", be_path, be_perms[0].id);
> + return -1;
> + }
> +
> + if (libxl__xs_read_checked(gc, XBT_NULL,
> + GCSPRINTF("%s/frontend", be_path), &fe_path_from_be)) {
> + LOG(ERROR, "Missing xenstore node %s/frontend", be_path);
> + return -1;
> + }
> +
> + if (strcmp(fe_path, fe_path_from_be)) {
> + /* possible malicious frontend action */
> + LOG(ERROR, "Frontend path from backend (%s) doesn't match original "
> + "frontend path (%s)", fe_path_from_be, fe_path);
> + return -1;
> + }
> +
> + *be_path_out = be_path;
> +
> + return be_domid;
> +}
> +
>
> /******************************************************************************/
> int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *vtpm)
> {
> @@ -2235,16 +2312,24 @@ void libxl__device_disk_add(libxl__egc *egc, uint32_t
> domid,
> device_disk_add(egc, domid, disk, aodev, NULL, NULL);
> }
>
> -static int libxl__device_disk_from_xs_be(libxl__gc *gc,
> - const char *be_path,
> +static int libxl__device_disk_from_xs_fe(libxl__gc *gc,
> + const char *fe_path,
> libxl_device_disk *disk)
> {
> libxl_ctx *ctx = libxl__gc_owner(gc);
> unsigned int len;
> + const char *be_path;
> + int be_domid;
> char *tmp;
>
> libxl_device_disk_init(disk);
>
> + be_domid = libxl__get_backend_from_xs_fe(gc, fe_path, &be_path);
> + if (be_domid < 0)
> + goto cleanup;
> +
> + disk->backend_domid = be_domid;
> +
> /* "params" may not be present; but everything else must be. */
> tmp = xs_read(ctx->xsh, XBT_NULL,
> libxl__sprintf(gc, "%s/params", be_path), &len);
> @@ -2322,13 +2407,13 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx,
> uint32_t domid,
> if (!dompath) {
> goto out;
> }
> - path = libxl__xs_read(gc, XBT_NULL,
> - libxl__sprintf(gc, "%s/device/vbd/%d/backend",
> - dompath, devid));
> + path = libxl__sprintf(gc, "%s/device/vbd/%d",
> + dompath, devid);
GCSPRINTF
> if (!path)
> goto out;
>
> - rc = libxl__device_disk_from_xs_be(gc, path, disk);
> + rc = libxl__device_disk_from_xs_fe(gc, path, disk);
> +
> out:
> GC_FREE;
> return rc;
> @@ -2341,17 +2426,17 @@ static int libxl__append_disk_list_of_type(libxl__gc
> *gc,
> libxl_device_disk **disks,
> int *ndisks)
> {
> - char *be_path = NULL;
> + char *fe_path = NULL;
> char **dir = NULL;
> unsigned int n = 0;
> libxl_device_disk *pdisk = NULL, *pdisk_end = NULL;
> int rc=0;
> int initial_disks = *ndisks;
>
> - be_path = libxl__sprintf(gc, "%s/backend/%s/%d",
> - libxl__xs_get_dompath(gc, 0), type, domid);
> - dir = libxl__xs_directory(gc, XBT_NULL, be_path, &n);
> - if (dir) {
> + fe_path = libxl__sprintf(gc, "%s/device/%s",
> + libxl__xs_get_dompath(gc, domid), type);
GCSPRINTF
> + dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &n);
> + if (dir && n) {
> libxl_device_disk *tmp;
> tmp = realloc(*disks, sizeof (libxl_device_disk) * (*ndisks + n));
> if (tmp == NULL)
> @@ -2360,11 +2445,10 @@ static int libxl__append_disk_list_of_type(libxl__gc
> *gc,
> pdisk = *disks + initial_disks;
> pdisk_end = *disks + initial_disks + n;
> for (; pdisk < pdisk_end; pdisk++, dir++) {
> - const char *p;
> - p = libxl__sprintf(gc, "%s/%s", be_path, *dir);
> - if ((rc=libxl__device_disk_from_xs_be(gc, p, pdisk)))
> + rc = libxl__device_disk_from_xs_fe(gc,
> + GCSPRINTF("%s/%s", fe_path, *dir), pdisk);
> + if (rc)
> goto out;
> - pdisk->backend_domid = 0;
> *ndisks += 1;
> }
> }
> @@ -2949,17 +3033,25 @@ out:
> return;
> }
>
> -static void libxl__device_nic_from_xs_be(libxl__gc *gc,
> - const char *be_path,
> +static int libxl__device_nic_from_xs_fe(libxl__gc *gc,
> + const char *fe_path,
> libxl_device_nic *nic)
> {
> libxl_ctx *ctx = libxl__gc_owner(gc);
> unsigned int len;
> + const char *be_path;
> + int be_domid;
> char *tmp;
> int rc;
>
> + be_domid = libxl__get_backend_from_xs_fe(gc, fe_path, &be_path);
> + if (be_domid < 0)
> + return ERROR_FAIL;
> +
> libxl_device_nic_init(nic);
>
> + nic->backend_domid = be_domid;
> +
> tmp = xs_read(ctx->xsh, XBT_NULL,
> libxl__sprintf(gc, "%s/handle", be_path), &len);
> if ( tmp )
> @@ -2988,6 +3080,8 @@ static void libxl__device_nic_from_xs_be(libxl__gc *gc,
> nic->nictype = LIBXL_NIC_TYPE_VIF;
> nic->model = NULL; /* XXX Only for TYPE_IOEMU */
> nic->ifname = NULL; /* XXX Only for TYPE_IOEMU */
> +
> + return 0;
> }
>
> int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
> @@ -3002,15 +3096,11 @@ int libxl_devid_to_device_nic(libxl_ctx *ctx,
> uint32_t domid,
> if (!dompath)
> goto out;
>
> - path = libxl__xs_read(gc, XBT_NULL,
> - libxl__sprintf(gc, "%s/device/vif/%d/backend",
> - dompath, devid));
> + path = libxl__sprintf(gc, "%s/device/vif/%d", dompath, devid);
GCSPRINTF
> if (!path)
> goto out;
>
> - libxl__device_nic_from_xs_be(gc, path, nic);
> -
> - rc = 0;
> + rc = libxl__device_nic_from_xs_fe(gc, path, nic);
> out:
> GC_FREE;
> return rc;
> @@ -3022,31 +3112,33 @@ static int libxl__append_nic_list_of_type(libxl__gc
> *gc,
> libxl_device_nic **nics,
> int *nnics)
> {
> - char *be_path = NULL;
> + char *fe_path = NULL;
> char **dir = NULL;
> unsigned int n = 0;
> libxl_device_nic *pnic = NULL, *pnic_end = NULL;
> + int rc = 0;
>
> - be_path = libxl__sprintf(gc, "%s/backend/%s/%d",
> - libxl__xs_get_dompath(gc, 0), type, domid);
> - dir = libxl__xs_directory(gc, XBT_NULL, be_path, &n);
> - if (dir) {
> + fe_path = libxl__sprintf(gc, "%s/device/%s",
> + libxl__xs_get_dompath(gc, domid), type);
GCSPRINTF
> + dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &n);
> + if (dir && n) {
> libxl_device_nic *tmp;
> tmp = realloc(*nics, sizeof (libxl_device_nic) * (*nnics + n));
> if (tmp == NULL)
> return ERROR_NOMEM;
> *nics = tmp;
> pnic = *nics + *nnics;
> - *nnics += n;
> - pnic_end = *nics + *nnics;
> + pnic_end = *nics + *nnics + n;
> for (; pnic < pnic_end; pnic++, dir++) {
> - const char *p;
> - p = libxl__sprintf(gc, "%s/%s", be_path, *dir);
> - libxl__device_nic_from_xs_be(gc, p, pnic);
> - pnic->backend_domid = 0;
> + rc = libxl__device_nic_from_xs_fe(gc,
> + GCSPRINTF("%s/%s", fe_path, *dir), pnic);
> + if (rc)
> + goto out;
> + *nnics += 1;
> }
> }
> - return 0;
> +out:
> + return rc;
> }
>
> libxl_device_nic *libxl_device_nic_list(libxl_ctx *ctx, uint32_t domid, int
> *num)
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |