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

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(-)

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.

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

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

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

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

> -                   header.block_size, 1 * MiB);
> +                   header.block_size, VDI_BLOCK_SIZE);

Replace VDI_BLOCK_SIZE by the existing macro here.

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

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

Regards
Stefan

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.

Am 27.03.2014 19:52, schrieb Jeff Cody:
[...]
> 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?

I used the reference which I also mentioned in the header comment of
block/vdi.c: http://forums.virtualbox.org/viewtopic.php?t=8046. It does
not say anything about a specific maximum size.

The image size is limited by some entries in VdiHeader: disk_size is 64
bit, so UINT64_MAX is a hard limit. blocks_in_image is 32 bit, so there
is another hard limit of UINT32_MAX * block_size where the default block
size is 1 MiB.

As you write below, the current implementation in block/vdi.c imposes
additional limits which are lower than the hard limits above: the block
cache is allocated and filled by functions which take a size_t argument.

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

I fixed this in the patch which I have sent this week. See
http://patchwork.ozlabs.org/patch/334116/.

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

Agreed, but it's also good to make the necessary changes so that they go
into the right direction.

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

Either we crash because of the 0x3fffffff limit, or we might crash
because a memory allocation fails (but it might also be successful). I
prefer this 2nd variant.

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

I implemented most of the variable block handling. The only reason why I
did not activate it by default was that I had no real test cases.

Regards
Stefan

On Thu, Mar 27, 2014 at 08:58:38PM +0100, Stefan Weil wrote:
> Am 27.03.2014 19:52, schrieb Jeff Cody:
> >> 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.
> 
> Either we crash because of the 0x3fffffff limit, or we might crash
> because a memory allocation fails (but it might also be successful). I
> prefer this 2nd variant.

From a user perspective, hotplugging a disk should never kill the VM.
Instead the hotplug command should fail for invalid image files.

If a valid image can cause QEMU abort then the block driver
implementation needs to use a metadata cache to avoid putting everything
in RAM.

Stefan

On Fri, Mar 28, 2014 at 10:07:22AM +0100, Stefan Hajnoczi wrote:
> On Thu, Mar 27, 2014 at 08:58:38PM +0100, Stefan Weil wrote:
> > Am 27.03.2014 19:52, schrieb Jeff Cody:
> > >> 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.
> > 
> > Either we crash because of the 0x3fffffff limit, or we might crash
> > because a memory allocation fails (but it might also be successful). I
> > prefer this 2nd variant.
> 
> From a user perspective, hotplugging a disk should never kill the VM.
> Instead the hotplug command should fail for invalid image files.
> 
> If a valid image can cause QEMU abort then the block driver
> implementation needs to use a metadata cache to avoid putting everything
> in RAM.
>

We also don't know what a valid image really is.  Regardless, it is
legitimate for QEMU to fail to open a valid image as 'unsupported', if
the current code is not able to reasonably handle the image.

On that note, I wonder if the disk size / blocks in image limit in my
original patch is too generous - it would still allow the allocation
of 4G of data for the block cache at the upper limit.