Re: New Defects reported by Coverity Scan for XenProject

Re: New Defects reported by Coverity Scan for XenProject

[Ian Jackson]

scan-admin@coverity.com writes ("New Defects reported by Coverity Scan for XenProject"):
> Please find the latest report on new defect(s) introduced to XenProject found with Coverity Scan.
>
> 35 new defect(s) introduced to XenProject found with Coverity Scan.
> 2 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.
> 
> New defect(s) Reported-by: Coverity Scan
> Showing 20 of 35 defect(s)

I have been through the tools reports here.  None of them are security
problems in released branches.


I would like some advice from Coverity experts on the two below:



> ** CID 1358088:  Concurrent data access violations  (MISSING_LOCK)

Applies to 1358086..1358105 inclusive.  Here is a sample:

> *** CID 1358088:  Concurrent data access violations  (MISSING_LOCK)
> /tools/libxl/libxl_event.c: 2189 in libxl__ao_progress_report()
> 2183         } else if (how->callback) {
> 2184             libxl__aop_occurred *aop = libxl__zalloc(&egc->gc, sizeof(*aop));
> 2185             ao->progress_reports_outstanding++;
> 2186             aop->ao = ao;
> 2187             aop->ev = ev;
> 2188             aop->how = how;
> >>>     CID 1358088:  Concurrent data access violations  (MISSING_LOCK)
> >>>     Accessing "egc->aops_for_callback.tqh_last" without holding lock "libxl__ctx.lock". Elsewhere, "libxl__egc.aops_for_callback.tqh_last" is accessed with "libxl__ctx.lock" held 34 out of 44 times.
> 2189             LIBXL_TAILQ_INSERT_TAIL(&egc->aops_for_callback, aop, entry);
> 2190             LOG(DEBUG,"ao %p: progress report: callback queued aop=%p",ao,aop);
> 2191         } else {
> 2192             LOG(DEBUG,"ao %p: progress report: event queued ev=%p type=%s",
> 2193                 ao, ev, libxl_event_type_to_string(ev->type));
> 2194             libxl__event_occurred(egc, ev);

This is a false positive.  An egc is always allocated on the stack of
the thread that uses it.  It is only ever used by that thread.  So
does not need to be protected by a lock.

Is there a way we can teach Coverity about this ?



> ** CID 1358085:  Incorrect expression  (IDENTICAL_BRANCHES)
> /tools/libxl/_libxl_types.c: 5611 in libxl_device_usbdev_gen_json()

Applies to 1358081..1358084 inclusive.

Here is a sample:

> *** CID 1358085:  Incorrect expression  (IDENTICAL_BRANCHES)
> /tools/libxl/_libxl_types.c: 5611 in libxl_device_usbdev_gen_json()
> 5605                     if (s != yajl_gen_status_ok)
> 5606                         goto out;
> 5607                 break;
> 5608             }
> 5609         }
> 5610         s = yajl_gen_map_close(hand);
> >>>     CID 1358085:  Incorrect expression  (IDENTICAL_BRANCHES)
> >>>     The same code is executed when the condition "s != yajl_gen_status_ok" is true or false, because the code in the if-then branch and after the if statement is identical. Should the if statement be removed?
> 5611         if (s != yajl_gen_status_ok)
> 5612             goto out;
> 5613         out:
> 5614         return s;

This is a false positive arising from autogenerated code.  The
autogenerated code naturally has a completely systematic error
handling pattern, which leads to it sometimes doing this:

    if (error)
       goto out;
  out:
    /* exit path */

Is there a way to arrange to always persuade Coverity that this is
intentional ?


Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: New Defects reported by Coverity Scan for XenProject

[Jan Beulich]

On 22.05.2024 11:56, scan-admin@coverity.com wrote:
> ** CID 1598431:  Memory - corruptions  (OVERRUN)
> 
> 
> ________________________________________________________________________________________________________
> *** CID 1598431:  Memory - corruptions  (OVERRUN)
> /xen/common/trace.c: 798 in trace()
> 792         }
> 793     
> 794         if ( rec_size > bytes_to_wrap )
> 795             insert_wrap_record(buf, rec_size);
> 796     
> 797         /* Write the original record */
>>>>     CID 1598431:  Memory - corruptions  (OVERRUN)
>>>>     Overrunning callee's array of size 28 by passing argument "extra" (which evaluates to 31) in call to "__insert_record".
> 798         __insert_record(buf, event, extra, cycles, rec_size, extra_data);
> 799     
> 800     unlock:
> 801         spin_unlock_irqrestore(&this_cpu(t_lock), flags);
> 802     
> 803         /* Notify trace buffer consumer that we've crossed the high water mark. */

How does the tool conclude "extra" evaluating to 31, when at the top of
the function it is clearly checked to be less than 28?

> ** CID 1598430:  Uninitialized variables  (UNINIT)
> 
> 
> ________________________________________________________________________________________________________
> *** CID 1598430:  Uninitialized variables  (UNINIT)
> /xen/arch/x86/mm/shadow/multi.c: 2109 in trace_shadow_emulate()
> 2103             d.va = va;
> 2104     #if GUEST_PAGING_LEVELS == 3
> 2105             d.emulation_count = this_cpu(trace_extra_emulation_count);
> 2106     #endif
> 2107             d.flags = this_cpu(trace_shadow_path_flags);
> 2108     
>>>>     CID 1598430:  Uninitialized variables  (UNINIT)
>>>>     Using uninitialized value "d". Field "d.emulation_count" is uninitialized when calling "trace".
> 2109             trace(event, sizeof(d), &d);
> 2110         }
> 2111     }
> 2112     #endif /* CONFIG_HVM */
> 2113     
> 2114     /**************************************************************************/

This, otoh, looks to be a valid (but long-standing) issue, which I'll make
a patch for.

Jan

Re: New Defects reported by Coverity Scan for XenProject

[Andrew Cooper]

On 22/05/2024 11:05 am, Jan Beulich wrote:
> On 22.05.2024 11:56, scan-admin@coverity.com wrote:
>> ** CID 1598431:  Memory - corruptions  (OVERRUN)
>>
>>
>> ________________________________________________________________________________________________________
>> *** CID 1598431:  Memory - corruptions  (OVERRUN)
>> /xen/common/trace.c: 798 in trace()
>> 792         }
>> 793     
>> 794         if ( rec_size > bytes_to_wrap )
>> 795             insert_wrap_record(buf, rec_size);
>> 796     
>> 797         /* Write the original record */
>>>>>     CID 1598431:  Memory - corruptions  (OVERRUN)
>>>>>     Overrunning callee's array of size 28 by passing argument "extra" (which evaluates to 31) in call to "__insert_record".
>> 798         __insert_record(buf, event, extra, cycles, rec_size, extra_data);
>> 799     
>> 800     unlock:
>> 801         spin_unlock_irqrestore(&this_cpu(t_lock), flags);
>> 802     
>> 803         /* Notify trace buffer consumer that we've crossed the high water mark. */
> How does the tool conclude "extra" evaluating to 31, when at the top of
> the function it is clearly checked to be less than 28?

Which "top" ?

The reasoning is:

 2. Condition extra % 4UL /* sizeof (uint32_t) */, taking false branch.
 3. Condition extra / 4UL /* sizeof (uint32_t) */ > 7, taking false branch.
 4. cond_at_most: Checking extra / 4UL > 7UL implies that extra may be
up to 31 on the false branch.

which is where 31 comes from.

What Coverity hasn't done is equated "<31 && multiple of 4" to mean
"<28".  I don't think this is unreasonable; analysis has to prune the
reasoning somewhere...

This is (fundamentally) a dumb-ABI problem where we're passing a byte
count but only ever wanting to use it as a unit-of-uint32_t's count.

But it's also problem that we're passing both extra and rec_size into
__insert_record() when one is calculated from the other.

I had decided to leave this alone for now, but maybe it could do with
some improvements (simplifications) to the code.

~Andrew

Re: New Defects reported by Coverity Scan for XenProject

[Jan Beulich]

On 05.05.2024 11:54, scan-admin@coverity.com wrote:
> Hi,
> 
> Please find the latest report on new defect(s) introduced to XenProject found with Coverity Scan.
> 
> 2 new defect(s) introduced to XenProject found with Coverity Scan.
> 1 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.
> 
> New defect(s) Reported-by: Coverity Scan
> Showing 2 of 2 defect(s)
> 
> 
> ** CID 1596837:    (USE_AFTER_FREE)
> /tools/firmware/xen-dir/xen-root/xen/common/gzip/inflate.c: 943 in inflate_dynamic()
> /xen/common/gzip/inflate.c: 935 in inflate_dynamic()
> /tools/firmware/xen-dir/xen-root/xen/common/gzip/inflate.c: 935 in inflate_dynamic()
> /xen/common/gzip/inflate.c: 935 in inflate_dynamic()
> /xen/common/gzip/inflate.c: 935 in inflate_dynamic()
> /xen/common/gzip/inflate.c: 943 in inflate_dynamic()
> /xen/common/gzip/inflate.c: 943 in inflate_dynamic()
> /tools/firmware/xen-dir/xen-root/xen/common/gzip/inflate.c: 935 in inflate_dynamic()
> /tools/firmware/xen-dir/xen-root/xen/common/gzip/inflate.c: 943 in inflate_dynamic()
> /xen/common/gzip/inflate.c: 943 in inflate_dynamic()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 1596837:    (USE_AFTER_FREE)
> /tools/firmware/xen-dir/xen-root/xen/common/gzip/inflate.c: 943 in inflate_dynamic()
> 937             goto out;
> 938         }
> 939     
> 940         DEBG("dyn6 ");
> 941     
> 942         /* decompress until an end-of-block code */
>>>>     CID 1596837:    (USE_AFTER_FREE)
>>>>     Calling "inflate_codes" dereferences freed pointer "tl".
> 943         if (inflate_codes(tl, td, bl, bd)) {
> 944             ret = 1;
> 945             goto out;
> 946         }

While first I thought the tool may be confused by the earlier huft_free()
(matching an earlier huft_build()), ...

> ** CID 1596836:    (USE_AFTER_FREE)
> /xen/common/gzip/inflate.c: 943 in inflate_dynamic()
> /xen/common/gzip/inflate.c: 943 in inflate_dynamic()
> /tools/firmware/xen-dir/xen-root/xen/common/gzip/inflate.c: 943 in inflate_dynamic()
> /xen/common/gzip/inflate.c: 943 in inflate_dynamic()
> /tools/firmware/xen-dir/xen-root/xen/common/gzip/inflate.c: 943 in inflate_dynamic()
> /tools/firmware/xen-dir/xen-root/xen/common/gzip/inflate.c: 943 in inflate_dynamic()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 1596836:    (USE_AFTER_FREE)
> /xen/common/gzip/inflate.c: 943 in inflate_dynamic()
> 937             goto out;
> 938         }
> 939     
> 940         DEBG("dyn6 ");
> 941     
> 942         /* decompress until an end-of-block code */
>>>>     CID 1596836:    (USE_AFTER_FREE)
>>>>     Calling "inflate_codes" dereferences freed pointer "td".
> 943         if (inflate_codes(tl, td, bl, bd)) {
> 944             ret = 1;
> 945             goto out;
> 946         }

... no dual usage exists for td. Hence I'm utterly confused as to what the
tool is "thinking". In fact it looks like there is an opposite issue in
both inflate_fixed() and inflate_dynamic(): tl and td are leaked when
inflate_codes() fails. I guess I'll make a patch ...

Jan

Re: New Defects reported by Coverity Scan for XenProject

[Jan Beulich]

On 05.11.2023 10:58, scan-admin@coverity.com wrote:
> Hi,
> 
> Please find the latest report on new defect(s) introduced to XenProject found with Coverity Scan.
> 
> 1 new defect(s) introduced to XenProject found with Coverity Scan.
> 1 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.
> 
> New defect(s) Reported-by: Coverity Scan
> Showing 1 of 1 defect(s)
> 
> 
> ** CID 1548622:  Null pointer dereferences  (NULL_RETURNS)
> /tools/firmware/xen-dir/xen-root/xen/arch/x86/cpu/mcheck/mcaction.c: 96 in mc_memerr_dhandler()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 1548622:  Null pointer dereferences  (NULL_RETURNS)
> /tools/firmware/xen-dir/xen-root/xen/arch/x86/cpu/mcheck/mcaction.c: 96 in mc_memerr_dhandler()
> 90                     d = rcu_lock_domain_by_id(bank->mc_domid);
> 91                     ASSERT(d);

No matter that this code can certainly do with hardening, how can - with
this ASSERT() - ...

> 92                     gfn = get_gpfn_from_mfn((bank->mc_addr) >> PAGE_SHIFT);
> 93     
> 94                     if ( unmmap_broken_page(d, mfn, gfn) )
> 95                     {
>>>>     CID 1548622:  Null pointer dereferences  (NULL_RETURNS)
>>>>     Dereferencing "d", which is known to be "NULL".
> 96                         printk("Unmap broken memory %"PRI_mfn" for DOM%d failed\n",
> 97                                mfn_x(mfn), d->domain_id);

... Coverity "know" that d is going to be NULL here? Best I can infer is
that in a release build d _may_ end up being NULL.

Jan

> 98                         goto vmce_failed;
> 99                     }
> 100     
> 101                     mc_vcpuid = global->mc_vcpuid;
> 
> 
> ________________________________________________________________________________________________________
> To view the defects in Coverity Scan visit, https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yrkTUyUdtq5BaG0O6OgOkaWpauWH6sxLlz8YmsOhJ7zG6w078-2FaiuRz-2FB00i-2BJg44c-3DkX6u_NrtkCdDF-2FTaaXLm1QcFPOFnojIs14Wzrh5dJFBeSVj1z0ksrlVQuW7Zy-2FqT57QjqzVjiJF2PJIK07-2BSEjUth5ouhE2qFNhId4LvekHJXd6ELiP0-2B8XnhP1gdLp7TRFFOvUeT6Lddf1YNNmN9XrN3-2BNawzLRRIl7-2FdJPswV5cGaWoBREWQjGxEUac95xJecxLgQNoDwwrxYdn9zW95rrbSw-3D-3D
> 
>   To manage Coverity Scan email notifications for "jbeulich@suse.com", click https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yped04pjJnmXOsUBtKYNIXxbOS2X-2FCa8eHT5AKto97sa5BC-2BcmyJ5bO5I-2FkczMtlG1epQQquNnD30oPjt4w9gsO1RjVU3f-2FwTsFle9tjKmWG5Kz60AmOhqmk5R1j-2BvLczY-3DAAhv_NrtkCdDF-2FTaaXLm1QcFPOFnojIs14Wzrh5dJFBeSVj1z0ksrlVQuW7Zy-2FqT57Qjqz6yaPk3udV-2B3LoxYpnR2IqMFgeYigqqRYw7WrjuBUd8j1KxekL3U98J70arECI-2BGRvwoXYzOJyfYbO5RKuSCm6Sa6RhyBu6G5o3KPByDgONSFcrLmsCWs9Tu18suJjyZJI0LMS3WFM-2BGpXs6QChyjA-3D-3D
> 

Re: New Defects reported by Coverity Scan for XenProject

[Jan Beulich]

On 11.06.2023 12:07, scan-admin@coverity.com wrote:
> *** CID 1532324:  Memory - corruptions  (OVERRUN)
> /xen/common/trace.c: 800 in __trace_var()
> 794         }
> 795     
> 796         if ( rec_size > bytes_to_wrap )
> 797             insert_wrap_record(buf, rec_size);
> 798     
> 799         /* Write the original record */
>>>>     CID 1532324:  Memory - corruptions  (OVERRUN)
>>>>     Overrunning callee's array of size 28 by passing argument "extra" (which evaluates to 31) in call to "__insert_record".
> 800         __insert_record(buf, event, extra, cycles, rec_size, extra_data);
> 801     
> 802     unlock:
> 803         spin_unlock_irqrestore(&this_cpu(t_lock), flags);
> 804     
> 805         /* Notify trace buffer consumer that we've crossed the high water mark. */

Earlier in the function we have

    if ( extra % sizeof(uint32_t) ||
         extra / sizeof(uint32_t) > TRACE_EXTRA_MAX )
        return printk_once(XENLOG_WARNING
                           "Trace event %#x bad size %u, discarding\n",
                           event, extra);

Therefore I don't see where the tool takes from that a value of 31 in
"extra" can reach said line.

> *** CID 1532322:  Null pointer dereferences  (FORWARD_NULL)
> /tools/libs/stat/xenstat_qmp.c: 220 in qmp_parse_stats()
> 214     
> 215     	ptr[0] = qstats[QMP_STATS_RETURN]; /* "return" */
> 216     	if ((ret_obj = yajl_tree_get(info, ptr, yajl_t_array)) == NULL)
> 217     		goto done;
> 218     
> 219     	/* Array of devices */
>>>>     CID 1532322:  Null pointer dereferences  (FORWARD_NULL)
>>>>     Dereferencing null pointer "(ret_obj != NULL && ret_obj->type == yajl_t_array) ? &ret_obj->u.array : NULL".
> 220     	for (i=0; i<YAJL_GET_ARRAY(ret_obj)->len; i++) {
> 221     		memset(&vbd, 0, sizeof(xenstat_vbd));
> 222     		qmp_devname = NULL;
> 223     		stats_obj = YAJL_GET_ARRAY(ret_obj)->values[i];
> 224     
> 225     		ptr[0] = qstats[QMP_STATS_DEVICE]; /* "device" */

At least to an uninformed user like me the tool looks to be right here,
in case ret_obj->type != yajl_t_array after yajl_tree_get(). But it may
of course be that yajl_tree_get() will only ever return NULL or objects
with their type set to its 3rd argument.

> ** CID 1532319:    (DEADCODE)
> /tools/tests/x86_emulator/avx512er.c: 1826 in simd_test()
> /tools/tests/x86_emulator/sse.c: 1324 in simd_test()
> /tools/tests/x86_emulator/avx512er.c: 1324 in simd_test()
> /tools/tests/x86_emulator/3dnow.c: 1826 in simd_test()
> /tools/tests/x86_emulator/sse.c: 1826 in simd_test()
> /tools/tests/x86_emulator/3dnow.c: 1324 in simd_test()

I'm going to ignore all these issues in emulator test harness test blob
sources.

> *** CID 1532318:  Memory - corruptions  (OVERLAPPING_COPY)
> /tools/firmware/xen-dir/xen-root/xen/arch/x86/x86_emulate/x86_emulate.c: 1987 in x86_emulate()
> 1981             dst.val  = *dst.reg;
> 1982             goto xchg;
> 1983     
> 1984         case 0x98: /* cbw/cwde/cdqe */
> 1985             switch ( op_bytes )
> 1986             {
>>>>     CID 1532318:  Memory - corruptions  (OVERLAPPING_COPY)
>>>>     Assigning "_regs.al" to "_regs.ax", which have overlapping memory locations and different types.
> 1987             case 2: _regs.ax = (int8_t)_regs.al; break; /* cbw */

I was under the impression that reading and then writing different parts
of the same union was permitted, even without -fno-strict-aliasing. Am I
missing anything here that Coverity knows better?

> *** CID 1532317:  Insecure data handling  (TAINTED_SCALAR)
> /tools/libs/guest/xg_dom_bzimageloader.c: 574 in xc_try_zstd_decode()
> 568         if ( xc_dom_kernel_check_size(dom, outsize) )
> 569         {
> 570             DOMPRINTF("ZSTD: output too large");
> 571             return -1;
> 572         }
> 573     
>>>>     CID 1532317:  Insecure data handling  (TAINTED_SCALAR)
>>>>     Passing tainted expression "outsize" to "malloc", which uses it as an allocation size.
> 574         outbuf = malloc(outsize);
> 575         if ( !outbuf )
> 576         {
> 577             DOMPRINTF("ZSTD: failed to alloc memory");
> 578             return -1;
> 579         }

I'm afraid I simply don't know what "tainted expression" here means.
xc_dom_kernel_check_size() certainly applies an upper bound ...

> *** CID 1532309:  Control flow issues  (DEADCODE)
> /tools/ocaml/libs/xc/xenctrl_stubs.c: 840 in physinfo_arch_caps()
> 834     
> 835     	arch_obj = Tag_cons;
> 836     
> 837     #endif
> 838     
> 839     	if ( tag < 0 )
>>>>     CID 1532309:  Control flow issues  (DEADCODE)
>>>>     Execution cannot reach this statement: "caml_failwith("Unhandled ar...".
> 840     		caml_failwith("Unhandled architecture");
> 841     
> 842     	arch_cap_flags = caml_alloc_small(1, tag);
> 843     	Store_field(arch_cap_flags, 0, arch_obj);
> 844     
> 845     	CAMLreturn(arch_cap_flags);

I think this wants to be left as is, not matter that Coverity complains.

For various of the UNUSED reports I'll send patches once having tested
them at least lightly.

Jan

Re: New Defects reported by Coverity Scan for XenProject

[Andrew Cooper]

On 12/06/2023 11:54 am, Jan Beulich wrote:
> On 11.06.2023 12:07, scan-admin@coverity.com wrote:
>> *** CID 1532318:  Memory - corruptions  (OVERLAPPING_COPY)
>> /tools/firmware/xen-dir/xen-root/xen/arch/x86/x86_emulate/x86_emulate.c: 1987 in x86_emulate()
>> 1981             dst.val  = *dst.reg;
>> 1982             goto xchg;
>> 1983     
>> 1984         case 0x98: /* cbw/cwde/cdqe */
>> 1985             switch ( op_bytes )
>> 1986             {
>>>>>     CID 1532318:  Memory - corruptions  (OVERLAPPING_COPY)
>>>>>     Assigning "_regs.al" to "_regs.ax", which have overlapping memory locations and different types.
>> 1987             case 2: _regs.ax = (int8_t)_regs.al; break; /* cbw */
> I was under the impression that reading and then writing different parts
> of the same union was permitted, even without -fno-strict-aliasing. Am I
> missing anything here that Coverity knows better?

It's permitted (hence why it compiles), and it's almost always a bug
(hence why Coverity complains).

In this case it's intentional to sign extend %al to %ax.

>
>> *** CID 1532317:  Insecure data handling  (TAINTED_SCALAR)
>> /tools/libs/guest/xg_dom_bzimageloader.c: 574 in xc_try_zstd_decode()
>> 568         if ( xc_dom_kernel_check_size(dom, outsize) )
>> 569         {
>> 570             DOMPRINTF("ZSTD: output too large");
>> 571             return -1;
>> 572         }
>> 573     
>>>>>     CID 1532317:  Insecure data handling  (TAINTED_SCALAR)
>>>>>     Passing tainted expression "outsize" to "malloc", which uses it as an allocation size.
>> 574         outbuf = malloc(outsize);
>> 575         if ( !outbuf )
>> 576         {
>> 577             DOMPRINTF("ZSTD: failed to alloc memory");
>> 578             return -1;
>> 579         }
> I'm afraid I simply don't know what "tainted expression" here means.
> xc_dom_kernel_check_size() certainly applies an upper bound ...

"tainted" is Coverity-speak for "externally-provided value not sanitised
yet".

I suspect that Coverity has failed to equate xc_dom_kernel_check_size()
to being a bounds check on outsize.

>
>> *** CID 1532309:  Control flow issues  (DEADCODE)
>> /tools/ocaml/libs/xc/xenctrl_stubs.c: 840 in physinfo_arch_caps()
>> 834     
>> 835     	arch_obj = Tag_cons;
>> 836     
>> 837     #endif
>> 838     
>> 839     	if ( tag < 0 )
>>>>>     CID 1532309:  Control flow issues  (DEADCODE)
>>>>>     Execution cannot reach this statement: "caml_failwith("Unhandled ar...".
>> 840     		caml_failwith("Unhandled architecture");
>> 841     
>> 842     	arch_cap_flags = caml_alloc_small(1, tag);
>> 843     	Store_field(arch_cap_flags, 0, arch_obj);
>> 844     
>> 845     	CAMLreturn(arch_cap_flags);
> I think this wants to be left as is, not matter that Coverity complains.

Yeah, this is deliberately too.  It's there to prevent other accidents
like we had last week with the bindings.

~Andrew

Re: New Defects reported by Coverity Scan for XenProject

[Jan Beulich]

On 24.01.2021 11:35, scan-admin@coverity.com wrote:
> *** CID 1472394:  Concurrent data access violations  (MISSING_LOCK)
> /xen/drivers/passthrough/x86/hvm.c: 1054 in pci_clean_dpci_irq()
> 1048         list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
> 1049         {
> 1050             list_del(&digl->list);
> 1051             xfree(digl);
> 1052         }
> 1053         /* Note the pirq is now unbound. */
>>>>     CID 1472394:  Concurrent data access violations  (MISSING_LOCK)
>>>>     Accessing "pirq_dpci->flags" without holding lock "domain.event_lock". Elsewhere, "hvm_pirq_dpci.flags" is accessed with "domain.event_lock" held 10 out of 11 times.
> 1054         pirq_dpci->flags = 0;
> 1055     
> 1056         return pt_pirq_softirq_active(pirq_dpci) ? -ERESTART : 0;
> 1057     }

The only (indirect) caller of this function is ...

> 1059     int arch_pci_clean_pirqs(struct domain *d)

... this one, which very clearly acquires the lock in question.
Does anyone have any idea what misleads Coverity here in its
conclusion, and hence possibly what may silence this?

Jan

Re: New Defects reported by Coverity Scan for XenProject

[Ian Campbell]

On Wed, 2016-02-24 at 21:02 -0800, scan-admin@coverity.com wrote:
> Hi,
> 
> Please find the latest report on new defect(s) introduced to XenProject found with Coverity Scan.
> 
> 2 new defect(s) introduced to XenProject found with Coverity Scan.
> 12 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.
> 
> New defect(s) Reported-by: Coverity Scan
> Showing 2 of 2 defect(s)
> 
> 
> ** CID 1354244:  Null pointer dereferences  (FORWARD_NULL)
> /tools/libxc/xc_tbuf.c: 72 in xc_tbuf_get_size()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 1354244:  Null pointer dereferences  (FORWARD_NULL)
> /tools/libxc/xc_tbuf.c: 72 in xc_tbuf_get_size()
> 66             return rc;
> 67     
> 68         t_info = xc_map_foreign_range(xch, DOMID_XEN,
> 69                         sysctl.u.tbuf_op.size, PROT_READ | PROT_WRITE,
> 70                         sysctl.u.tbuf_op.buffer_mfn);
> 71     
> > > >     CID 1354244:  Null pointer dereferences  (FORWARD_NULL)
> > > >     Comparing "t_info" to null implies that "t_info" might be null.
> 72         if ( t_info == NULL || t_info->tbuf_size == 0 )
> 73             rc = -1;
> 74         else
> 75     	*size = t_info->tbuf_size;
> 76     
> 77         xenforeignmemory_unmap(xch->fmem, t_info, sysctl.u.tbuf_op.size);

This is complaining about the eventual munmap(t_info) => munmap(NULL) which
is behind xenforeignmemory_unmap().

Looks like it was newly added by the fix to CID 1351228 in 7c479883b04a
("libxc: fix leak of t_info in xc_tbuf_get_size()").
xenforeignmemory_unmap() should behave like munmap WRT tollerance of NULL,
I'm not 100% sure what that behaviour is since 0 is a valid address.
xenforeignmemory.h no doubt wants updating with the desired semantics and
either this code of the implementation adjusting to match.

While here I notice that using xc_map_*() to create the mapping and
xenforeignmemory_unmap() to destroy it is a bit odd since they are strictly
two separate APIs, even if one happens to be implemented in terms of the
other. Being libxc internal this code is at liberty to use xc_map_* but
should then use plain munmap to undo it, or it would also be reasonable to
port this code fully to the xenforeignmemory interface.

> 
> ** CID 1354243:  Control flow issues  (DEADCODE)
> /tools/xentrace/xenalyze.c: 4148 in cr3_dump_list()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 1354243:  Control flow issues  (DEADCODE)
> /tools/xentrace/xenalyze.c: 4148 in cr3_dump_list()
> 4142     
> 4143         /* Count the number of elements */
> 4144         for(p=head; p; p=p->next)
> 4145             N++;
> 4146     
> 4147         if(!N)
> > > >     CID 1354243:  Control flow issues  (DEADCODE)
> > > >     Execution cannot reach this statement: "return;".

Here it has observed that due to the (above, just out of the context given
here) "if (!head) return" that the for loop must run at least once, so N
cannot be 0.

My guess is that this is a prexisting issue which was exposed to coverities
beady eye somehow by 28ab9f3d0e7c ("tools/xenalyze: Fix build with clang").
Or maybe this was previous marked deliberate but the change has caused
coverity to think this is a different instance of the same thing, eitherway
I don't think the issue itself is new.

FWIW having both if (!head) return and if (!N) return looks redundant to
me, the other two similar looking instances (from grepping for N++) have
only the latter check.

Ian.

> 4148             return;
> 4149     
> 4150         /* Alloc a struct of the right size */
> 4151         qsort_array = malloc(N * sizeof(struct eip_list_struct *));
> 4152     
> 4153         /* Point the array into it */
> 
> 
> ________________________________________________________________________________________________________
> To view the defects in Coverity Scan visit, https://scan.coverity.com/projects/xenproject?tab=overview
> 
> To manage Coverity Scan email notifications for "ian.campbell@citrix.com", click https://scan.coverity.com/subscriptions/edit?email=ian.campbell%40citrix.com&token=86eecf9a70a32d8ef6ac41e4c7cdaf58
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: New Defects reported by Coverity Scan for XenProject

[George Dunlap]

On 25/02/16 10:00, Ian Campbell wrote:
> 
> On Wed, 2016-02-24 at 21:02 -0800, scan-admin@coverity.com wrote:
>> Hi,
>>
>> Please find the latest report on new defect(s) introduced to XenProject found with Coverity Scan.
>>
>> 2 new defect(s) introduced to XenProject found with Coverity Scan.
>> 12 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.
>>
>> New defect(s) Reported-by: Coverity Scan
>> Showing 2 of 2 defect(s)
>>
>>
>> ** CID 1354244:  Null pointer dereferences  (FORWARD_NULL)
>> /tools/libxc/xc_tbuf.c: 72 in xc_tbuf_get_size()
>>
>>
>> ________________________________________________________________________________________________________
>> *** CID 1354244:  Null pointer dereferences  (FORWARD_NULL)
>> /tools/libxc/xc_tbuf.c: 72 in xc_tbuf_get_size()
>> 66             return rc;
>> 67     
>> 68         t_info = xc_map_foreign_range(xch, DOMID_XEN,
>> 69                         sysctl.u.tbuf_op.size, PROT_READ | PROT_WRITE,
>> 70                         sysctl.u.tbuf_op.buffer_mfn);
>> 71     
>>>>>     CID 1354244:  Null pointer dereferences  (FORWARD_NULL)
>>>>>     Comparing "t_info" to null implies that "t_info" might be null.
>> 72         if ( t_info == NULL || t_info->tbuf_size == 0 )
>> 73             rc = -1;
>> 74         else
>> 75     	*size = t_info->tbuf_size;
>> 76     
>> 77         xenforeignmemory_unmap(xch->fmem, t_info, sysctl.u.tbuf_op.size);
> 
> This is complaining about the eventual munmap(t_info) => munmap(NULL) which
> is behind xenforeignmemory_unmap().
> 
> Looks like it was newly added by the fix to CID 1351228 in 7c479883b04a
> ("libxc: fix leak of t_info in xc_tbuf_get_size()").
> xenforeignmemory_unmap() should behave like munmap WRT tollerance of NULL,
> I'm not 100% sure what that behaviour is since 0 is a valid address.
> xenforeignmemory.h no doubt wants updating with the desired semantics and
> either this code of the implementation adjusting to match.
> 
> While here I notice that using xc_map_*() to create the mapping and
> xenforeignmemory_unmap() to destroy it is a bit odd since they are strictly
> two separate APIs, even if one happens to be implemented in terms of the
> other. Being libxc internal this code is at liberty to use xc_map_* but
> should then use plain munmap to undo it, or it would also be reasonable to
> port this code fully to the xenforeignmemory interface.
> 
>>
>> ** CID 1354243:  Control flow issues  (DEADCODE)
>> /tools/xentrace/xenalyze.c: 4148 in cr3_dump_list()
>>
>>
>> ________________________________________________________________________________________________________
>> *** CID 1354243:  Control flow issues  (DEADCODE)
>> /tools/xentrace/xenalyze.c: 4148 in cr3_dump_list()
>> 4142     
>> 4143         /* Count the number of elements */
>> 4144         for(p=head; p; p=p->next)
>> 4145             N++;
>> 4146     
>> 4147         if(!N)
>>>>>     CID 1354243:  Control flow issues  (DEADCODE)
>>>>>     Execution cannot reach this statement: "return;".
> 
> Here it has observed that due to the (above, just out of the context given
> here) "if (!head) return" that the for loop must run at least once, so N
> cannot be 0.
> 
> My guess is that this is a prexisting issue which was exposed to coverities
> beady eye somehow by 28ab9f3d0e7c ("tools/xenalyze: Fix build with clang").
> Or maybe this was previous marked deliberate but the change has caused
> coverity to think this is a different instance of the same thing, eitherway
> I don't think the issue itself is new.
> 
> FWIW having both if (!head) return and if (!N) return looks redundant to
> me, the other two similar looking instances (from grepping for N++) have
> only the latter check.

Yes, they're certainly redundant, and I definitely prefer the latter
check rather than the former.  I'll send a patch.

 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: New Defects reported by Coverity Scan for XenProject

[Ian Campbell]

Hi Charles,

I'm not sure if this is a real issue in the qdisk support for xenstat,
but I think it may relate to the first return 0 in qmp_read which may
not free the accumulated array.

Could you take a look please?

Ian.

On Wed, 2015-04-01 at 05:51 -0700, scan-admin@coverity.com wrote:
> Hi,
> 
> Please find the latest report on new defect(s) introduced to XenProject found with Coverity Scan.
> 
> 1 new defect(s) introduced to XenProject found with Coverity Scan.
> 2 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.
> 
> New defect(s) Reported-by: Coverity Scan
> Showing 1 of 1 defect(s)
> 
> 
> ** CID 1292691:  Resource leaks  (RESOURCE_LEAK)
> /tools/xenstat/libxenstat/src/xenstat_qmp.c: 327 in qmp_query()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 1292691:  Resource leaks  (RESOURCE_LEAK)
> /tools/xenstat/libxenstat/src/xenstat_qmp.c: 327 in qmp_query()
> 321     	int n;
> 322     
> 323     	n = strlen(cmd);
> 324     	if (qmp_write(qfd, cmd, n) != n)
> 325     		return NULL;
> 326     	if (!qmp_read(qfd, &qstats))
> >>>     CID 1292691:  Resource leaks  (RESOURCE_LEAK)
> >>>     Variable "qstats" going out of scope leaks the storage it points to.
> 327     		return NULL;
> 328     	return qstats;
> 329     }
> 330     
> 331     /* Returns a socket connected to the QMP socket. Returns -1 on failure. */
> 332     static int qmp_connect(char *path)
> 
> 
> ________________________________________________________________________________________________________
> To view the defects in Coverity Scan visit, https://scan.coverity.com/projects/606?tab=overview
> 
> To manage Coverity Scan email notifications for "ian.campbell@citrix.com", click https://scan.coverity.com/subscriptions/edit?email=ian.campbell%40citrix.com&token=1ce0fc428b9f94f66fd8d1ecf6cbb76a .
> 

Re: New Defects reported by Coverity Scan for XenProject

[Charles Arnold]

>>> On 4/2/2015 at 08:32 AM, Ian Campbell <ian.campbell@citrix.com> wrote: 
> Hi Charles,
> 
> I'm not sure if this is a real issue in the qdisk support for xenstat,
> but I think it may relate to the first return 0 in qmp_read which may
> not free the accumulated array.

Yes. That is a leak on the failure case.

> 
> Could you take a look please?

Patch sent, Thanks

- Charles

> 
> Ian.
> 
> On Wed, 2015-04-01 at 05:51 -0700, scan-admin@coverity.com wrote:
>> Hi,
>> 
>> Please find the latest report on new defect(s) introduced to XenProject 
> found with Coverity Scan.
>> 
>> 1 new defect(s) introduced to XenProject found with Coverity Scan.
>> 2 defect(s), reported by Coverity Scan earlier, were marked fixed in the 
> recent build analyzed by Coverity Scan.
>> 
>> New defect(s) Reported-by: Coverity Scan
>> Showing 1 of 1 defect(s)
>> 
>> 
>> ** CID 1292691:  Resource leaks  (RESOURCE_LEAK)
>> /tools/xenstat/libxenstat/src/xenstat_qmp.c: 327 in qmp_query()
>> 
>> 
>> 
> _____________________________________________________________________________
> ___________________________
>> *** CID 1292691:  Resource leaks  (RESOURCE_LEAK)
>> /tools/xenstat/libxenstat/src/xenstat_qmp.c: 327 in qmp_query()
>> 321     	int n;
>> 322     
>> 323     	n = strlen(cmd);
>> 324     	if (qmp_write(qfd, cmd, n) != n)
>> 325     		return NULL;
>> 326     	if (!qmp_read(qfd, &qstats))
>> >>>     CID 1292691:  Resource leaks  (RESOURCE_LEAK)
>> >>>     Variable "qstats" going out of scope leaks the storage it points to.
>> 327     		return NULL;
>> 328     	return qstats;
>> 329     }
>> 330     
>> 331     /* Returns a socket connected to the QMP socket. Returns -1 on 
> failure. */
>> 332     static int qmp_connect(char *path)
>> 
>> 
>> 
> _____________________________________________________________________________
> ___________________________
>> To view the defects in Coverity Scan visit, 
> https://scan.coverity.com/projects/606?tab=overview
>> 
>> To manage Coverity Scan email notifications for "ian.campbell@citrix.com", 
> click 
> https://scan.coverity.com/subscriptions/edit?email=ian.campbell%40citrix.com&t
> oken=1ce0fc428b9f94f66fd8d1ecf6cbb76a .
>> 

Re: New Defects reported by Coverity Scan for XenProject

[Ian Campbell]

Hi David,

Coverity picked up some issues in the kexec patches. At least the
locking one looks valid to me...

I didn't investigate the endianness one.

Ian.

On Wed, 2013-11-13 at 05:34 -0800, scan-admin@coverity.com wrote:
> ________________________________________________________________________
> CID 1128573: Missing unlock (LOCK)
> 
> /xen/common/kexec.c: 788 ( lock)
>    785    
>    786        *old = NULL;
>    787    
> >>> "_spin_lock(spinlock_t *)" locks "kexec_lock".
>    788        spin_lock(&kexec_lock);
>    789    
>    790        if ( test_bit(KEXEC_FLAG_IN_PROGRESS, &kexec_flags) )
>    791        {
>    792            spin_unlock(&kexec_lock);
>   
> 
> /xen/common/kexec.c: 797 ( missing_unlock)
>    794        }
>    795    
>    796        if ( kexec_load_get_bits(type, &base, &bit) )
> >>> CID 1128573: Missing unlock (LOCK)
> >>> Returning without unlocking "kexec_lock".
>    797            return -EINVAL;
>    798    
>    799        pos = (test_bit(bit, &kexec_flags) != 0);
>    800        old_slot = base + pos;
>    801        new_slot = base + !pos;
>   
> ________________________________________________________________________
> CID 1128572: Reliance on integer endianness (INCOMPATIBLE_CAST)
> 
> /xen/arch/x86/machine_kexec.c: 58 ( incompatible_cast)
>    55            l3_page = kimage_alloc_control_page(image, 0);
>    56            if ( !l3_page )
>    57                goto out;
> >>> CID 1128572: Reliance on integer endianness (INCOMPATIBLE_CAST)
> >>> Pointer "&l4->l4" points to an object whose effective type is "unsigned long" (64 bits, unsigned) but is dereferenced as a narrower "unsigned int" (32 bits, unsigned).  This may lead to unexpected results depending on machine endianness.
>    58            l4e_write(l4, l4e_from_page(l3_page, __PAGE_HYPERVISOR));
>    59        }
>    60        else
>    61            l3_page = l4e_get_page(*l4);
>    62    
>   
> ________________________________________________________________________
> CID 1128571: Reliance on integer endianness (INCOMPATIBLE_CAST)
> 
> /xen/arch/x86/machine_kexec.c: 70 ( incompatible_cast)
>    67            l2_page = kimage_alloc_control_page(image, 0);
>    68            if ( !l2_page )
>    69                goto out;
> >>> CID 1128571: Reliance on integer endianness (INCOMPATIBLE_CAST)
> >>> Pointer "&l3->l3" points to an object whose effective type is "unsigned long" (64 bits, unsigned) but is dereferenced as a narrower "unsigned int" (32 bits, unsigned).  This may lead to unexpected results depending on machine endianness.
>    70            l3e_write(l3, l3e_from_page(l2_page, __PAGE_HYPERVISOR));
>    71        }
>    72        else
>    73            l2_page = l3e_get_page(*l3);
>    74    
>   
> ________________________________________________________________________
> To view the defects in Coverity Scan visit, http://scan.coverity.com
> 
> To unsubscribe from the email notification for new defects, http://scan5.coverity.com/cgi-bin/unsubscribe.py

Re: New Defects reported by Coverity Scan for XenProject

[David Vrabel]

On 13/11/13 13:51, Ian Campbell wrote:
> Hi David,
> 
> Coverity picked up some issues in the kexec patches. At least the
> locking one looks valid to me...

Thanks.  Andy just pointed them out to me as well.

> I didn't investigate the endianness one.

We believe these are coverity being confused by the underlying
atomic_read()/atomic_write() macros and getting the type wrong.

David

> On Wed, 2013-11-13 at 05:34 -0800, scan-admin@coverity.com wrote:
>> ________________________________________________________________________
>> CID 1128573: Missing unlock (LOCK)
>>
>> /xen/common/kexec.c: 788 ( lock)
>>    785    
>>    786        *old = NULL;
>>    787    
>>>>> "_spin_lock(spinlock_t *)" locks "kexec_lock".
>>    788        spin_lock(&kexec_lock);
>>    789    
>>    790        if ( test_bit(KEXEC_FLAG_IN_PROGRESS, &kexec_flags) )
>>    791        {
>>    792            spin_unlock(&kexec_lock);
>>   
>>
>> /xen/common/kexec.c: 797 ( missing_unlock)
>>    794        }
>>    795    
>>    796        if ( kexec_load_get_bits(type, &base, &bit) )
>>>>> CID 1128573: Missing unlock (LOCK)
>>>>> Returning without unlocking "kexec_lock".
>>    797            return -EINVAL;
>>    798    
>>    799        pos = (test_bit(bit, &kexec_flags) != 0);
>>    800        old_slot = base + pos;
>>    801        new_slot = base + !pos;
>>   
>> ________________________________________________________________________
>> CID 1128572: Reliance on integer endianness (INCOMPATIBLE_CAST)
>>
>> /xen/arch/x86/machine_kexec.c: 58 ( incompatible_cast)
>>    55            l3_page = kimage_alloc_control_page(image, 0);
>>    56            if ( !l3_page )
>>    57                goto out;
>>>>> CID 1128572: Reliance on integer endianness (INCOMPATIBLE_CAST)
>>>>> Pointer "&l4->l4" points to an object whose effective type is "unsigned long" (64 bits, unsigned) but is dereferenced as a narrower "unsigned int" (32 bits, unsigned).  This may lead to unexpected results depending on machine endianness.
>>    58            l4e_write(l4, l4e_from_page(l3_page, __PAGE_HYPERVISOR));
>>    59        }
>>    60        else
>>    61            l3_page = l4e_get_page(*l4);
>>    62    
>>   
>> ________________________________________________________________________
>> CID 1128571: Reliance on integer endianness (INCOMPATIBLE_CAST)
>>
>> /xen/arch/x86/machine_kexec.c: 70 ( incompatible_cast)
>>    67            l2_page = kimage_alloc_control_page(image, 0);
>>    68            if ( !l2_page )
>>    69                goto out;
>>>>> CID 1128571: Reliance on integer endianness (INCOMPATIBLE_CAST)
>>>>> Pointer "&l3->l3" points to an object whose effective type is "unsigned long" (64 bits, unsigned) but is dereferenced as a narrower "unsigned int" (32 bits, unsigned).  This may lead to unexpected results depending on machine endianness.
>>    70            l3e_write(l3, l3e_from_page(l2_page, __PAGE_HYPERVISOR));
>>    71        }
>>    72        else
>>    73            l2_page = l3e_get_page(*l3);
>>    74    
>>   
>> ________________________________________________________________________
>> To view the defects in Coverity Scan visit, http://scan.coverity.com