From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:44185)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <jcody@redhat.com>) id 1UUzvr-0001kk-Dh
	for qemu-devel@nongnu.org; Wed, 24 Apr 2013 09:40:28 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <jcody@redhat.com>) id 1UUzvl-0006GB-VO
	for qemu-devel@nongnu.org; Wed, 24 Apr 2013 09:40:23 -0400
Received: from mx1.redhat.com ([209.132.183.28]:49780)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <jcody@redhat.com>) id 1UUzvl-0006Fw-K7
	for qemu-devel@nongnu.org; Wed, 24 Apr 2013 09:40:17 -0400
Date: Wed, 24 Apr 2013 09:40:09 -0400
From: Jeff Cody <jcody@redhat.com>
Message-ID: <20130424134009.GD4131@localhost.localdomain>
References: <cover.1366726446.git.jcody@redhat.com>
	<56a0e0347fde5396fb6b2a260de5b1a867a588d6.1366726446.git.jcody@redhat.com>
	<20130424132110.GD24635@stefanha-thinkpad.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20130424132110.GD24635@stefanha-thinkpad.redhat.com>
Subject: Re: [Qemu-devel] [PATCH v2 3/5] block: initial VHDX driver support
 framework - supports open and probe
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Stefan Hajnoczi <stefanha@gmail.com>
Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com

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.