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

[Qemu-devel] [PATCH v3 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.

v1 -> v2
Based on feedback from Eric Blake:
patch2: reworded commit message to clarify issue
patch6: Reverted common qlist routines and added assert to qlist_dump instead
patch7: Fixed incorrect logic
patch8: Added QEMU_BUILD_BUG_ON to catch future іnstance at compile-time

v2 -> v3
Based on feedback from Eric Blake:
patch6: removed double space from commit message
patch8: removed unnecessary comment and updated QEMU_BUILD_BUG_ON to use ARRAY_SIZE
Added Eric's R-b to patches 6,7,8

Liam Merwick (8):
  configure: Provide option to explicitly disable AVX2
  job: Fix off-by-one assert checks for 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: potential unnecessary check in qio_channel_command_new_spawn()
  qcow2: Read outside array bounds in qcow2_pre_write_overlap_check()

 block/block-backend.c  |  2 +-
 block/qapi.c           |  2 ++
 block/qcow2-refcount.c | 18 ++++++++--------
 block/vvfat.c          | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++
 configure              | 11 ++++++++--
 io/channel-command.c   |  3 +--
 job.c                  |  4 ++--
 qemu-img.c             |  3 +++
 8 files changed, 84 insertions(+), 15 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 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 v3 2/8] job: Fix off-by-one assert checks for JobSTT and JobVerbTable

[Liam Merwick]

In the assert checking 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 assert check of JobSTT[s0][s1] with index s1
in job_state_transition(), an off-by-one overrun is not flagged
either.

This is not a run-time issue as there are no callers actually
passing in the max value.

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>
Reviewed-by: Eric Blake <eblake@redhat.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

Re: [Qemu-devel] [PATCH v3 2/8] job: Fix off-by-one assert checks for JobSTT and JobVerbTable

[John Snow]

On 08/31/2018 02:16 PM, Liam Merwick wrote:
> In the assert checking 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 assert check of JobSTT[s0][s1] with index s1
> in job_state_transition(), an off-by-one overrun is not flagged
> either.
> 

Oops.

> This is not a run-time issue as there are no callers actually
> passing in the max value.
> 

Good.

> 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>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: John Snow <jsnow@redhat.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]) {
> 

[Qemu-devel] [PATCH v3 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

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1812 bytes --]

Hi,

On 31.08.18 20:16, Liam Merwick wrote:
> 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;

I rather think that blk_get_attached_dev_id() needs attention first.  It
returns an explicitly empty string if blk->dev is NULL.  If NULL was a
valid return value, it should just return NULL there.

Besides this caller, there are two callers that pass the dev_id to
qapi_event_send_device_tray_moved().  Now in practice that allows the
string to be NULL, but there is a comment in visit_type_str() that says
one should not pass NULL.

So it's either changing blk_get_attached_dev_id() to return NULL when
there is no valid ID (instead of the empty string, and then we could
save ourselves the check "*dev_id" here and elsewhere), but then we have
to fix all callers.

Or we make it return an empty string if object_get_canonical_path()
returned NULL.

Max

>      } else {
>          /* TODO Callback into the BB owner for something more detailed */
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Liam Merwick]

On 12/10/18 15:48, Max Reitz wrote:
> Hi,
> 
> On 31.08.18 20:16, Liam Merwick wrote:
>> 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;
> 
> I rather think that blk_get_attached_dev_id() needs attention first.  It
> returns an explicitly empty string if blk->dev is NULL.  If NULL was a
> valid return value, it should just return NULL there.
> 
> Besides this caller, there are two callers that pass the dev_id to
> qapi_event_send_device_tray_moved().  Now in practice that allows the
> string to be NULL, but there is a comment in visit_type_str() that says
> one should not pass NULL.
> 
> So it's either changing blk_get_attached_dev_id() to return NULL when
> there is no valid ID (instead of the empty string, and then we could
> save ourselves the check "*dev_id" here and elsewhere), but then we have
> to fix all callers.
> 
> Or we make it return an empty string if object_get_canonical_path()
> returned NULL.
> 

I went with the latter and now (in upcoming v4) check the return value 
from object_get_canonical_path() and return an empty string if it's NULL.

Regards,
Liam


> Max
> 
>>       } else {
>>           /* TODO Callback into the BB owner for something more detailed */
>>
> 
> 

[Qemu-devel] [PATCH v3 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

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

[John Snow]

On 08/31/2018 02:16 PM, Liam Merwick wrote:
> 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>

Reviewed-by: John Snow <jsnow@redhat.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;
> 

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]

On 31.08.18 20:16, Liam Merwick wrote:
> The function block_job_get() may return NULL so before dereferencing
> the 'job' pointer in img_commit() it should be checked.

It may not because the job yields before executing anything (if it
started successfully; but otherwise, commit_active_start() would have
returned an error).  Therefore, I think the better solution is to
assert(job) here.

(It would be a serious bug if block_job_get() returned NULL here, so
it's definitely not something we can be quiet about.  But this patch
makes it so the user doesn't even notice.)

Max

> 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;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Liam Merwick]

On 12/10/18 15:51, Max Reitz wrote:
> On 31.08.18 20:16, Liam Merwick wrote:
>> The function block_job_get() may return NULL so before dereferencing
>> the 'job' pointer in img_commit() it should be checked.
> 
> It may not because the job yields before executing anything (if it
> started successfully; but otherwise, commit_active_start() would have
> returned an error).  Therefore, I think the better solution is to
> assert(job) here.
> 


Switched patch to use assert()

Regards,
Liam


> (It would be a serious bug if block_job_get() returned NULL here, so
> it's definitely not something we can be quiet about.  But this patch
> makes it so the user doesn't even notice.)
> 
> Max
> 
>> 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;
>>
> 
> 

[Qemu-devel] [PATCH v3 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

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 9659 bytes --]

On 31.08.18 20:16, Liam Merwick wrote:
> 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;
> +        }

This is a bug in array_ensure_allocated().  It uses g_realloc() with a
non-zero size, so that function will never return NULL.  It will rather
abort().

Therefore, array_ensure_allocated() cannot fail.  Consequentially,
array_get_next() cannot fail.

>          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;
> +        }

This is only reached if array_get_next() was called before.  Therefore,
this cannot return NULL.

However, an assert(array->pointer); in array_get() can't hurt.

>          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;
> +        }

As above.

>          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;
> +            }

As above.

>              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;
> +        }

As above.

>          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;
> +    }

As above.

>      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;
> +    }

As above.

>      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;
> +    }

As above.

>      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;
> +    }

As above.

>      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;
> +    }


As above.

>      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;
> +        }

I haven't checked array_insert()'s code for buffer overflows, but just
like the other functions above, it cannot ever return NULL (unless
array->item_size were 0, which it never is), because g_realloc() never
returns 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;
> +    }

This looks like a real issue.  I'm not sufficiently proficient in vvfat
to know whether this would warrant an assertion (it looks after the root
directory, which probably just should be there), so I'm OK with just
returning an error here.

However, qemu's codestyle forbids mixing statements with declarations,
so this needs to be moved below all of the declarations that follow still.

>      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;
> +            }

Looks OK.

>              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;
> +    }
> +

Agreed.

(What's going on with the return values in this function?)

>      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;

Agreed, but needs to be moved below this declaration.

>  
>              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;
> +                            }

I wouldn't try to follow the weird style (unless you understand the
reasoning behind it, which I don't). :-)

Just return either -1 or -EIO.

>                              int l = strlen(m->path);
>                              char* new_path = g_malloc(l + diff + 1);

And again, needs to be moved below the declarations, although we don't
want to do the g_malloc() before, of course.

>  
> @@ -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;
> +    }

This cannot return NULL because we pass &error_abort.  Therefore, qemu
will abort before this function returns NULL.

Max

>      *(void**) backing->opaque = s;
>  
>      bdrv_set_backing_hd(s->bs, backing, &error_abort);
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Liam Merwick]

On 12/10/18 16:14, Max Reitz wrote:
> On 31.08.18 20:16, Liam Merwick wrote:
>> 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;
>> +        }
> 
> This is a bug in array_ensure_allocated().  It uses g_realloc() with a
> non-zero size, so that function will never return NULL.  It will rather
> abort().
> 
> Therefore, array_ensure_allocated() cannot fail.  Consequentially,
> array_get_next() cannot fail.
> 


I've reverted this (and the rest of the 'As above' comments below)


>>           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;
>> +        }
> 
> This is only reached if array_get_next() was called before.  Therefore,
> this cannot return NULL.
> 
> However, an assert(array->pointer); in array_get() can't hurt.


Done.

> 
>>           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;
>> +        }
> 
> As above.
> 
>>           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;
>> +            }
> 
> As above.
> 
>>               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;
>> +        }
> 
> As above.
> 
>>           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;
>> +    }
> 
> As above.
> 
>>       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;
>> +    }
> 
> As above.
> 
>>       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;
>> +    }
> 
> As above.
> 
>>       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;
>> +    }
> 
> As above.
> 
>>       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;
>> +    }
> 
> 
> As above.
> 
>>       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;
>> +        }
> 
> I haven't checked array_insert()'s code for buffer overflows, but just
> like the other functions above, it cannot ever return NULL (unless
> array->item_size were 0, which it never is), because g_realloc() never
> returns NULL.


Reverted


> 
>>           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;
>> +    }
> 
> This looks like a real issue.  I'm not sufficiently proficient in vvfat
> to know whether this would warrant an assertion (it looks after the root
> directory, which probably just should be there), so I'm OK with just
> returning an error here.
> 
> However, qemu's codestyle forbids mixing statements with declarations,
> so this needs to be moved below all of the declarations that follow still.
>


Fixed


>>       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;
>> +            }
> 
> Looks OK.
> 
>>               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;
>> +    }
>> +
> 
> Agreed.
> 
> (What's going on with the return values in this function?)
> 
>>       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;
> 
> Agreed, but needs to be moved below this declaration.

Done

> 
>>   
>>               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;
>> +                            }
> 
> I wouldn't try to follow the weird style (unless you understand the
> reasoning behind it, which I don't). :-)
> 
> Just return either -1 or -EIO.
> 

I had tried to guess the sequence in the previous version but it was a 
lottery :-)
Will go with -1


>>                               int l = strlen(m->path);
>>                               char* new_path = g_malloc(l + diff + 1);
> 
> And again, needs to be moved below the declarations, although we don't
> want to do the g_malloc() before, of course.
> 


Done

>>   
>> @@ -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;
>> +    }
> 
> This cannot return NULL because we pass &error_abort.  Therefore, qemu
> will abort before this function returns NULL.


I just added an assert instead.

Thanks for the review.

Regards,
Liam

> 
>>       *(void**) backing->opaque = s;
>>   
>>       bdrv_set_backing_hd(s->bs, backing, &error_abort);
>>
> 
> 

[Qemu-devel] [PATCH v3 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.

Given that dump_qlist() is static, and callers already do the right
thing, just add an assert to catch future potential bugs.

Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/qapi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/qapi.c b/block/qapi.c
index c66f949db839..e81be604217c 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -740,6 +740,8 @@ static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
     const QListEntry *entry;
     int i = 0;
 
+    assert(list);
+
     for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
         QType type = qobject_type(entry->value);
         bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
-- 
1.8.3.1

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1866 bytes --]

On 31.08.18 20:16, 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.
> 
> Given that dump_qlist() is static, and callers already do the right
> thing, just add an assert to catch future potential bugs.
> 
> Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/qapi.c | 2 ++
>  1 file changed, 2 insertions(+)

I don't disagree, but I don't see why the program just wouldn't crash if
someone passed a NULL pointer.  And I don't quite see why anyone would
pass a NULL pointer.

Of course it's reasonable to just add an assert() to reinforce the
contract; but we have so many functions that just take a pointer that
they assume to be non-NULL and then immediately dereference it.  Nearly
every blk_* function takes a BlockBackend that is always assumed to be
non-NULL, for instance, and I don't really want to put assert()s into
all of them.  Or another example: dump_qobject() and dump_qdict() do
exactly the same -- if we added an assertion in dump_qlist(), we would
actually have to add the very same assertions there, too.

So I don't really object this patch (because it's not wrong), but I
don't think it's very useful.

Max

> diff --git a/block/qapi.c b/block/qapi.c
> index c66f949db839..e81be604217c 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -740,6 +740,8 @@ static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
>      const QListEntry *entry;
>      int i = 0;
>  
> +    assert(list);
> +
>      for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
>          QType type = qobject_type(entry->value);
>          bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Liam Merwick]

On 12/10/18 16:22, Max Reitz wrote:
> On 31.08.18 20:16, 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.
>>
>> Given that dump_qlist() is static, and callers already do the right
>> thing, just add an assert to catch future potential bugs.
>>
>> Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   block/qapi.c | 2 ++
>>   1 file changed, 2 insertions(+)
> 
> I don't disagree, but I don't see why the program just wouldn't crash if
> someone passed a NULL pointer.  And I don't quite see why anyone would
> pass a NULL pointer.
> 
> Of course it's reasonable to just add an assert() to reinforce the
> contract; but we have so many functions that just take a pointer that
> they assume to be non-NULL and then immediately dereference it.  Nearly
> every blk_* function takes a BlockBackend that is always assumed to be
> non-NULL, for instance, and I don't really want to put assert()s into
> all of them.  Or another example: dump_qobject() and dump_qdict() do
> exactly the same -- if we added an assertion in dump_qlist(), we would
> actually have to add the very same assertions there, too.
> 
> So I don't really object this patch (because it's not wrong), but I
> don't think it's very useful.


I agree with all the above - however I kept the patch in the series 
given it was helping reduce the static analysis noise (hopefully making 
it easier to spot real issues)

Regards,
Liam


> 
> Max
> 
>> diff --git a/block/qapi.c b/block/qapi.c
>> index c66f949db839..e81be604217c 100644
>> --- a/block/qapi.c
>> +++ b/block/qapi.c
>> @@ -740,6 +740,8 @@ static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
>>       const QListEntry *entry;
>>       int i = 0;
>>   
>> +    assert(list);
>> +
>>       for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
>>           QType type = qobject_type(entry->value);
>>           bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
>>
> 
> 

[Qemu-devel] [PATCH v3 7/8] io: potential unnecessary check in qio_channel_command_new_spawn()

[Liam Merwick]

In qio_channel_command_new_spawn() the 'flags' variable is checked
to see if /dev/null should be used for stdin or stdout; first with
O_RDONLY and then O_WRONLY.  However the second check for O_WRONLY
is only needed if flags != O_RDONLY and therefore should be an
else if statement.

This minor optimization has the added benefit of suppressing a warning
from a static analysis tool (Parfait) which incorrectly reported an
incorrect checking of flags in qio_channel_command_new_spawn() could
result in an uninitialized file descriptor being used. Removing this
noise will help us better find real issues.

Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 io/channel-command.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/io/channel-command.c b/io/channel-command.c
index 3e7eb17eff54..82acd3234915 100644
--- 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;
     }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 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: Eric Blake <eblake@redhat.com>
---
 block/qcow2-refcount.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3c539f02e5ec..46082aeac1d6 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2719,15 +2719,17 @@ 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",
 };
+QEMU_BUILD_BUG_ON(QCOW2_OL_MAX_BITNR != ARRAY_SIZE(metadata_ol_names));
 
 /*
  * First performs a check for metadata overlaps (through
-- 
1.8.3.1

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2324 bytes --]

On 31.08.18 20:16, 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: Eric Blake <eblake@redhat.com>
> ---
>  block/qcow2-refcount.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 3c539f02e5ec..46082aeac1d6 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -2719,15 +2719,17 @@ 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",
>  };
> +QEMU_BUILD_BUG_ON(QCOW2_OL_MAX_BITNR != ARRAY_SIZE(metadata_ol_names));
>  
>  /*
>   * First performs a check for metadata overlaps (through
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Markus Armbruster]

I'm afraid this fell through the cracks, most likely because you
neglected to cc: maintainers.  I'm doing that for you now.  Next time,
feed your patches to scripts/get_maintainer.pl for suggestions on whom
to copy.  Thanks!

Liam Merwick <Liam.Merwick@oracle.com> writes:

> 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.
>
> v1 -> v2
> Based on feedback from Eric Blake:
> patch2: reworded commit message to clarify issue
> patch6: Reverted common qlist routines and added assert to qlist_dump instead
> patch7: Fixed incorrect logic
> patch8: Added QEMU_BUILD_BUG_ON to catch future іnstance at compile-time
>
> v2 -> v3
> Based on feedback from Eric Blake:
> patch6: removed double space from commit message
> patch8: removed unnecessary comment and updated QEMU_BUILD_BUG_ON to use ARRAY_SIZE
> Added Eric's R-b to patches 6,7,8
>
> Liam Merwick (8):
>   configure: Provide option to explicitly disable AVX2
>   job: Fix off-by-one assert checks for 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: potential unnecessary check in qio_channel_command_new_spawn()
>   qcow2: Read outside array bounds in qcow2_pre_write_overlap_check()
>
>  block/block-backend.c  |  2 +-
>  block/qapi.c           |  2 ++
>  block/qcow2-refcount.c | 18 ++++++++--------
>  block/vvfat.c          | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  configure              | 11 ++++++++--
>  io/channel-command.c   |  3 +--
>  job.c                  |  4 ++--
>  qemu-img.c             |  3 +++
>  8 files changed, 84 insertions(+), 15 deletions(-)