[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 06/29] qcow2-bitmap: add qcow2_read_bitmaps()
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [PATCH 06/29] qcow2-bitmap: add qcow2_read_bitmaps() |
Date: |
Thu, 11 Aug 2016 11:36:07 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <address@hidden>
> ---
> 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.
> +/* 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.
Kevin
- [Qemu-block] [PATCH 22/29] qmp: add persistent flag to block-dirty-bitmap-add, (continued)
- [Qemu-block] [PATCH 22/29] qmp: add persistent flag to block-dirty-bitmap-add, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 24/29] qcow2-bitmap: maintian BlockDirtyBitmap.autoload, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 01/29] hbitmap: fix dirty iter, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 14/29] qcow2-bitmap: add qcow2_bitmap_load_check(), Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 15/29] block/dirty-bitmap: introduce persistent bitmaps, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 11/29] qcow2-bitmap: check constraints, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 12/29] qcow2: add qcow2_delete_bitmaps, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 06/29] qcow2-bitmap: add qcow2_read_bitmaps(), Vladimir Sementsov-Ogievskiy, 2016/08/08
- Re: [Qemu-block] [PATCH 06/29] qcow2-bitmap: add qcow2_read_bitmaps(),
Kevin Wolf <=
- [Qemu-block] [PATCH 17/29] qcow2-bitmap: add autoclear bit, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 10/29] qcow2-bitmap: add IN_USE flag, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 20/29] qcow2-bitmap: add AUTO flag, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 03/29] block: fix bdrv_dirty_bitmap_granularity signature, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 29/29] qcow2-dirty-bitmap: refcounts, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 19/29] block/dirty-bitmap: add autoload field to BdrvDirtyBitmap, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 23/29] qmp: add autoload parameter to block-dirty-bitmap-add, Vladimir Sementsov-Ogievskiy, 2016/08/08