Re: [Qemu-devel] [PATCH 2/2] vdi: add bounds checks for block related header fields (CVE-2014-0144)

[Jeff Cody]

On Wed, Mar 26, 2014 at 10:38:05PM +0100, Stefan Weil wrote:
> (1) block_size must not be null.
> 
> (2) blocks_in_image * 4 must fit into a size_t.
> 
> (3) blocks_in_image * block_size must fit into a uint64_t.
> 
> Header field disk_size already has a bounds check which now works
> because of modification (1) and (3).
> 
> This patch was inspired by Jeff Cody's patch for the same problem.
> 
> Cc: Jeff Cody <address@hidden>
> Cc: Kevin Wolf <address@hidden>
> Cc: Stefan Hajnoczi <address@hidden>
> Signed-off-by: Stefan Weil <address@hidden>
> ---
> 
> Hello Stefan, hello Jeff,
> 
> I tried to improve your previous patch - maybe you want to improve it further
> or take parts from it to fix that CVE (of which I have no information).
>

Thanks!

> The patch was compiled on 32 and 64 bit Linux and cross compiled with 
> MinGW-w64,
> but not tested otherwise.
> 
> Regards
> Stefan
> 
>  block/vdi.c |   50 +++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 5 deletions(-)
> 
> diff --git a/block/vdi.c b/block/vdi.c
> index b832905..8fb59a1 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -420,7 +420,12 @@ static int vdi_open(BlockDriverState *bs, QDict 
> *options, int flags,
>                     header.sector_size, SECTOR_SIZE);
>          ret = -ENOTSUP;
>          goto fail;
> -#if !defined(CONFIG_VDI_BLOCK_SIZE)

Is this from qemu/master?  I don't see the above in the master
branch.

> +#if defined(CONFIG_VDI_BLOCK_SIZE)
> +    } else if (header.block_size == 0) {
> +        error_setg(errp, "unsupported VDI image (block size is null)");
> +        ret = -EINVAL;
> +        goto fail;
> +#else

The above is essentially dead code, since CONFIG_VDI_BLOCK_SIZE is
commented out.  Should this be added to configure, so that it can be
compiled in like other options?  I think if it is worth putting the
code in the driver, it probably makes sense to have a configure option
for it.  Otherwise, in my opinion, it is generally best to excise dead
code.

With regards to adding this prior to QEMU 2.0, this would be more in
line with a feature addition, I think (i.e., the support of
non-default block sizes).

>      } else if (header.block_size != DEFAULT_CLUSTER_SIZE) {
>          error_setg(errp, "unsupported VDI image (block size %u is not %u)",
>                     header.block_size, DEFAULT_CLUSTER_SIZE);
> @@ -435,6 +440,18 @@ static int vdi_open(BlockDriverState *bs, QDict 
> *options, int flags,
>                     (uint64_t)header.blocks_in_image * header.block_size);
>          ret = -ENOTSUP;
>          goto fail;
> +#if SIZE_MAX <= UINT32_MAX
> +    } else if (header.blocks_in_image > SIZE_MAX / sizeof(uint32_t)) {
> +        /* blocks_in_image * sizeof(uint32_t) must fit into size_t. */
> +        error_setg(errp, "unsupported VDI image (number of blocks %u, "
> +                   "only %zu are possible)",
> +                   header.blocks_in_image, SIZE_MAX / sizeof(uint32_t));
> +#endif
> +    } else if (header.blocks_in_image > UINT64_MAX / header.block_size) {
> +        /* blocks_in_image * block_size must fit into uint64_t. */
> +        error_setg(errp, "unsupported VDI image (number of blocks %u, "
> +                   "only %" PRIu64 " are possible)",
> +                   header.blocks_in_image, UINT64_MAX / header.block_size);

Since header.blocks_in_image is a uint32_t, this means the driver may
allocate up to 16GB of ram... that is my main concern here.


>      } else if (!uuid_is_null(header.uuid_link)) {
>          error_setg(errp, "unsupported VDI image (non-NULL link UUID)");
>          ret = -ENOTSUP;
> @@ -691,6 +708,33 @@ static int vdi_create(const char *filename, 
> QEMUOptionParameter *options,
>          options++;
>      }
>  
> +#if defined(CONFIG_VDI_BLOCK_SIZE)
> +    if (block_size == 0) {
> +        return -EINVAL;
> +    }
> +#endif
> +

Same concern as above with regards to dead code / new feature


> +    if (bytes > UINT64_MAX - block_size) {
> +        /* Overflow in calculation of blocks (see below). */
> +        return -ENOTSUP;
> +    }
> +
> +    /* We need enough blocks to store the given disk size,
> +       so always round up. */
> +    blocks = (bytes + block_size - 1) / block_size;
> +
> +#if SIZE_MAX <= UINT32_MAX
> +    if (blocks > SIZE_MAX / sizeof(uint32_t)) {
> +        /* blocks * sizeof(uint32_t) must fit into size_t. */
> +        return -ENOTSUP;
> +    }
> +#endif
> +
> +    if (blocks > UINT64_MAX / block_size) {
> +        /* blocks * block_size must fit into uint64_t. */
> +        return -EINVAL;
> +    }
> +
>      fd = qemu_open(filename,
>                     O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
>                     0644);
> @@ -698,10 +742,6 @@ static int vdi_create(const char *filename, 
> QEMUOptionParameter *options,
>          return -errno;
>      }
>  
> -    /* We need enough blocks to store the given disk size,
> -       so always round up. */
> -    blocks = (bytes + block_size - 1) / block_size;
> -
>      bmap_size = blocks * sizeof(uint32_t);
>      bmap_size = ((bmap_size + SECTOR_SIZE - 1) & ~(SECTOR_SIZE -1));
>  
> -- 
> 1.7.10.4
>