| Message ID | 1535644031-848-9-git-send-email-Liam.Merwick@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | off-by-one and NULL pointer accesses detected by static analysis | expand |
On 08/30/2018 10:47 AM, Liam Merwick wrote: > The commit for 0e4e4318eaa5 increments QCOW2_OL_MAX_BITNR but does not > add an array entry for QCOW2_OL_BITMAP_DIRECTORY_BITNR to metadata_ol_names[]. > As a result, an array dereference of metadata_ol_names[8] in > qcow2_pre_write_overlap_check() could result in a read outside of the array bounds. > > Fixes: 0e4e4318eaa5 ('qcow2: add overlap check for bitmap directory') > > Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com> > Reviewed-by: Darren Kenny <Darren.Kenny@oracle.com> > Reviewed-by: Mark Kanda <Mark.Kanda@oracle.com> > --- > block/qcow2-refcount.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) The fix looks correct, but to prevent the problem from happening again, I'd suggest you also add a compile-time BUG_ON that fails if the array size gets out of sync again due to another addition of another overlap detection bit.
On 30/08/18 19:43, Eric Blake wrote: > On 08/30/2018 10:47 AM, Liam Merwick wrote: >> The commit for 0e4e4318eaa5 increments QCOW2_OL_MAX_BITNR but does not >> add an array entry for QCOW2_OL_BITMAP_DIRECTORY_BITNR to >> metadata_ol_names[]. >> As a result, an array dereference of metadata_ol_names[8] in >> qcow2_pre_write_overlap_check() could result in a read outside of the >> array bounds. >> >> Fixes: 0e4e4318eaa5 ('qcow2: add overlap check for bitmap directory') >> >> Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com> >> Reviewed-by: Darren Kenny <Darren.Kenny@oracle.com> >> Reviewed-by: Mark Kanda <Mark.Kanda@oracle.com> >> --- >> block/qcow2-refcount.c | 17 +++++++++-------- >> 1 file changed, 9 insertions(+), 8 deletions(-) > > The fix looks correct, but to prevent the problem from happening again, > I'd suggest you also add a compile-time BUG_ON that fails if the array > size gets out of sync again due to another addition of another overlap > detection bit. > Good idea. There is no generic BUG_ON in QEMU (just a few private copies) or BUILD_BUG_ON. I can add a commit that introduces a copy of include/linux/build_bug.h from the Linux kernel and use BUILD_BUG_ON in this commit. Is there any reason not to do that? Regards, Liam
On 08/31/2018 08:32 AM, Liam Merwick wrote: >> >> The fix looks correct, but to prevent the problem from happening >> again, I'd suggest you also add a compile-time BUG_ON that fails if >> the array size gets out of sync again due to another addition of >> another overlap detection bit. >> > > Good idea. There is no generic BUG_ON in QEMU (just a few private > copies) or BUILD_BUG_ON. I can add a commit that introduces a copy of > include/linux/build_bug.h from the Linux kernel and use BUILD_BUG_ON in > this commit. Is there any reason not to do that? We already have the generic QEMU_BUILD_BUG_ON() used throughout the tree; that's the one to use here, rather than adding yet another macro with a similar functionality.
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 3c539f02e5ec..6504e7421324 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -2719,14 +2719,15 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset, } static const char *metadata_ol_names[] = { - [QCOW2_OL_MAIN_HEADER_BITNR] = "qcow2_header", - [QCOW2_OL_ACTIVE_L1_BITNR] = "active L1 table", - [QCOW2_OL_ACTIVE_L2_BITNR] = "active L2 table", - [QCOW2_OL_REFCOUNT_TABLE_BITNR] = "refcount table", - [QCOW2_OL_REFCOUNT_BLOCK_BITNR] = "refcount block", - [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = "snapshot table", - [QCOW2_OL_INACTIVE_L1_BITNR] = "inactive L1 table", - [QCOW2_OL_INACTIVE_L2_BITNR] = "inactive L2 table", + [QCOW2_OL_MAIN_HEADER_BITNR] = "qcow2_header", + [QCOW2_OL_ACTIVE_L1_BITNR] = "active L1 table", + [QCOW2_OL_ACTIVE_L2_BITNR] = "active L2 table", + [QCOW2_OL_REFCOUNT_TABLE_BITNR] = "refcount table", + [QCOW2_OL_REFCOUNT_BLOCK_BITNR] = "refcount block", + [QCOW2_OL_SNAPSHOT_TABLE_BITNR] = "snapshot table", + [QCOW2_OL_INACTIVE_L1_BITNR] = "inactive L1 table", + [QCOW2_OL_INACTIVE_L2_BITNR] = "inactive L2 table", + [QCOW2_OL_BITMAP_DIRECTORY_BITNR] = "bitmap directory", }; /*
The commit for 0e4e4318eaa5 increments QCOW2_OL_MAX_BITNR but does not add an array entry for QCOW2_OL_BITMAP_DIRECTORY_BITNR to metadata_ol_names[]. As a result, an array dereference of metadata_ol_names[8] in qcow2_pre_write_overlap_check() could result in a read outside of the array bounds. Fixes: 0e4e4318eaa5 ('qcow2: add overlap check for bitmap directory') Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com> Reviewed-by: Darren Kenny <Darren.Kenny@oracle.com> Reviewed-by: Mark Kanda <Mark.Kanda@oracle.com> --- block/qcow2-refcount.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)