Re: [Qemu-devel] [PATCH v2 3/5] block: initial VHDX driver support framework - supports open and probe

[Stefan Hajnoczi <stefanha@gmail.com>]

On Tue, Apr 23, 2013 at 10:24:22AM -0400, Jeff Cody wrote:
> +    if (!vhdx_checksum_is_valid(buffer, VHDX_HEADER_BLOCK_SIZE, 4) ||
> +        s->rt.signature != VHDX_RT_MAGIC) {
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
> +    for (i = 0; i < s->rt.entry_count; i++) {

It's nice to avoid signed/unsigned comparisons.  i should be uint32_t
just like entry_count.

> +        memcpy(&rt_entry, buffer+offset, sizeof(rt_entry));
> +        offset += sizeof(rt_entry);

Looks like we're trusting rt.entry_count to be a sane value?  Need to
prevent offset from exceeding buffer size.

> +    while (logical_sector_size >>= 1) {
> +        s->logical_sector_size_bits++;
> +    }
> +    while (sectors_per_block >>= 1) {
> +        s->sectors_per_block_bits++;
> +    }
> +    while (chunk_ratio >>= 1) {
> +        s->chunk_ratio_bits++;
> +    }
> +    while (block_size >>= 1) {
> +        s->block_size_bits++;
> +    }

ctz()/clo() do this.

> +static int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s)
> +{
> +    int ret = 0;
> +    int i;
> +    vhdx_header *hdr;
> +
> +    hdr = s->headers[s->curr_header];
> +
> +    /* either either the log guid, or log length is zero,

either either

> +    s->bat_offset = s->bat_rt.file_offset;
> +    s->bat_entries = s->bat_rt.length / sizeof(vhdx_bat_entry);
> +    s->bat = qemu_blockalign(bs, s->bat_rt.length);

No sanity check was done on bat_rt.length.  If this allocation fails
QEMU will exit.  Could be used as a DoS if you can get someone to attach
a malicious VHDX to their VM?