Re: [Qemu-stable] [Qemu-devel] [PATCH for-2.0 16/47] vdi: add bounds checks for blocks_in_image and disk_size header fields (CVE-2014-0144)

[Stefan Weil]

Hi Stefan, hi Jeff,

please cc me for future block/vdi.c related patches. See more comments
below.

Am 26.03.2014 13:05, schrieb Stefan Hajnoczi:
> From: Jeff Cody <address@hidden>
> 
> The maximum blocks_in_image is 0xffffffff / 4, which also limits the
> maximum disk_size for a VDI image.
> 
> Signed-off-by: Jeff Cody <address@hidden>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block/vdi.c | 28 +++++++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/block/vdi.c b/block/vdi.c
> index ac9a025..718884d 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -120,6 +120,12 @@ typedef unsigned char uuid_t[16];
>  
>  #define VDI_IS_ALLOCATED(X) ((X) < VDI_DISCARDED)
>  
> +#define VDI_BLOCK_SIZE           (1 * MiB)

There is already DEFAULT_CLUSTER_SIZE which should be used instead of
VDI_BLOCK_SIZE.

> +/* max blocks in image is (0xffffffff / 4) */
> +#define VDI_BLOCKS_IN_IMAGE_MAX  0x3fffffff

This looks wrong. I think it should be (SIZE_MAX / sizeof(uint32_t)). 64
bit platforms with a 64 bit size_t allow more blocks.

> +#define VDI_DISK_SIZE_MAX        ((uint64_t)VDI_BLOCKS_IN_IMAGE_MAX * \
> +                                  (uint64_t)VDI_BLOCK_SIZE)
> +

This macro cannot be used because the block size might have a non
default value.

>  #if !defined(CONFIG_UUID)
>  static inline void uuid_generate(uuid_t out)
>  {
> @@ -385,6 +391,11 @@ static int vdi_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      vdi_header_print(&header);
>  #endif
>  
> +    if (header.disk_size > VDI_DISK_SIZE_MAX) {

Here error_setg is missing. The style does not match the other checks.
More changes are needed because VDI_DISK_SIZE_MAX cannot be used.

> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
>      if (header.disk_size % SECTOR_SIZE != 0) {
>          /* 'VBoxManage convertfromraw' can create images with odd disk sizes.
>             We accept them but round the disk size to the next multiple of
> @@ -420,9 +431,9 @@ static int vdi_open(BlockDriverState *bs, QDict *options, 
> int flags,
>                     header.sector_size, SECTOR_SIZE);
>          ret = -ENOTSUP;
>          goto fail;
> -    } else if (header.block_size != 1 * MiB) {
> +    } else if (header.block_size != VDI_BLOCK_SIZE) {
>          error_setg(errp, "unsupported VDI image (sector size %u is not %u)",

Here is a copy+paste bug which was recently introduced.

> -                   header.block_size, 1 * MiB);
> +                   header.block_size, VDI_BLOCK_SIZE);

Replace VDI_BLOCK_SIZE by the existing macro here.

>          ret = -ENOTSUP;
>          goto fail;
>      } else if (header.disk_size >
> @@ -441,6 +452,10 @@ static int vdi_open(BlockDriverState *bs, QDict 
> *options, int flags,
>          error_setg(errp, "unsupported VDI image (non-NULL parent UUID)");
>          ret = -ENOTSUP;
>          goto fail;
> +    } else if (header.blocks_in_image > VDI_BLOCKS_IN_IMAGE_MAX) {
> +        error_setg(errp, "unsupported VDI image (too many blocks)");

Fix test and improve error message (show limit) here.

> +        ret = -ENOTSUP;
> +        goto fail;
>      }
>  
>      bs->total_sectors = header.disk_size / SECTOR_SIZE;
> @@ -689,11 +704,17 @@ static int vdi_create(const char *filename, 
> QEMUOptionParameter *options,
>          options++;
>      }
>  
> +    if (bytes > VDI_DISK_SIZE_MAX) {

Dto.

> +        result = -EINVAL;
> +        goto exit;
> +    }
> +
>      fd = qemu_open(filename,
>                     O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
>                     0644);
>      if (fd < 0) {
> -        return -errno;
> +        result = -errno;
> +        goto exit;
>      }
>  
>      /* We need enough blocks to store the given disk size,
> @@ -754,6 +775,7 @@ static int vdi_create(const char *filename, 
> QEMUOptionParameter *options,
>          result = -errno;
>      }
>  
> +exit:

Is goto+label better than a simple return statement?

>      return result;
>  }
>  
> 

Do we need this patch for QEMU 2.0? For 32 bit systems, the image size
limit is 1000 TB, and that image would need 4 GB for the block cache in
memory. Are such image sizes used anywhere? For 64 bit systems, the
limit will be much higher.

Regards
Stefan