Re: [Xen-devel] [PATCH] fix invalid free segfault and use-after-free in libxl_device_disk_list()

[Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>]

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@xxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel