Re: [Qemu-devel] [PATCH] qemu: block.c: introducing "fmt:FMT:" prefix to image-filenames

[Anthony Liguori <anthony@codemonkey.ws>]

Uri Lublin wrote:
> Hello,
>
> This patch below can be considered as a version 2 of Shahar's "Qemu 
> image over raw devices" patch
> http://lists.gnu.org/archive/html/qemu-devel/2008-12/msg01083.html
>
> I think we've fixed the security flaw (that was discovered but not 
> introduced by Shahar's patch).

Doesn't the fmt= option to the block drivers achieve the same thing 
(except for not probing the backend formats)?

Regards,

Anthony Liguori

> Also I'm attaching an examples file with some debugging messages.
>
> Thanks,
>     Uri.
>
>
> ----------
> From: Uri Lublin <uril@redhat.com>
>
> The purpose of this prefix is to
> 1. Provide a way to know the backing file format without probing
>    it (setting the format upon creation time).
> 2. Enable using qcow2 format (and others) over host block devices.
>    (only if the user specifically asks for it).
>
> If no fmt:FMT: is provided we go back to probing.
>
> Based on a similar patch from Shahar Frank ("Qemu image over raw 
> devices").
> http://lists.gnu.org/archive/html/qemu-devel/2008-12/msg01083.html
>
> Also fixes a security flaw found by Daniel P. Berrange on the
> above thread which summarizes: "Autoprobing: just say no."
>
> Examples:
>
> backing file format is qcow2 (even though it's on a host block device)
> $ qemu-img create -b fmt:qcow2:/dev/loop0 -f qcow2 /tmp/uuu.qcow2
>
> force backing file format to raw (no probing)
> $ qemu-img create -f raw /tmp/image1.raw 10G
> $ qemu-img create -b fmt:raw:/tmp/image1.raw -f qcow2 /tmp/image1.qcow2
>
> Use together with other protocols, e.g. nbd
> $ qemu-nbd -v -n --snapshot -t -k /tmp/uuu.socket 
> fmt:qcow2:/tmp/images/uuu.qcow2 &
> $ qemu-img info nbd:unix:/tmp/uuu.socket
> $ qemu-system-x86_64 -snapshot -hda nbd:unix:/tmp/uuu.socket
>
> Or fat
> $ qemu-system-x86_64 -hda fmt:qcow2:/tmp/uuu.qcow2 -hdb 
> fat:floppy:/tmp/images
>
> Signed-off-by: Uri Lublin <uril@redhat.com>
> ---
>  qemu/block.c |   72 
> ++++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 60 insertions(+), 12 deletions(-)
>
> diff --git a/qemu/block.c b/qemu/block.c
> index dc744dd..5b4c517 100644
> --- a/qemu/block.c
> +++ b/qemu/block.c
> @@ -72,7 +72,7 @@ int path_is_absolute(const char *path)
>      if (*path == '/' || *path == '\\')
>          return 1;
>  #endif
> -    p = strchr(path, ':');
> +    p = strrchr(path, ':');
>      if (p)
>          p++;
>      else
> @@ -96,10 +96,23 @@ void path_combine(char *dest, int dest_size,
>
>      if (dest_size <= 0)
>          return;
> +
> +    /* copy "fmt:" prefix of filename if exists */
> +    p = strrchr(filename, ':');
> +    if (p) {
> +        len = p - filename + 1;
> +        if (dest_size <= len)
> +            return;
> +        strncpy(dest, filename, len);
> +        filename  += len;
> +        dest      += len;
> +        dest_size -= len;
> +    }
> +
>      if (path_is_absolute(filename)) {
>          pstrcpy(dest, dest_size, filename);
>      } else {
> -        p = strchr(base_path, ':');
> +        p = strrchr(base_path, ':');
>          if (p)
>              p++;
>          else
> @@ -227,30 +240,51 @@ static int is_windows_drive(const char *filename)
>  }
>  #endif
>
> +static const char *raw_filename(const char *filename)
> +{
> +    char *_filename;
> +
> +    _filename = strrchr(filename, ':');
> +    if (_filename)
> +        return _filename + 1;
> +    else
> +        return filename;
> +}
> +
>  static BlockDriver *find_protocol(const char *filename)
>  {
>      BlockDriver *drv1;
>      char protocol[128];
> -    int len;
> +    int len, is_fmt = 0;
>      const char *p;
>
> +    if (!strncmp(filename, "fmt:", 4)) {
> +        filename += 4;
> +        is_fmt = 1;
> +    }
> +
>  #ifdef _WIN32
>      if (is_windows_drive(filename) ||
>          is_windows_drive_prefix(filename))
> -        return &bdrv_raw;
> +        return NULL;
>  #endif
>      p = strchr(filename, ':');
>      if (!p)
> -        return &bdrv_raw;
> +        return NULL;
>      len = p - filename;
>      if (len > sizeof(protocol) - 1)
>          len = sizeof(protocol) - 1;
>      memcpy(protocol, filename, len);
>      protocol[len] = '\0';
>      for(drv1 = first_drv; drv1 != NULL; drv1 = drv1->next) {
> -        if (drv1->protocol_name &&
> -            !strcmp(drv1->protocol_name, protocol))
> -            return drv1;
> +        if (is_fmt) {
> +            if (!strcmp(drv1->format_name, protocol))
> +                return drv1;
> +        } else {
> +            if (drv1->protocol_name &&
> +                !strcmp(drv1->protocol_name, protocol))
> +                return drv1;
> +        }
>      }
>      return NULL;
>  }
> @@ -268,6 +302,14 @@ static BlockDriver *find_image_format(const char 
> *filename)
>         recognized as a host CDROM */
>      if (strstart(filename, "/dev/cdrom", NULL))
>          return &bdrv_host_device;
> +
> +    drv = find_protocol(filename);
> +    if ((drv != NULL) && (drv->protocol_name == NULL))
> +        return drv;
> +
> +    if (drv == NULL)
> +        filename = raw_filename(filename);
> +
>  #ifdef _WIN32
>      if (is_windows_drive(filename))
>          return &bdrv_host_device;
> @@ -281,7 +323,6 @@ static BlockDriver *find_image_format(const char 
> *filename)
>      }
>  #endif
>
> -    drv = find_protocol(filename);
>      /* no need to test disk image formats for vvfat */
>      if (drv == &bdrv_vvfat)
>          return drv;
> @@ -371,8 +412,11 @@ int bdrv_open2(BlockDriverState *bs, const char 
> *filename, int flags,
>          if (is_protocol)
>              snprintf(backing_filename, sizeof(backing_filename),
>                       "%s", filename);
> -        else
> -            realpath(filename, backing_filename);
> +        else {
> +            const char *p;
> +            p = realpath(raw_filename(filename), backing_filename);
> +            path_combine(backing_filename, sizeof(backing_filename), 
> p, filename);
> +        }
>
>          if (bdrv_create(&bdrv_qcow2, tmp_filename,
>                          total_size, backing_filename, 0) < 0) {
> @@ -386,7 +430,7 @@ int bdrv_open2(BlockDriverState *bs, const char 
> *filename, int flags,
>      if (flags & BDRV_O_FILE) {
>          drv = find_protocol(filename);
>          if (!drv)
> -            return -ENOENT;
> +            drv = &bdrv_raw;
>      } else {
>          if (!drv) {
>              drv = find_image_format(filename);
> @@ -404,6 +448,10 @@ int bdrv_open2(BlockDriverState *bs, const char 
> *filename, int flags,
>          open_flags = BDRV_O_RDWR | (flags & BDRV_O_CACHE_MASK);
>      else
>          open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
> +
> +    if (drv && !drv->protocol_name)
> +        filename = raw_filename(filename);
> +
>      ret = drv->bdrv_open(bs, filename, open_flags);
>      if ((ret == -EACCES || ret == -EPERM) && !(flags & BDRV_O_FILE)) {
>          ret = drv->bdrv_open(bs, filename, open_flags & ~BDRV_O_RDWR);