Re: [Qemu-devel] [PATCH 2/2] vdi: add bounds checks for block related header fields (CVE-2014-0144)

[Markus Armbruster]

Stefan Weil <address@hidden> writes:

> Am 27.03.2014 20:49, schrieb Jeff Cody:
>> On Wed, Mar 26, 2014 at 10:38:05PM +0100, Stefan Weil wrote:
>>> (1) block_size must not be null.
>>>
>>> (2) blocks_in_image * 4 must fit into a size_t.
>>>
>>> (3) blocks_in_image * block_size must fit into a uint64_t.
>>>
>>> Header field disk_size already has a bounds check which now works
>>> because of modification (1) and (3).
>>>
>>> This patch was inspired by Jeff Cody's patch for the same problem.
>>>
>>> Cc: Jeff Cody <address@hidden>
>>> Cc: Kevin Wolf <address@hidden>
>>> Cc: Stefan Hajnoczi <address@hidden>
>>> Signed-off-by: Stefan Weil <address@hidden>
>>> ---
>>>
>>> Hello Stefan, hello Jeff,
>>>
>>> I tried to improve your previous patch - maybe you want to improve it 
>>> further
>>> or take parts from it to fix that CVE (of which I have no information).
>>>
>> 
>> Thanks!
>> 
>>> The patch was compiled on 32 and 64 bit Linux and cross compiled with 
>>> MinGW-w64,
>>> but not tested otherwise.
>>>
>>> Regards
>>> Stefan
>>>
>>>  block/vdi.c |   50 +++++++++++++++++++++++++++++++++++++++++++++-----
>>>  1 file changed, 45 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/block/vdi.c b/block/vdi.c
>>> index b832905..8fb59a1 100644
>>> --- a/block/vdi.c
>>> +++ b/block/vdi.c
>>> @@ -420,7 +420,12 @@ static int vdi_open(BlockDriverState *bs, QDict 
>>> *options, int flags,
>>>                     header.sector_size, SECTOR_SIZE);
>>>          ret = -ENOTSUP;
>>>          goto fail;
>>> -#if !defined(CONFIG_VDI_BLOCK_SIZE)
>> 
>> Is this from qemu/master?  I don't see the above in the master
>> branch.
>
> It's from the previous patch (PATCH 1/2):
> http://patchwork.ozlabs.org/patch/334116/. You have to apply both
> patches in sequence.
>
>> 
>>> +#if defined(CONFIG_VDI_BLOCK_SIZE)
>>> +    } else if (header.block_size == 0) {
>>> +        error_setg(errp, "unsupported VDI image (block size is null)");
>>> +        ret = -EINVAL;
>>> +        goto fail;
>>> +#else
>> 
>> The above is essentially dead code, since CONFIG_VDI_BLOCK_SIZE is
>> commented out.  Should this be added to configure, so that it can be
>> compiled in like other options?  I think if it is worth putting the
>> code in the driver, it probably makes sense to have a configure option
>> for it.  Otherwise, in my opinion, it is generally best to excise dead
>> code.
>> 
>> With regards to adding this prior to QEMU 2.0, this would be more in
>> line with a feature addition, I think (i.e., the support of
>> non-default block sizes).
>> 
>
> Yes, it would be a very bad idea to activate variable block sizes for
> QEMU 2.0. I don't think adding a configure option makes sense here. If
> it's only for code coverage, you can pass a -DCONFIG_VDI_BLOCK_SIZE to
> the compiler. The main point is that I did not find any existing VDI
> image in the wild which uses a non default block size, nor were there
> any bug reports about that missing feature. As long as there are no
> hints that variable block sizes not only exist in theory but also in
> real live outside of QEMU, they should remain disabled in QEMU, too.
>
>
>>>      } else if (header.block_size != DEFAULT_CLUSTER_SIZE) {
>>>          error_setg(errp, "unsupported VDI image (block size %u is not %u)",
>>>                     header.block_size, DEFAULT_CLUSTER_SIZE);
>>> @@ -435,6 +440,18 @@ static int vdi_open(BlockDriverState *bs, QDict 
>>> *options, int flags,
>>>                     (uint64_t)header.blocks_in_image * header.block_size);
>>>          ret = -ENOTSUP;
>>>          goto fail;
>>> +#if SIZE_MAX <= UINT32_MAX
>>> +    } else if (header.blocks_in_image > SIZE_MAX / sizeof(uint32_t)) {
>>> +        /* blocks_in_image * sizeof(uint32_t) must fit into size_t. */
>>> +        error_setg(errp, "unsupported VDI image (number of blocks %u, "
>>> +                   "only %zu are possible)",
>>> +                   header.blocks_in_image, SIZE_MAX / sizeof(uint32_t));
>>> +#endif
>>> +    } else if (header.blocks_in_image > UINT64_MAX / header.block_size) {
>>> +        /* blocks_in_image * block_size must fit into uint64_t. */
>>> +        error_setg(errp, "unsupported VDI image (number of blocks %u, "
>>> +                   "only %" PRIu64 " are possible)",
>>> +                   header.blocks_in_image, UINT64_MAX / header.block_size);
>> 
>> Since header.blocks_in_image is a uint32_t, this means the driver may
>> allocate up to 16GB of ram... that is my main concern here.
>> 
>
> People who want to run disk images with hundreds of terabyte should be
> able to afford 16 GB of RAM :-) And if that memory is not available,
> QEMU will tell them so (the memory allocators terminate the process in
> this case).

Hot plug the wrong image, kill your guest.  Not nice.  Suggest to use a
non-aborting memory allocation function for allocations whose size comes
from the user.  Precedence: we do that for guest RAM even though it's
not hot-plug, just to get a decent error message.

>> 
>>>      } else if (!uuid_is_null(header.uuid_link)) {
>>>          error_setg(errp, "unsupported VDI image (non-NULL link UUID)");
>>>          ret = -ENOTSUP;
>>> @@ -691,6 +708,33 @@ static int vdi_create(const char *filename, 
>>> QEMUOptionParameter *options,
>>>          options++;
>>>      }
>>>  
>>> +#if defined(CONFIG_VDI_BLOCK_SIZE)
>>> +    if (block_size == 0) {
>>> +        return -EINVAL;
>>> +    }
>>> +#endif
>>> +
>> 
>> Same concern as above with regards to dead code / new feature
>> 
>
> Activating that code would not be a good idea without prior tests and
> existing real live examples. Otherwise we might create buggy images or
> flood the world with incompatible VDI files.
>
> Removing that optional code would make it harder if somebody needs it:
> he or she would have to reinvent the wheel.
>
> Yes, we have a dilemma here.

Code under #if 0 invariably rots.  If it's an untested sketch like here,
it starts life already rotten.  Trap for the unwary.

If you want to sketch some future feature in the code, please mark it
clearly, so nobody gets tempted to treat it as anything but a sketch.