[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 06/29] qcow2-bitmap: add qcow2_read_bitmaps()
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 06/29] qcow2-bitmap: add qcow2_read_bitmaps() |
Date: |
Thu, 11 Aug 2016 14:54:46 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Am 11.08.2016 um 14:00 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 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 <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.
>
> 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.
For a correct images, yes. But for a malicious image, size can be
anything.
> 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.
If you have the check directly after reading the bitmap, then it doesn't
really matter any more what you do in for_each_bitmap_header_in_dir().
But yes, the assertion that you suggest looks good.
Kevin
- [Qemu-devel] [PATCH 12/29] qcow2: add qcow2_delete_bitmaps, (continued)
- [Qemu-devel] [PATCH 12/29] qcow2: add qcow2_delete_bitmaps, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-devel] [PATCH 02/29] tests: add hbitmap iter test, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-devel] [PATCH 08/29] qcow2-bitmap: delete bitmap from qcow2 after load, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-devel] [PATCH 20/29] qcow2-bitmap: add AUTO flag, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-devel] [PATCH 11/29] qcow2-bitmap: check constraints, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-devel] [PATCH 19/29] block/dirty-bitmap: add autoload field to BdrvDirtyBitmap, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-devel] [PATCH 18/29] qcow2-bitmap: disallow storing bitmap to other bs, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-devel] [PATCH 06/29] qcow2-bitmap: add qcow2_read_bitmaps(), Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-devel] [PATCH 22/29] qmp: add persistent flag to block-dirty-bitmap-add, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-devel] [PATCH 03/29] block: fix bdrv_dirty_bitmap_granularity signature, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-devel] [PATCH 24/29] qcow2-bitmap: maintian BlockDirtyBitmap.autoload, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-devel] [PATCH 26/29] iotests: test qcow2 persistent dirty bitmap, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-devel] [PATCH 27/29] qcow2-bitmap: delete in_use bitmaps on image load, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-devel] [PATCH 09/29] qcow2-bitmap: add qcow2_bitmap_store(), Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-devel] [PATCH 29/29] qcow2-dirty-bitmap: refcounts, Vladimir Sementsov-Ogievskiy, 2016/08/08