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)

[Stefan Weil]

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