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

[Jeff Cody <jcody@redhat.com>]

On Wed, Mar 26, 2014 at 07:21:29PM +0100, Stefan Weil wrote:
> 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 <jcody@redhat.com>
> > 
> > The maximum blocks_in_image is 0xffffffff / 4, which also limits the
> > maximum disk_size for a VDI image.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  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.
> 

OK, great.

> > +/* 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.
>

I looked around, and I could not find a definitive source for a VDI
specification.  Do you know if there is a specified max size for a VDI
image?

If we assume support for 64 bit size_t values, that may or may not be
within the VDI spec (I don't know).  But I think there are practical
limits we need to set in place with our current driver implementation,
as we currently dynamically allocate the block cache based on
blocks_in_image * 4 * SECTOR_SIZE.

Of course, this needs to be tempered with the notion of not breaking
existing support for VDI images that are in-spec.

> > +#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.
> 

The VDI driver does not currently support non-default block sizes.
There is partial support for vdi image creation for variable block
sizes, but it is commented out (with a note saying it is untested).
But for image open, the vdi_open() function currently has a hard check
for header.block_size != 1MB.

So, the above macro is in line with what the VDI driver supports, I
believe.

This was meant to be a bug-fix only, so adding in new support for
non-default block sizes wasn't the original intent.


> >  #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.
> 

I agree error_setg could be useful here.

> > +        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.
>

Yes, the error message should be modified: s/sector/block


> > -                   header.block_size, 1 * MiB);
> > +                   header.block_size, VDI_BLOCK_SIZE);
> 
> Replace VDI_BLOCK_SIZE by the existing macro here.
> 

OK

> >          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?
>

In this case, maybe not.

> >      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.
>

I don't know what systems are in use in the wild.  But since we
allocate block cache to fit the entire cache in RAM currently, that
does open us up to potentially allocating a lot of memory, based on
what the image file header specifies.

That is a reason to keep the maximum blocks_in_image size bounded to
the size of 0x3fffffff.  With an unbound blocks_in_image size (except
to UINT32_MAX), the driver would potentially attempt to allocate 16GB
of RAM, if qemu attempts to open a VDI image file with such a header.

Of course, if the VDI spec allows for image sizes > 1000TB, then maybe
you are right and we should allow it, even if it means a lot of RAM
allocation.  I don't know.

I think allowing blocks_in_images and block_size to be more variable
is probably a good idea, but I think that if we are going to allow
that, we should probably modify how we handle the block cache.  And to
add that support in would seem more in line with a feature addition.