Gianni Tedesco
2010-Aug-13 15:57 UTC
[Xen-devel] [PATCH] fix invalid free segfault and use-after-free in libxl_device_disk_list()
Gah, libxl_device_disk_list() is returning a lot of pointers to free''d
data. Fix that by replacing libxl_xs_read() with xs_read() in line with
the policy.
Also fix a segfault caused by an erroneous free of the last disk-list
array element rather than the first one. This was causing xl create to
segfault when using the new qemu-dm code-base. Fix that and add a
comment about the fact that this libxl API requires a corresponding
libxl_device_disk_free() function.
Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>
diff -r dc335ebde3b5 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Thu Aug 12 18:03:23 2010 +0100
+++ b/tools/libxl/libxl.c Fri Aug 13 17:01:34 2010 +0100
@@ -1303,17 +1303,17 @@ static char ** libxl_build_device_model_
flexarray_set(dm_args, num++, "xenfv");
disks = libxl_device_disk_list(libxl_gc_owner(gc), info->domid,
&nb);
- for (; nb > 0; --nb, ++disks) {
+ for (i; i < nb; i++) {
if ( disks->is_cdrom ) {
flexarray_set(dm_args, num++, "-cdrom");
- flexarray_set(dm_args, num++, disks->physpath);
+ flexarray_set(dm_args, num++, disks[i].physpath);
}else{
- flexarray_set(dm_args, num++, libxl_sprintf(gc, "-%s",
disks->virtpath));
- flexarray_set(dm_args, num++, disks->physpath);
+ flexarray_set(dm_args, num++, libxl_sprintf(gc, "-%s",
disks[i].virtpath));
+ flexarray_set(dm_args, num++, disks[i].physpath);
}
}
+ /* FIXME: leaks disk paths */
free(disks);
-
flexarray_set(dm_args, num++, NULL);
return (char **) flexarray_contents(dm_args);
}
@@ -2435,7 +2435,7 @@ libxl_device_disk *libxl_device_disk_lis
char *be_path_tap, *be_path_vbd;
libxl_device_disk *dend, *disks, *ret = NULL;
char **b, **l = NULL;
- unsigned int numl;
+ unsigned int numl, len;
char *type;
be_path_vbd = libxl_sprintf(&gc, "%s/backend/vbd/%d",
libxl_xs_get_dompath(&gc, 0), domid);
@@ -2450,9 +2450,9 @@ libxl_device_disk *libxl_device_disk_lis
for (; disks < dend; ++disks, ++l) {
disks->backend_domid = 0;
disks->domid = domid;
- disks->physpath = libxl_xs_read(&gc, XBT_NULL,
libxl_sprintf(&gc, "%s/%s/params", be_path_vbd, *l));
+ disks->physpath = xs_read(ctx->xsh, XBT_NULL,
libxl_sprintf(&gc, "%s/%s/params", be_path_vbd, *l), &len);
libxl_string_to_phystype(ctx, libxl_xs_read(&gc, XBT_NULL,
libxl_sprintf(&gc, "%s/%s/type", be_path_vbd, *l)),
&(disks->phystype));
- disks->virtpath = libxl_xs_read(&gc, XBT_NULL,
libxl_sprintf(&gc, "%s/%s/dev", be_path_vbd, *l));
+ disks->virtpath = xs_read(ctx->xsh, XBT_NULL,
libxl_sprintf(&gc, "%s/%s/dev", be_path_vbd, *l), &len);
disks->unpluggable = atoi(libxl_xs_read(&gc, XBT_NULL,
libxl_sprintf(&gc, "%s/%s/removable", be_path_vbd, *l)));
if (!strcmp(libxl_xs_read(&gc, XBT_NULL, libxl_sprintf(&gc,
"%s/%s/mode", be_path_vbd, *l)), "w"))
disks->readwrite = 1;
@@ -2470,9 +2470,9 @@ libxl_device_disk *libxl_device_disk_lis
for (dend = ret + *num; disks < dend; ++disks, ++l) {
disks->backend_domid = 0;
disks->domid = domid;
- disks->physpath = libxl_xs_read(&gc, XBT_NULL,
libxl_sprintf(&gc, "%s/%s/params", be_path_tap, *l));
+ disks->physpath = xs_read(ctx->xsh, XBT_NULL,
libxl_sprintf(&gc, "%s/%s/params", be_path_tap, *l), &len);
libxl_string_to_phystype(ctx, libxl_xs_read(&gc, XBT_NULL,
libxl_sprintf(&gc, "%s/%s/type", be_path_tap, *l)),
&(disks->phystype));
- disks->virtpath = libxl_xs_read(&gc, XBT_NULL,
libxl_sprintf(&gc, "%s/%s/dev", be_path_tap, *l));
+ disks->virtpath = xs_read(ctx->xsh, XBT_NULL,
libxl_sprintf(&gc, "%s/%s/dev", be_path_tap, *l), &len);
disks->unpluggable = atoi(libxl_xs_read(&gc, XBT_NULL,
libxl_sprintf(&gc, "%s/%s/removable", be_path_tap, *l)));
if (!strcmp(libxl_xs_read(&gc, XBT_NULL, libxl_sprintf(&gc,
"%s/%s/mode", be_path_tap, *l)), "w"))
disks->readwrite = 1;
@@ -2552,6 +2552,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u
libxl_device_disk_add(ctx, stubdomid, disk);
disk->domid = domid;
}
+ /* FIXME: leaks disk paths */
free(disks);
return 0;
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Gianni Tedesco
2010-Aug-13 16:16 UTC
Re: [Xen-devel] [PATCH] fix invalid free segfault and use-after-free in libxl_device_disk_list()
On Fri, 2010-08-13 at 16:57 +0100, Gianni Tedesco wrote:> Gah, libxl_device_disk_list() is returning a lot of pointers to free''d > data. Fix that by replacing libxl_xs_read() with xs_read() in line with > the policy. > > Also fix a segfault caused by an erroneous free of the last disk-list > array element rather than the first one. This was causing xl create to > segfault when using the new qemu-dm code-base. Fix that and add a > comment about the fact that this libxl API requires a corresponding > libxl_device_disk_free() function. > > Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com> > > diff -r dc335ebde3b5 tools/libxl/libxl.c > --- a/tools/libxl/libxl.c Thu Aug 12 18:03:23 2010 +0100 > +++ b/tools/libxl/libxl.c Fri Aug 13 17:01:34 2010 +0100 > @@ -1303,17 +1303,17 @@ static char ** libxl_build_device_model_ > flexarray_set(dm_args, num++, "xenfv"); > > disks = libxl_device_disk_list(libxl_gc_owner(gc), info->domid, &nb); > - for (; nb > 0; --nb, ++disks) { > + for (i; i < nb; i++) { > if ( disks->is_cdrom ) {^^^^^^^^^^^^^^^^^^^^ This part is in error, please see v2 patch. Sorry for the noise> flexarray_set(dm_args, num++, "-cdrom"); > - flexarray_set(dm_args, num++, disks->physpath); > + flexarray_set(dm_args, num++, disks[i].physpath); > }else{ > - flexarray_set(dm_args, num++, libxl_sprintf(gc, "-%s", disks->virtpath)); > - flexarray_set(dm_args, num++, disks->physpath); > + flexarray_set(dm_args, num++, libxl_sprintf(gc, "-%s", disks[i].virtpath)); > + flexarray_set(dm_args, num++, disks[i].physpath); > } > } > + /* FIXME: leaks disk paths */ > free(disks); > - > flexarray_set(dm_args, num++, NULL); > return (char **) flexarray_contents(dm_args); > } > @@ -2435,7 +2435,7 @@ libxl_device_disk *libxl_device_disk_lis > char *be_path_tap, *be_path_vbd; > libxl_device_disk *dend, *disks, *ret = NULL; > char **b, **l = NULL; > - unsigned int numl; > + unsigned int numl, len; > char *type; > > be_path_vbd = libxl_sprintf(&gc, "%s/backend/vbd/%d", libxl_xs_get_dompath(&gc, 0), domid); > @@ -2450,9 +2450,9 @@ libxl_device_disk *libxl_device_disk_lis > for (; disks < dend; ++disks, ++l) { > disks->backend_domid = 0; > disks->domid = domid; > - disks->physpath = libxl_xs_read(&gc, XBT_NULL, libxl_sprintf(&gc, "%s/%s/params", be_path_vbd, *l)); > + disks->physpath = xs_read(ctx->xsh, XBT_NULL, libxl_sprintf(&gc, "%s/%s/params", be_path_vbd, *l), &len); > libxl_string_to_phystype(ctx, libxl_xs_read(&gc, XBT_NULL, libxl_sprintf(&gc, "%s/%s/type", be_path_vbd, *l)), &(disks->phystype)); > - disks->virtpath = libxl_xs_read(&gc, XBT_NULL, libxl_sprintf(&gc, "%s/%s/dev", be_path_vbd, *l)); > + disks->virtpath = xs_read(ctx->xsh, XBT_NULL, libxl_sprintf(&gc, "%s/%s/dev", be_path_vbd, *l), &len); > disks->unpluggable = atoi(libxl_xs_read(&gc, XBT_NULL, libxl_sprintf(&gc, "%s/%s/removable", be_path_vbd, *l))); > if (!strcmp(libxl_xs_read(&gc, XBT_NULL, libxl_sprintf(&gc, "%s/%s/mode", be_path_vbd, *l)), "w")) > disks->readwrite = 1; > @@ -2470,9 +2470,9 @@ libxl_device_disk *libxl_device_disk_lis > for (dend = ret + *num; disks < dend; ++disks, ++l) { > disks->backend_domid = 0; > disks->domid = domid; > - disks->physpath = libxl_xs_read(&gc, XBT_NULL, libxl_sprintf(&gc, "%s/%s/params", be_path_tap, *l)); > + disks->physpath = xs_read(ctx->xsh, XBT_NULL, libxl_sprintf(&gc, "%s/%s/params", be_path_tap, *l), &len); > libxl_string_to_phystype(ctx, libxl_xs_read(&gc, XBT_NULL, libxl_sprintf(&gc, "%s/%s/type", be_path_tap, *l)), &(disks->phystype)); > - disks->virtpath = libxl_xs_read(&gc, XBT_NULL, libxl_sprintf(&gc, "%s/%s/dev", be_path_tap, *l)); > + disks->virtpath = xs_read(ctx->xsh, XBT_NULL, libxl_sprintf(&gc, "%s/%s/dev", be_path_tap, *l), &len); > disks->unpluggable = atoi(libxl_xs_read(&gc, XBT_NULL, libxl_sprintf(&gc, "%s/%s/removable", be_path_tap, *l))); > if (!strcmp(libxl_xs_read(&gc, XBT_NULL, libxl_sprintf(&gc, "%s/%s/mode", be_path_tap, *l)), "w")) > disks->readwrite = 1; > @@ -2552,6 +2552,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u > libxl_device_disk_add(ctx, stubdomid, disk); > disk->domid = domid; > } > + /* FIXME: leaks disk paths */ > free(disks); > return 0; > } > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel