Re: [Qemu-devel] [PATCH 06/29] qcow2-bitmap: add qcow2_read_bitmaps()

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

On 11.08.2016 12:36, Kevin Wolf wrote:
> Am 08.08.2016 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Add qcow2_read_bitmaps, reading bitmap directory as specified in
>> docs/specs/qcow2.txt
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2-bitmap.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   block/qcow2.h        |   9 +++++
>>   2 files changed, 109 insertions(+)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index cd18b07..91ddd5f 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -25,6 +25,12 @@
>>    * THE SOFTWARE.
>>    */
>>   
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +
>> +#include "block/block_int.h"
>> +#include "block/qcow2.h"
>> +
>>   /* NOTICE: BME here means Bitmaps Extension and used as a namespace for
>>    * _internal_ constants. Please do not use this _internal_ abbreviation for
>>    * other needs and/or outside of this file. */
>> @@ -42,6 +48,100 @@
>>   /* bits [1, 8] U [56, 63] are reserved */
>>   #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001fe
>>   
>> +#define for_each_bitmap_header_in_dir(h, dir, size) \
>> +    for (h = (QCow2BitmapHeader *)(dir); \
>> +         h < (QCow2BitmapHeader *)((uint8_t *)(dir) + size); \
>> +         h = next_dir_entry(h))
> It's hard to see just from this patch (see below), but 'size' contains
> user input and cannot be trusted to be a multiple of sizeof(*h).
> If it isn't, I think this loop will run for a final element where only
> half of the QCow2BitmapHeader is covererd by size and a buffer overflow
> follows.

this macro loops through the Bitmap Directory, so, here Bitmap Directory 
is defined as pair (dir, size), and size is a size of Bitmap Directory 
and by define it must be sum of all bitmap header sizes. However, you 
are right, something should be checked..  Like this I think:

bool check_dir_iter(QCow2BitmapHeader *it, void *directory_end) {
    return ((void *)it == directory_end) || ((void *)(it + 1) <= 
directory_end) && ((void *)next_dir_entry(it) <= directory_end);
}

+#define for_each_bitmap_header_in_dir(h, dir, size) \
+    for (h = (QCow2BitmapHeader *)(dir); \
+         assert(check_dir_iter(h)), h < (QCow2BitmapHeader *)((uint8_t *)(dir) + size); \
+         h = next_dir_entry(h))


And immediately after reading bitmap from file there should be similar 
checking loop but with error output instead of assert.

>
>> +/* directory_read
>> + * Read bitmaps directory from bs by @offset and @size. Convert it to cpu
>> + * format from BE.
>> + */
>> +static uint8_t *directory_read(BlockDriverState *bs,
>> +                               uint64_t offset, uint64_t size, Error **errp)
>> +{
>> +    int ret;
>> +    uint8_t *dir;
>> +    QCow2BitmapHeader *h;
>> +
>> +    dir = g_try_malloc0(size);
> This could be g_try_malloc without 0 because you immediately overwrite
> all of it anyway.
>
>> +    if (dir == NULL) {
>> +        error_setg(errp, "Can't allocate space for bitmap directory.");
>> +        return NULL;
>> +    }
>> +
>> +    ret = bdrv_pread(bs->file, offset, dir, size);
>> +    if (ret < 0) {
>> +        error_setg_errno(errp, -ret, "Can't read bitmap directory.");
>> +        goto fail;
>> +    }
>> +
>> +    /* loop is safe because next entry offset is calculated after conversion to
>> +     * cpu format */
>> +    for_each_bitmap_header_in_dir(h, dir, size) {
>> +        bitmap_header_to_cpu(h);
>> +    }
>> +
>> +    if ((uint8_t *)h != dir + size) {
>> +        error_setg(errp, "Broken bitmap directory.");
>> +        goto fail;
>> +    }
> Aha, you check for the unaligned case, but only after the damage has
> already been done (you byteswapped bytes outside the allocated memory).
>
>> +    return dir;
>> +
>> +fail:
>> +    g_free(dir);
>> +
>> +    return NULL;
>> +}
>> +
>> +int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp)
>> +{
>> +    BDRVQcow2State *s = bs->opaque;
>> +
>> +    if (s->bitmap_directory != NULL) {
>> +        error_setg(errp, "Try read bitmaps, when they are already read.");
>> +        return -EEXIST;
>> +    }
> Is this error ever supposed to happen? If not, should this be assert()?
>
>> +    if (s->nb_bitmaps == 0) {
>> +        /* No bitmaps - nothing to do */
>> +        return 0;
>> +    }
>> +
>> +    s->bitmap_directory = directory_read(bs, s->bitmap_directory_offset,
>> +                                         s->bitmap_directory_size, errp);
>> +    if (s->bitmap_directory == NULL) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index b12cecc..7f6e023 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -292,6 +292,11 @@ typedef struct BDRVQcow2State {
>>       unsigned int nb_snapshots;
>>       QCowSnapshot *snapshots;
>>   
>> +    uint64_t bitmap_directory_offset;
>> +    uint64_t bitmap_directory_size;
>> +    uint8_t *bitmap_directory;
>> +    unsigned int nb_bitmaps;
> I think for a good review, patch 13 must come much earlier. Currently
> you declare the variables, but they aren't actually initialised, so I
> would have to guess what they could mean. And when reviewing patch 13 I
> must remember what my assumptions were and check whether they match the
> actual code. I know that I can't reliably do this.
>
> So as a general rule of thumb, try to introduce things in an order that
> every step can be reviewed and if possible also tested on its own rather
> than introducing lots of dead code and putting all of it to use only in
> the final patch.

Ok, thanks for explanation.

>
> Kevin

Agree with all comments, will fix in next version

-- 
Best regards,
Vladimir