[Qemu-devel] [PATCH 0/8] off-by-one and NULL pointer accesses detected by static analysis

[Qemu-devel] [PATCH 0/8] off-by-one and NULL pointer accesses detected by static analysis

[Liam Merwick]

Below are a number of fixes to some off-by-one, read outside array bounds, and
NULL pointer accesses detected by an internal Oracle static analysis tool (Parfait).
https://labs.oracle.com/pls/apex/f?p=labs:49:::::P49_PROJECT_ID:13

I have also included a patch to add a command-line option to configure to
select if AVX2 is used or not (keeping the existing behaviour by default).
My motivation was avoiding an issue with the static analysis tool but NetSpectre
was announced as I was working on this and I felt it may have more general uses.


Liam Merwick (8):
  configure: Provide option to explicitly disable AVX2
  job: Fix off-by-one accesses to JobSTT and JobVerbTable
  block: Null pointer dereference in blk_root_get_parent_desc()
  qemu-img: potential Null pointer deref in img_commit()
  block: Fix potential Null pointer dereferences in vvfat.c
  block: dump_qlist() may dereference a Null pointer
  io: file descriptor not initialized in qio_channel_command_new_spawn()
  qcow2: Read outside array bounds in qcow2_pre_write_overlap_check()

 block/block-backend.c    |  2 +-
 block/qcow2-refcount.c   | 17 ++++++++-------
 block/vvfat.c            | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
 configure                | 11 ++++++++--
 include/qapi/qmp/qlist.h |  6 ++++++
 io/channel-command.c     |  4 ++--
 job.c                    |  4 ++--
 qemu-img.c               |  3 +++
 8 files changed, 88 insertions(+), 15 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/8] configure: Provide option to explicitly disable AVX2

[Liam Merwick]

The configure script detects if the compiler has AVX2 support and
automatically sets avx2_opt="yes" which in turn defines CONFIG_AVX2_OPT.
There is no way of explicitly overriding this setting so this commit adds
two command-line options: --enable-avx2 and --disable-avx2.

The default behaviour, when no option is specified, is to maintain the
current behaviour and enable AVX2 if the compiler supports it.

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>
---
 configure | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 58862d2ae88a..8904a91715b3 100755
--- a/configure
+++ b/configure
@@ -427,7 +427,7 @@ usb_redir=""
 opengl=""
 opengl_dmabuf="no"
 cpuid_h="no"
-avx2_opt="no"
+avx2_opt=""
 zlib="yes"
 capstone=""
 lzo=""
@@ -1332,6 +1332,10 @@ for opt do
   ;;
   --disable-glusterfs) glusterfs="no"
   ;;
+  --disable-avx2) avx2_opt="no"
+  ;;
+  --enable-avx2) avx2_opt="yes"
+  ;;
   --enable-glusterfs) glusterfs="yes"
   ;;
   --disable-virtio-blk-data-plane|--enable-virtio-blk-data-plane)
@@ -1709,6 +1713,7 @@ disabled with --disable-FEATURE, default is enabled if available:
   libxml2         for Parallels image format
   tcmalloc        tcmalloc support
   jemalloc        jemalloc support
+  avx2            AVX2 optimization support
   replication     replication support
   vhost-vsock     virtio sockets device support
   opengl          opengl support
@@ -5127,7 +5132,7 @@ fi
 # There is no point enabling this if cpuid.h is not usable,
 # since we won't be able to select the new routines.
 
-if test $cpuid_h = yes; then
+if test "$cpuid_h" = "yes" -a "$avx2_opt" != "no"; then
   cat > $TMPC << EOF
 #pragma GCC push_options
 #pragma GCC target("avx2")
@@ -5141,6 +5146,8 @@ int main(int argc, char *argv[]) { return bar(argv[0]); }
 EOF
   if compile_object "" ; then
     avx2_opt="yes"
+  else
+    avx2_opt="no"
   fi
 fi
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/8] job: Fix off-by-one accesses to JobSTT and JobVerbTable

[Liam Merwick]

In the array dereference of JobVerbTable[verb] in job_apply_verb()
the check of the index, verb, allows an overrun because an index
equal to the array size is permitted.

Similarly, in the array dereference of JobSTT[s0][s1] with index s1
in job_state_transition(), an off-by-one overrun is possible.

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>
---
 job.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/job.c b/job.c
index e36ebaafd81c..40320566f43b 100644
--- a/job.c
+++ b/job.c
@@ -166,7 +166,7 @@ bool job_is_internal(Job *job)
 static void job_state_transition(Job *job, JobStatus s1)
 {
     JobStatus s0 = job->status;
-    assert(s1 >= 0 && s1 <= JOB_STATUS__MAX);
+    assert(s1 >= 0 && s1 < JOB_STATUS__MAX);
     trace_job_state_transition(job, job->ret,
                                JobSTT[s0][s1] ? "allowed" : "disallowed",
                                JobStatus_str(s0), JobStatus_str(s1));
@@ -181,7 +181,7 @@ static void job_state_transition(Job *job, JobStatus s1)
 int job_apply_verb(Job *job, JobVerb verb, Error **errp)
 {
     JobStatus s0 = job->status;
-    assert(verb >= 0 && verb <= JOB_VERB__MAX);
+    assert(verb >= 0 && verb < JOB_VERB__MAX);
     trace_job_apply_verb(job, JobStatus_str(s0), JobVerb_str(verb),
                          JobVerbTable[verb][s0] ? "allowed" : "prohibited");
     if (JobVerbTable[verb][s0]) {
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/8] block: Null pointer dereference in blk_root_get_parent_desc()

[Liam Merwick]

The dev_id returned by the call to blk_get_attached_dev_id() in
blk_root_get_parent_desc() can be NULL (an internal call to
object_get_canonical_path may have returned NULL) so it should
be checked before dereferencing.

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/block-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index fa120630be83..210eee75006a 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -136,7 +136,7 @@ static char *blk_root_get_parent_desc(BdrvChild *child)
     }
 
     dev_id = blk_get_attached_dev_id(blk);
-    if (*dev_id) {
+    if (dev_id && *dev_id) {
         return dev_id;
     } else {
         /* TODO Callback into the BB owner for something more detailed */
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/8] qemu-img: potential Null pointer deref in img_commit()

[Liam Merwick]

The function block_job_get() may return NULL so before dereferencing
the 'job' pointer in img_commit() it should be checked.

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>
---
 qemu-img.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index b12f4cd19b0a..51fe09bd08ed 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1029,6 +1029,9 @@ static int img_commit(int argc, char **argv)
     }
 
     job = block_job_get("commit");
+    if (job == NULL) {
+        goto unref_backing;
+    }
     run_block_job(job, &local_err);
     if (local_err) {
         goto unref_backing;
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/8] block: Fix potential Null pointer dereferences in vvfat.c

[Liam Merwick]

The calls to bdrv_new_open_driver(), find_mapping_for_cluster(),
and array_get_next() may return NULL but it isn't always checked for
before dereferencing the value returned.

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/vvfat.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/block/vvfat.c b/block/vvfat.c
index fc41841a5c3c..0f1f10a2f94b 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -448,6 +448,9 @@ static direntry_t *create_long_filename(BDRVVVFATState *s, const char *filename)
 
     for(i=0;i<number_of_entries;i++) {
         entry=array_get_next(&(s->directory));
+        if (entry == NULL) {
+            continue;
+        }
         entry->attributes=0xf;
         entry->reserved[0]=0;
         entry->begin=0;
@@ -665,6 +668,9 @@ static inline void fat_set(BDRVVVFATState* s,unsigned int cluster,uint32_t value
     } else {
         int offset = (cluster*3/2);
         unsigned char* p = array_get(&(s->fat), offset);
+        if (p == NULL) {
+            return;
+        }
         switch (cluster&1) {
         case 0:
                 p[0] = value&0xff;
@@ -730,6 +736,9 @@ static inline direntry_t* create_short_and_long_name(BDRVVVFATState* s,
 
     if(is_dot) {
         entry=array_get_next(&(s->directory));
+        if (entry == NULL) {
+            return NULL;
+        }
         memset(entry->name, 0x20, sizeof(entry->name));
         memcpy(entry->name,filename,strlen(filename));
         return entry;
@@ -844,6 +853,12 @@ static int read_directory(BDRVVVFATState* s, int mapping_index)
         /* create mapping for this file */
         if(!is_dot && !is_dotdot && (S_ISDIR(st.st_mode) || st.st_size)) {
             s->current_mapping = array_get_next(&(s->mapping));
+            if (s->current_mapping == NULL) {
+                fprintf(stderr, "Failed to create mapping for file\n");
+                g_free(buffer);
+                closedir(dir);
+                return -2;
+            }
             s->current_mapping->begin=0;
             s->current_mapping->end=st.st_size;
             /*
@@ -941,6 +956,9 @@ static int init_directories(BDRVVVFATState* s,
     /* add volume label */
     {
         direntry_t* entry=array_get_next(&(s->directory));
+        if (entry == NULL) {
+            return -1;
+        }
         entry->attributes=0x28; /* archive | volume label */
         memcpy(entry->name, s->volume_label, sizeof(entry->name));
     }
@@ -953,6 +971,9 @@ static int init_directories(BDRVVVFATState* s,
     s->cluster_count=sector2cluster(s, s->sector_count);
 
     mapping = array_get_next(&(s->mapping));
+    if (mapping == NULL) {
+        return -1;
+    }
     mapping->begin = 0;
     mapping->dir_index = 0;
     mapping->info.dir.parent_mapping_index = -1;
@@ -1630,6 +1651,9 @@ static void schedule_rename(BDRVVVFATState* s,
         uint32_t cluster, char* new_path)
 {
     commit_t* commit = array_get_next(&(s->commits));
+    if (commit == NULL) {
+        return;
+    }
     commit->path = new_path;
     commit->param.rename.cluster = cluster;
     commit->action = ACTION_RENAME;
@@ -1639,6 +1663,9 @@ static void schedule_writeout(BDRVVVFATState* s,
         int dir_index, uint32_t modified_offset)
 {
     commit_t* commit = array_get_next(&(s->commits));
+    if (commit == NULL) {
+        return;
+    }
     commit->path = NULL;
     commit->param.writeout.dir_index = dir_index;
     commit->param.writeout.modified_offset = modified_offset;
@@ -1649,6 +1676,9 @@ static void schedule_new_file(BDRVVVFATState* s,
         char* path, uint32_t first_cluster)
 {
     commit_t* commit = array_get_next(&(s->commits));
+    if (commit == NULL) {
+        return;
+    }
     commit->path = path;
     commit->param.new_file.first_cluster = first_cluster;
     commit->action = ACTION_NEW_FILE;
@@ -1657,6 +1687,9 @@ static void schedule_new_file(BDRVVVFATState* s,
 static void schedule_mkdir(BDRVVVFATState* s, uint32_t cluster, char* path)
 {
     commit_t* commit = array_get_next(&(s->commits));
+    if (commit == NULL) {
+        return;
+    }
     commit->path = path;
     commit->param.mkdir.cluster = cluster;
     commit->action = ACTION_MKDIR;
@@ -2261,6 +2294,9 @@ static mapping_t* insert_mapping(BDRVVVFATState* s,
     }
     if (index >= s->mapping.next || mapping->begin > begin) {
         mapping = array_insert(&(s->mapping), index, 1);
+        if (mapping == NULL) {
+            return NULL;
+        }
         mapping->path = NULL;
         adjust_mapping_indices(s, index, +1);
     }
@@ -2428,6 +2464,9 @@ static int commit_direntries(BDRVVVFATState* s,
     direntry_t* direntry = array_get(&(s->directory), dir_index);
     uint32_t first_cluster = dir_index == 0 ? 0 : begin_of_direntry(direntry);
     mapping_t* mapping = find_mapping_for_cluster(s, first_cluster);
+    if (mapping == NULL) {
+        return -1;
+    }
 
     int factor = 0x10 * s->sectors_per_cluster;
     int old_cluster_count, new_cluster_count;
@@ -2494,6 +2533,9 @@ DLOG(fprintf(stderr, "commit_direntries for %s, parent_mapping_index %d\n", mapp
         direntry = array_get(&(s->directory), first_dir_index + i);
         if (is_directory(direntry) && !is_dot(direntry)) {
             mapping = find_mapping_for_cluster(s, first_cluster);
+            if (mapping == NULL) {
+                return -1;
+            }
             assert(mapping->mode & MODE_DIRECTORY);
             ret = commit_direntries(s, first_dir_index + i,
                 array_index(&(s->mapping), mapping));
@@ -2522,6 +2564,10 @@ static int commit_one_file(BDRVVVFATState* s,
     assert(offset < size);
     assert((offset % s->cluster_size) == 0);
 
+    if (mapping == NULL) {
+        return -1;
+    }
+
     for (i = s->cluster_size; i < offset; i += s->cluster_size)
         c = modified_fat_get(s, c);
 
@@ -2668,6 +2714,9 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
         if (commit->action == ACTION_RENAME) {
             mapping_t* mapping = find_mapping_for_cluster(s,
                     commit->param.rename.cluster);
+            if (mapping == NULL) {
+                return -1;
+            }
             char* old_path = mapping->path;
 
             assert(commit->path);
@@ -2692,6 +2741,9 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
                         if (is_file(d) || (is_directory(d) && !is_dot(d))) {
                             mapping_t* m = find_mapping_for_cluster(s,
                                     begin_of_direntry(d));
+                            if (m == NULL) {
+                                return -3;
+                            }
                             int l = strlen(m->path);
                             char* new_path = g_malloc(l + diff + 1);
 
@@ -3193,6 +3245,10 @@ static int enable_write_target(BlockDriverState *bs, Error **errp)
 
     backing = bdrv_new_open_driver(&vvfat_write_target, NULL, BDRV_O_ALLOW_RDWR,
                                    &error_abort);
+    if (!backing) {
+        ret = -EINVAL;
+        goto err;
+    }
     *(void**) backing->opaque = s;
 
     bdrv_set_backing_hd(s->bs, backing, &error_abort);
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/8] block: dump_qlist() may dereference a Null pointer

[Liam Merwick]

A NULL 'list' passed into function dump_qlist() isn't correctly
validated and can be passed to qlist_first() where it is dereferenced.

This could be resolved by checking if the list is NULL in dump_qlist()
and returning immediately. However, the general case can be handled by
adding a NULL arg check to to qlist_first() and qlist_next() and all
the callers to those functions handle that cleanly.

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>

---
 include/qapi/qmp/qlist.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
index 8d2c32ca2863..1ec716e2eb9e 100644
--- a/include/qapi/qmp/qlist.h
+++ b/include/qapi/qmp/qlist.h
@@ -58,11 +58,17 @@ void qlist_destroy_obj(QObject *obj);
 
 static inline const QListEntry *qlist_first(const QList *qlist)
 {
+    if (!qlist) {
+        return NULL;
+    }
     return QTAILQ_FIRST(&qlist->head);
 }
 
 static inline const QListEntry *qlist_next(const QListEntry *entry)
 {
+    if (!entry) {
+        return NULL;
+    }
     return QTAILQ_NEXT(entry, next);
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 7/8] io: file descriptor not initialized in qio_channel_command_new_spawn()

[Liam Merwick]

Incorrect checking of flags could result in uninitialized
file descriptor being used.

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>
---
 io/channel-command.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/io/channel-command.c b/io/channel-command.c
index 3e7eb17eff54..38deb687da21 100644
--- a/io/channel-command.c
+++ b/io/channel-command.c
@@ -59,10 +59,10 @@ qio_channel_command_new_spawn(const char *const argv[],
 
     flags = flags & O_ACCMODE;
 
-    if (flags == O_RDONLY) {
+    if ((flags & O_RDONLY) == O_RDONLY) {
         stdinnull = true;
     }
-    if (flags == O_WRONLY) {
+    if ((flags & O_WRONLY) == O_WRONLY) {
         stdoutnull = true;
     }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 8/8] qcow2: Read outside array bounds in qcow2_pre_write_overlap_check()

[Liam Merwick]

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(-)

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",
 };
 
 /*
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 7/8] io: file descriptor not initialized in qio_channel_command_new_spawn()

[Eric Blake]

On 08/30/2018 10:47 AM, Liam Merwick wrote:
> Incorrect checking of flags could result in uninitialized
> file descriptor being used.
> 
> 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>
> ---
>   io/channel-command.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/io/channel-command.c b/io/channel-command.c
> index 3e7eb17eff54..38deb687da21 100644
> --- a/io/channel-command.c
> +++ b/io/channel-command.c
> @@ -59,10 +59,10 @@ qio_channel_command_new_spawn(const char *const argv[],
>   
>       flags = flags & O_ACCMODE;
>   
> -    if (flags == O_RDONLY) {
> +    if ((flags & O_RDONLY) == O_RDONLY) {

NACK.  O_RDONLY and O_WRONLY are subsets of O_ACCMODE, which we already 
masked out above.

On some systems, we have:
O_RDONLY == 0
O_WRONLY == 1
O_RDWR == 2

On other systems, we have:
O_RDONLY == 1
O_WRONLY == 2
O_RDWR == 3

Either way, if the user passed in O_RDWR, (flags & O_RDONLY) == O_RDONLY 
returns true, which is wrong.

O_ACCMODE was historically 0x3, although now that POSIX has O_EXEC and 
O_SEARCH (which can be the same bit pattern), some systems now make 
O_ACCMODE occupy 3 bits instead of 2.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 2/8] job: Fix off-by-one accesses to JobSTT and JobVerbTable

[Eric Blake]

On 08/30/2018 10:47 AM, Liam Merwick wrote:
> In the array dereference of JobVerbTable[verb] in job_apply_verb()
> the check of the index, verb, allows an overrun because an index
> equal to the array size is permitted.
> 
> Similarly, in the array dereference of JobSTT[s0][s1] with index s1
> in job_state_transition(), an off-by-one overrun is possible.

Fortunately, these are just assertions that are too weak; we don't have 
any actual callers passing the __MAX entry to cause an actual overrun.

> 
> 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>
> ---
>   job.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/job.c b/job.c
> index e36ebaafd81c..40320566f43b 100644
> --- a/job.c
> +++ b/job.c
> @@ -166,7 +166,7 @@ bool job_is_internal(Job *job)
>   static void job_state_transition(Job *job, JobStatus s1)
>   {
>       JobStatus s0 = job->status;
> -    assert(s1 >= 0 && s1 <= JOB_STATUS__MAX);
> +    assert(s1 >= 0 && s1 < JOB_STATUS__MAX);
>       trace_job_state_transition(job, job->ret,
>                                  JobSTT[s0][s1] ? "allowed" : "disallowed",
>                                  JobStatus_str(s0), JobStatus_str(s1));
> @@ -181,7 +181,7 @@ static void job_state_transition(Job *job, JobStatus s1)
>   int job_apply_verb(Job *job, JobVerb verb, Error **errp)
>   {
>       JobStatus s0 = job->status;
> -    assert(verb >= 0 && verb <= JOB_VERB__MAX);
> +    assert(verb >= 0 && verb < JOB_VERB__MAX);
>       trace_job_apply_verb(job, JobStatus_str(s0), JobVerb_str(verb),
>                            JobVerbTable[verb][s0] ? "allowed" : "prohibited");
>       if (JobVerbTable[verb][s0]) {
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 6/8] block: dump_qlist() may dereference a Null pointer

[Eric Blake]

On 08/30/2018 10:47 AM, Liam Merwick wrote:
> A NULL 'list' passed into function dump_qlist() isn't correctly
> validated and can be passed to qlist_first() where it is dereferenced.

But dump_qlist() is static, and it is easy to prove that it will never 
be called with a NULL 'list' parameter (it's lone caller did switch 
(qobject_type(obj)), and calls dump_qlist() only for QTYPE_QLIST, which 
implies that the qobject_to(QList, obj) will succeed and never be NULL).

> 
> This could be resolved by checking if the list is NULL in dump_qlist()
> and returning immediately. However, the general case can be handled by
> adding a NULL arg check to to qlist_first() and qlist_next() and all
> the callers to those functions handle that cleanly.

NACK.  If anything, I'd be happier with:

assert(list);

in dump_qlist() to shut up the lint checker, as we do not want to slow 
down the common case of qlist_first() for something that does not 
happen.  That is, the null dereference in qlist_first() is a feature for 
detecting buggy code, and not something we need to change.

> 
> 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>
> 
> ---
>   include/qapi/qmp/qlist.h | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
> index 8d2c32ca2863..1ec716e2eb9e 100644
> --- a/include/qapi/qmp/qlist.h
> +++ b/include/qapi/qmp/qlist.h
> @@ -58,11 +58,17 @@ void qlist_destroy_obj(QObject *obj);
>   
>   static inline const QListEntry *qlist_first(const QList *qlist)
>   {
> +    if (!qlist) {
> +        return NULL;
> +    }
>       return QTAILQ_FIRST(&qlist->head);
>   }
>   
>   static inline const QListEntry *qlist_next(const QListEntry *entry)
>   {
> +    if (!entry) {
> +        return NULL;
> +    }
>       return QTAILQ_NEXT(entry, next);
>   }
>   
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 8/8] qcow2: Read outside array bounds in qcow2_pre_write_overlap_check()

[Eric Blake]

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 2/8] job: Fix off-by-one accesses to JobSTT and JobVerbTable

[Liam Merwick]

On 30/08/18 19:34, Eric Blake wrote:
> On 08/30/2018 10:47 AM, Liam Merwick wrote:
>> In the array dereference of JobVerbTable[verb] in job_apply_verb()
>> the check of the index, verb, allows an overrun because an index
>> equal to the array size is permitted.
>>
>> Similarly, in the array dereference of JobSTT[s0][s1] with index s1
>> in job_state_transition(), an off-by-one overrun is possible.
> 
> Fortunately, these are just assertions that are too weak; we don't have 
> any actual callers passing the __MAX entry to cause an actual overrun.
> 

True. In v2 I'll clarify that in the commit message

>>
>> 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>
>> ---
>>   job.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks,
Liam


>>
>> diff --git a/job.c b/job.c
>> index e36ebaafd81c..40320566f43b 100644
>> --- a/job.c
>> +++ b/job.c
>> @@ -166,7 +166,7 @@ bool job_is_internal(Job *job)
>>   static void job_state_transition(Job *job, JobStatus s1)
>>   {
>>       JobStatus s0 = job->status;
>> -    assert(s1 >= 0 && s1 <= JOB_STATUS__MAX);
>> +    assert(s1 >= 0 && s1 < JOB_STATUS__MAX);
>>       trace_job_state_transition(job, job->ret,
>>                                  JobSTT[s0][s1] ? "allowed" : 
>> "disallowed",
>>                                  JobStatus_str(s0), JobStatus_str(s1));
>> @@ -181,7 +181,7 @@ static void job_state_transition(Job *job, 
>> JobStatus s1)
>>   int job_apply_verb(Job *job, JobVerb verb, Error **errp)
>>   {
>>       JobStatus s0 = job->status;
>> -    assert(verb >= 0 && verb <= JOB_VERB__MAX);
>> +    assert(verb >= 0 && verb < JOB_VERB__MAX);
>>       trace_job_apply_verb(job, JobStatus_str(s0), JobVerb_str(verb),
>>                            JobVerbTable[verb][s0] ? "allowed" : 
>> "prohibited");
>>       if (JobVerbTable[verb][s0]) {
>>
> 

Re: [Qemu-devel] [PATCH 6/8] block: dump_qlist() may dereference a Null pointer

[Liam Merwick]

On 30/08/18 19:41, Eric Blake wrote:
> On 08/30/2018 10:47 AM, Liam Merwick wrote:
>> A NULL 'list' passed into function dump_qlist() isn't correctly
>> validated and can be passed to qlist_first() where it is dereferenced.
> 
> But dump_qlist() is static, and it is easy to prove that it will never 
> be called with a NULL 'list' parameter (it's lone caller did switch 
> (qobject_type(obj)), and calls dump_qlist() only for QTYPE_QLIST, which 
> implies that the qobject_to(QList, obj) will succeed and never be NULL).
> 
>>
>> This could be resolved by checking if the list is NULL in dump_qlist()
>> and returning immediately. However, the general case can be handled by
>> adding a NULL arg check to to qlist_first() and qlist_next() and all
>> the callers to those functions handle that cleanly.
> 
> NACK.  If anything, I'd be happier with:
> 
> assert(list);
> 

Thank works for me too.

Regards,
Liam

> in dump_qlist() to shut up the lint checker, as we do not want to slow 
> down the common case of qlist_first() for something that does not 
> happen.  That is, the null dereference in qlist_first() is a feature for 
> detecting buggy code, and not something we need to change.
> 
>>
>> 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>
>>
>> ---
>>   include/qapi/qmp/qlist.h | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
>> index 8d2c32ca2863..1ec716e2eb9e 100644
>> --- a/include/qapi/qmp/qlist.h
>> +++ b/include/qapi/qmp/qlist.h
>> @@ -58,11 +58,17 @@ void qlist_destroy_obj(QObject *obj);
>>   static inline const QListEntry *qlist_first(const QList *qlist)
>>   {
>> +    if (!qlist) {
>> +        return NULL;
>> +    }
>>       return QTAILQ_FIRST(&qlist->head);
>>   }
>>   static inline const QListEntry *qlist_next(const QListEntry *entry)
>>   {
>> +    if (!entry) {
>> +        return NULL;
>> +    }
>>       return QTAILQ_NEXT(entry, next);
>>   }
>>
> 

Re: [Qemu-devel] [PATCH 8/8] qcow2: Read outside array bounds in qcow2_pre_write_overlap_check()

[Liam Merwick]

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

Re: [Qemu-devel] [PATCH 8/8] qcow2: Read outside array bounds in qcow2_pre_write_overlap_check()

[Eric Blake]

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 7/8] io: file descriptor not initialized in qio_channel_command_new_spawn()

[Liam Merwick]

On 30/08/2018 17:18, Eric Blake wrote:
> On 08/30/2018 10:47 AM, Liam Merwick wrote:
>> Incorrect checking of flags could result in uninitialized
>> file descriptor being used.
>>
>> 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>
>> ---
>>   io/channel-command.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/io/channel-command.c b/io/channel-command.c
>> index 3e7eb17eff54..38deb687da21 100644
>> --- a/io/channel-command.c
>> +++ b/io/channel-command.c
>> @@ -59,10 +59,10 @@ qio_channel_command_new_spawn(const char *const 
>> argv[],
>>       flags = flags & O_ACCMODE;
>> -    if (flags == O_RDONLY) {
>> +    if ((flags & O_RDONLY) == O_RDONLY) {
> 
> NACK.  O_RDONLY and O_WRONLY are subsets of O_ACCMODE, which we already 
> masked out above.
> 
> On some systems, we have:
> O_RDONLY == 0
> O_WRONLY == 1
> O_RDWR == 2
> 
> On other systems, we have:
> O_RDONLY == 1
> O_WRONLY == 2
> O_RDWR == 3
> 
> Either way, if the user passed in O_RDWR, (flags & O_RDONLY) == O_RDONLY 
> returns true, which is wrong.
> 
> O_ACCMODE was historically 0x3, although now that POSIX has O_EXEC and 
> O_SEARCH (which can be the same bit pattern), some systems now make 
> O_ACCMODE occupy 3 bits instead of 2.
> 

Thanks for catching that and for the explanation.

Looking at it again, the very minor optimisation of converting the 2nd 
'if' to an 'else if' has the useful side-effect of appeasing the static 
analysis tool.

--- a/io/channel-command.c
+++ b/io/channel-command.c
@@ -61,8 +61,7 @@ qio_channel_command_new_spawn(const char *const argv[],

      if (flags == O_RDONLY) {
          stdinnull = true;
-    }
-    if (flags == O_WRONLY) {
+    } else if (flags == O_WRONLY) {
          stdoutnull = true;
      }

Regards,
Liam

Re: [Qemu-devel] [PATCH 7/8] io: file descriptor not initialized in qio_channel_command_new_spawn()

[Eric Blake]

On 08/31/2018 10:36 AM, Liam Merwick wrote:
> On 30/08/2018 17:18, Eric Blake wrote:
>> On 08/30/2018 10:47 AM, Liam Merwick wrote:
>>> Incorrect checking of flags could result in uninitialized
>>> file descriptor being used.
>>>

> 
> Looking at it again, the very minor optimisation of converting the 2nd 
> 'if' to an 'else if' has the useful side-effect of appeasing the static 
> analysis tool.

I never figured out what the tool precisely thought was wrong in the 
first place. Can you paste the output of the tool to see exactly what it 
analyzed as the potential flaw?  Perhaps the analyzer was trying to see 
what would happen if a caller submitting the fourth value (3 on systems 
where O_RDONLY is 0, 0 on systems where O_RDONLY is 1) and the code not 
behaving in that setup, even though we know that all callers only submit 
the three valid values and never the fourth invalid value?  Or maybe 
it's a weakness where we have made dependent assumptions but in 
independent branches, in such a way that we know it will never fail but 
the analyzer doesn't?

> 
> --- a/io/channel-command.c
> +++ b/io/channel-command.c
> @@ -61,8 +61,7 @@ qio_channel_command_new_spawn(const char *const argv[],
> 
>       if (flags == O_RDONLY) {
>           stdinnull = true;
> -    }
> -    if (flags == O_WRONLY) {
> +    } else if (flags == O_WRONLY) {

But this sort of change is acceptable, especially if it does shut up the 
analyzer, and done with a commit message mentioning that it is not a 
semantic change but does make it easier to use the static checker to 
find real problems by getting rid of noise.

>           stdoutnull = true;
>       }
> 
> Regards,
> Liam
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 7/8] io: file descriptor not initialized in qio_channel_command_new_spawn()

[Liam Merwick]

On 31/08/18 16:50, Eric Blake wrote:
> On 08/31/2018 10:36 AM, Liam Merwick wrote:
>> On 30/08/2018 17:18, Eric Blake wrote:
>>> On 08/30/2018 10:47 AM, Liam Merwick wrote:
>>>> Incorrect checking of flags could result in uninitialized
>>>> file descriptor being used.
>>>>
>
>>
>> Looking at it again, the very minor optimisation of converting the 
>> 2nd 'if' to an 'else if' has the useful side-effect of appeasing the 
>> static analysis tool.
>
> I never figured out what the tool precisely thought was wrong in the 
> first place. Can you paste the output of the tool to see exactly what 
> it analyzed as the potential flaw?  Perhaps the analyzer was trying to 
> see what would happen if a caller submitting the fourth value (3 on 
> systems where O_RDONLY is 0, 0 on systems where O_RDONLY is 1) and the 
> code not behaving in that setup, even though we know that all callers 
> only submit the three valid values and never the fourth invalid 
> value?  Or maybe it's a weakness where we have made dependent 
> assumptions but in independent branches, in such a way that we know it 
> will never fail but the analyzer doesn't?
>

The specific error it reported was

Error: File Invalid
    File Descriptor not Initialized [file-desc-not-init]:
       The value <unknown> is not initialized as a file descriptor.
         at line 91 of io/channel-command.c in function 
'qio_channel_command_new_spawn'.
           resource  not initialized when flags != 0 at line 62
               and flags != 1 at line 65
               and stdinnull is false at line 69
               and stdoutnull is false at line 69.


I've been staring at the code and can see no reason why it isn't a false 
positive (and I'll let the tool authors know)

Regards,
Liam

>>
>> --- a/io/channel-command.c
>> +++ b/io/channel-command.c
>> @@ -61,8 +61,7 @@ qio_channel_command_new_spawn(const char *const 
>> argv[],
>>
>>       if (flags == O_RDONLY) {
>>           stdinnull = true;
>> -    }
>> -    if (flags == O_WRONLY) {
>> +    } else if (flags == O_WRONLY) {
>
> But this sort of change is acceptable, especially if it does shut up 
> the analyzer, and done with a commit message mentioning that it is not 
> a semantic change but does make it easier to use the static checker to 
> find real problems by getting rid of noise.
>
>>           stdoutnull = true;
>>       }
>>
>> Regards,
>> Liam
>>
>

Re: [Qemu-devel] [PATCH 7/8] io: file descriptor not initialized in qio_channel_command_new_spawn()

[Eric Blake]

On 08/31/2018 11:19 AM, Liam Merwick wrote:

>>> Looking at it again, the very minor optimisation of converting the 
>>> 2nd 'if' to an 'else if' has the useful side-effect of appeasing the 
>>> static analysis tool.
>>
>> I never figured out what the tool precisely thought was wrong in the 
>> first place. Can you paste the output of the tool to see exactly what 
>> it analyzed as the potential flaw?

Thanks.

>>  Perhaps the analyzer was trying to 
>> see what would happen if a caller submitting the fourth value (3 on 
>> systems where O_RDONLY is 0, 0 on systems where O_RDONLY is 1) and the 
>> code not behaving in that setup, even though we know that all callers 
>> only submit the three valid values and never the fourth invalid 
>> value?  Or maybe it's a weakness where we have made dependent 
>> assumptions but in independent branches, in such a way that we know it 
>> will never fail but the analyzer doesn't?
>>
> 
> The specific error it reported was
> 
> Error: File Invalid
>     File Descriptor not Initialized [file-desc-not-init]:
>        The value <unknown> is not initialized as a file descriptor.
>          at line 91 of io/channel-command.c in function 
> 'qio_channel_command_new_spawn'.
>            resource  not initialized when flags != 0 at line 62

flags is not O_RDONLY,

>                and flags != 1 at line 65

flags is not O_WRONLY,

>                and stdinnull is false at line 69
>                and stdoutnull is false at line 69.

so yes, both stdinnull and stdoutnull will be false at this point. 
Based on our earlier masking, that means flags is either O_RDWR (2) or 
bogus (3).  The analyzer is then complaining about:

         dup2(stdinnull ? devnull : stdinfd[0], STDIN_FILENO);

So either it is complaining about devnull being uninitialized (which it 
is when flags is O_RDWR - but in that situation, we know stdinnull is 
false which also implies that the left half of ?: is unreachable and 
therefore shouldn't matter) or it is complaining about stdinfd[0] being 
uninitialized (but we can't reach here without having gone through the 
earlier (!stdinnull && pipe(stdinfd)), and again, given that stdinnull 
is false, that is initialized).  The fact that it lists '<unknown>' 
rather than the actual (sub-)expression that it claims is uninitialized 
doesn't help.  So yeah, I'm seriously confused as to why this false 
positive is being reported, or why the change from 'if/if' to 'if/else 
if' shuts it up.

> 
> 
> I've been staring at the code and can see no reason why it isn't a false 
> positive (and I'll let the tool authors know)

Thanks for the followup.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org