On Wed, Apr 24, 2013 at 03:21:10PM +0200, Stefan Hajnoczi wrote:
> 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.
I agree. I will also double check the other parsing routines (e.g.
vhdx_parse_metadata()).
>
> > + 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.
>
Agree again, and I will also check the other parsers as well.
> > + 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.
>
Ah, yes! I will switch over to using those.
> > +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
>
Thanks
> > + 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?
Yes, bat_rt.length needs to be verified here as well. I will add that
in.
