From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38577) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXofD-0003a6-Sl for qemu-devel@nongnu.org; Thu, 11 Aug 2016 08:00:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXof6-0006PT-Pw for qemu-devel@nongnu.org; Thu, 11 Aug 2016 08:00:42 -0400 Received: from mailhub.sw.ru ([195.214.232.25]:28737 helo=relay.sw.ru) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXof6-0006P0-7a for qemu-devel@nongnu.org; Thu, 11 Aug 2016 08:00:36 -0400 References: <1470668720-211300-1-git-send-email-vsementsov@virtuozzo.com> <1470668720-211300-7-git-send-email-vsementsov@virtuozzo.com> <20160811093607.GB5035@noname.redhat.com> From: Vladimir Sementsov-Ogievskiy Message-ID: <57AC68D4.1080605@virtuozzo.com> Date: Thu, 11 Aug 2016 15:00:20 +0300 MIME-Version: 1.0 In-Reply-To: <20160811093607.GB5035@noname.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 06/29] qcow2-bitmap: add qcow2_read_bitmaps() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, mreitz@redhat.com, armbru@redhat.com, eblake@redhat.com, jsnow@redhat.com, famz@redhat.com, den@openvz.org, stefanha@redhat.com, pbonzini@redhat.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 >> --- >> 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