| Message ID | 1470668720-211300-7-git-send-email-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > +/* 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
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
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 <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. 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
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)) + typedef enum BitmapType { BT_DIRTY_TRACKING_BITMAP = 1 } BitmapType; + +static inline void bitmap_header_to_cpu(QCow2BitmapHeader *h) +{ + be64_to_cpus(&h->bitmap_table_offset); + be32_to_cpus(&h->bitmap_table_size); + be32_to_cpus(&h->flags); + be16_to_cpus(&h->name_size); + be32_to_cpus(&h->extra_data_size); +} + +static inline int calc_dir_entry_size(size_t name_size) +{ + return align_offset(sizeof(QCow2BitmapHeader) + name_size, 8); +} + +static inline int dir_entry_size(QCow2BitmapHeader *h) +{ + return calc_dir_entry_size(h->name_size); +} + +static inline QCow2BitmapHeader *next_dir_entry(QCow2BitmapHeader *entry) +{ + return (QCow2BitmapHeader *)((uint8_t *)entry + dir_entry_size(entry)); +} + +/* 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); + 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; + } + + 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; + } + + 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; + int flags; int qcow_version; bool use_lazy_refcounts; @@ -599,6 +604,10 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs, void qcow2_free_snapshots(BlockDriverState *bs); int qcow2_read_snapshots(BlockDriverState *bs); +/* qcow2-bitmap.c functions */ +void qcow2_free_bitmaps(BlockDriverState *bs); +int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp); + /* qcow2-cache.c functions */ Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables); int qcow2_cache_destroy(BlockDriverState* bs, Qcow2Cache *c);
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(+)