Re: New Defects reported by Coverity Scan for XenProject
On 22/05/2024 11:05 am, Jan Beulich wrote: > On 22.05.2024 11:56, scan-ad...@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(_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
On 22.05.2024 11:56, scan-ad...@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(_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), ); > 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
On 05.05.2024 11:54, scan-ad...@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: Fwd: New Defects reported by Coverity Scan for XenProject
On 28/02/2024 1:26 pm, Roger Pau Monné wrote: > On Wed, Feb 28, 2024 at 02:04:23PM +0100, Jan Beulich wrote: >> On 28.02.2024 13:19, Andrew Cooper wrote: >>> Take 2, hopefully with Stewart's correct email address this time. >>> >>> ~Andrew >>> >>> On 28/02/2024 12:17 pm, Andrew Cooper wrote: Not sure how well this is going to be formatted, but there's one new and potentially interesting issue found by Coverity. >> To be honest I didn't consider this interesting at all, but instead a false >> positive due to limited insight that the tool has. But maybe I was wrong >> and you see something I didn't see? vpci_process_pending() is vCPU-local >> (run from the guest resume path), and hence there simply are no two threads >> who want to look at the field. Storing NULL into it is merely a kind of >> progress indicator, relevant to the given vCPU only. > Indeed, there's no (intended) way for vpci_process_pending() to be > executed concurrently against the same vcpu parameter, and hence there > should be no concurrency hazards. Setting it to NULL is a mere > indication there's no further work to be done, and the vCPU can resume > guest context execution. > > defer_map() (the function that queues the work to be done) can only be > reached as a result of a guest accessing the PCI config space, so if > the vCPU is not running defer_map() can't be called either for that > vCPU. Fine. So long as someone has looked into it and thinks we're fine. ~Andrew
Re: Fwd: New Defects reported by Coverity Scan for XenProject
On Wed, Feb 28, 2024 at 02:04:23PM +0100, Jan Beulich wrote: > On 28.02.2024 13:19, Andrew Cooper wrote: > > Take 2, hopefully with Stewart's correct email address this time. > > > > ~Andrew > > > > On 28/02/2024 12:17 pm, Andrew Cooper wrote: > >> Not sure how well this is going to be formatted, but there's one new and > >> potentially interesting issue found by Coverity. > > To be honest I didn't consider this interesting at all, but instead a false > positive due to limited insight that the tool has. But maybe I was wrong > and you see something I didn't see? vpci_process_pending() is vCPU-local > (run from the guest resume path), and hence there simply are no two threads > who want to look at the field. Storing NULL into it is merely a kind of > progress indicator, relevant to the given vCPU only. Indeed, there's no (intended) way for vpci_process_pending() to be executed concurrently against the same vcpu parameter, and hence there should be no concurrency hazards. Setting it to NULL is a mere indication there's no further work to be done, and the vCPU can resume guest context execution. defer_map() (the function that queues the work to be done) can only be reached as a result of a guest accessing the PCI config space, so if the vCPU is not running defer_map() can't be called either for that vCPU. Thanks, Roger.
Re: Fwd: New Defects reported by Coverity Scan for XenProject
On 28.02.2024 13:19, Andrew Cooper wrote:
> Take 2, hopefully with Stewart's correct email address this time.
>
> ~Andrew
>
> On 28/02/2024 12:17 pm, Andrew Cooper wrote:
>> Not sure how well this is going to be formatted, but there's one new and
>> potentially interesting issue found by Coverity.
To be honest I didn't consider this interesting at all, but instead a false
positive due to limited insight that the tool has. But maybe I was wrong
and you see something I didn't see? vpci_process_pending() is vCPU-local
(run from the guest resume path), and hence there simply are no two threads
who want to look at the field. Storing NULL into it is merely a kind of
progress indicator, relevant to the given vCPU only.
Jan
>> 8<
>>
>> New defect(s) Reported-by: Coverity Scan
>> Showing 1 of 1 defect(s)
>>
>>
>> ** CID 1592633: (LOCK_EVASION)
>> /xen/drivers/vpci/header.c: 229 in vpci_process_pending()
>> /xen/drivers/vpci/header.c: 189 in vpci_process_pending()
>> /xen/drivers/vpci/header.c: 239 in vpci_process_pending()
>>
>>
>>
>> *** CID 1592633: (LOCK_EVASION)
>> /xen/drivers/vpci/header.c: 229 in vpci_process_pending()
>> 223 224 /* Clean all the rangesets */
>> 225 for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
>> 226 if ( !rangeset_is_empty(header->bars[i].mem) )
>> 227 rangeset_purge(header->bars[i].mem);
>> 228
> CID 1592633: (LOCK_EVASION)
> Thread1 sets "pdev" to a new value. Now the two threads have an
> inconsistent view of "pdev" and updates to fields of "pdev" or
> fields correlated with "pdev" may be lost.
>> 229 v->vpci.pdev = NULL;
>> 230 231 read_unlock(>domain->pci_lock);
>> 232 233 if ( !is_hardware_domain(v->domain) )
>> 234 domain_crash(v->domain);
>> /xen/drivers/vpci/header.c: 189 in vpci_process_pending()
>> 183 return false;
>> 184 185 read_lock(>domain->pci_lock);
>> 186 187 if ( !pdev->vpci || (v->domain != pdev->domain) )
>> 188 {
> CID 1592633: (LOCK_EVASION)
> Thread1 sets "pdev" to a new value. Now the two threads have an
> inconsistent view of "pdev" and updates to fields of "pdev" or
> fields correlated with "pdev" may be lost.
>> 189 v->vpci.pdev = NULL;
>> 190 read_unlock(>domain->pci_lock);
>> 191 return false;
>> 192 }
>> 193 194 header = >vpci->header;
>> /xen/drivers/vpci/header.c: 239 in vpci_process_pending()
>> 233 if ( !is_hardware_domain(v->domain) )
>> 234 domain_crash(v->domain);
>> 235 236 return false;
>> 237 }
>> 238 }
> CID 1592633: (LOCK_EVASION)
> Thread1 sets "pdev" to a new value. Now the two threads have an
> inconsistent view of "pdev" and updates to fields of "pdev" or
> fields correlated with "pdev" may be lost.
>> 239 v->vpci.pdev = NULL;
>> 240 241 spin_lock(>vpci->lock);
>> 242 modify_decoding(pdev, v->vpci.cmd, v->vpci.rom_only);
>> 243 spin_unlock(>vpci->lock);
>> 244
>>
>
Re: Fwd: New Defects reported by Coverity Scan for XenProject
Take 2, hopefully with Stewart's correct email address this time.
~Andrew
On 28/02/2024 12:17 pm, Andrew Cooper wrote:
> Not sure how well this is going to be formatted, but there's one new and
> potentially interesting issue found by Coverity.
>
> ~Andrew
>
> 8<
>
> New defect(s) Reported-by: Coverity Scan
> Showing 1 of 1 defect(s)
>
>
> ** CID 1592633: (LOCK_EVASION)
> /xen/drivers/vpci/header.c: 229 in vpci_process_pending()
> /xen/drivers/vpci/header.c: 189 in vpci_process_pending()
> /xen/drivers/vpci/header.c: 239 in vpci_process_pending()
>
>
>
> *** CID 1592633: (LOCK_EVASION)
> /xen/drivers/vpci/header.c: 229 in vpci_process_pending()
> 223 224 /* Clean all the rangesets */
> 225 for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
> 226 if ( !rangeset_is_empty(header->bars[i].mem) )
> 227 rangeset_purge(header->bars[i].mem);
> 228
CID 1592633: (LOCK_EVASION)
Thread1 sets "pdev" to a new value. Now the two threads have an
inconsistent view of "pdev" and updates to fields of "pdev" or
fields correlated with "pdev" may be lost.
> 229 v->vpci.pdev = NULL;
> 230 231 read_unlock(>domain->pci_lock);
> 232 233 if ( !is_hardware_domain(v->domain) )
> 234 domain_crash(v->domain);
> /xen/drivers/vpci/header.c: 189 in vpci_process_pending()
> 183 return false;
> 184 185 read_lock(>domain->pci_lock);
> 186 187 if ( !pdev->vpci || (v->domain != pdev->domain) )
> 188 {
CID 1592633: (LOCK_EVASION)
Thread1 sets "pdev" to a new value. Now the two threads have an
inconsistent view of "pdev" and updates to fields of "pdev" or
fields correlated with "pdev" may be lost.
> 189 v->vpci.pdev = NULL;
> 190 read_unlock(>domain->pci_lock);
> 191 return false;
> 192 }
> 193 194 header = >vpci->header;
> /xen/drivers/vpci/header.c: 239 in vpci_process_pending()
> 233 if ( !is_hardware_domain(v->domain) )
> 234 domain_crash(v->domain);
> 235 236 return false;
> 237 }
> 238 }
CID 1592633: (LOCK_EVASION)
Thread1 sets "pdev" to a new value. Now the two threads have an
inconsistent view of "pdev" and updates to fields of "pdev" or
fields correlated with "pdev" may be lost.
> 239 v->vpci.pdev = NULL;
> 240 241 spin_lock(>vpci->lock);
> 242 modify_decoding(pdev, v->vpci.cmd, v->vpci.rom_only);
> 243 spin_unlock(>vpci->lock);
> 244
>
Fwd: New Defects reported by Coverity Scan for XenProject
Not sure how well this is going to be formatted, but there's one new and
potentially interesting issue found by Coverity.
~Andrew
8<
New defect(s) Reported-by: Coverity Scan
Showing 1 of 1 defect(s)
** CID 1592633: (LOCK_EVASION)
/xen/drivers/vpci/header.c: 229 in vpci_process_pending()
/xen/drivers/vpci/header.c: 189 in vpci_process_pending()
/xen/drivers/vpci/header.c: 239 in vpci_process_pending()
*** CID 1592633: (LOCK_EVASION)
/xen/drivers/vpci/header.c: 229 in vpci_process_pending()
223 224 /* Clean all the rangesets */
225 for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
226 if ( !rangeset_is_empty(header->bars[i].mem) )
227 rangeset_purge(header->bars[i].mem);
228
>>> CID 1592633: (LOCK_EVASION)
>>> Thread1 sets "pdev" to a new value. Now the two threads have an
>>> inconsistent view of "pdev" and updates to fields of "pdev" or
>>> fields correlated with "pdev" may be lost.
229 v->vpci.pdev = NULL;
230 231 read_unlock(>domain->pci_lock);
232 233 if ( !is_hardware_domain(v->domain) )
234 domain_crash(v->domain);
/xen/drivers/vpci/header.c: 189 in vpci_process_pending()
183 return false;
184 185 read_lock(>domain->pci_lock);
186 187 if ( !pdev->vpci || (v->domain != pdev->domain) )
188 {
>>> CID 1592633: (LOCK_EVASION)
>>> Thread1 sets "pdev" to a new value. Now the two threads have an
>>> inconsistent view of "pdev" and updates to fields of "pdev" or
>>> fields correlated with "pdev" may be lost.
189 v->vpci.pdev = NULL;
190 read_unlock(>domain->pci_lock);
191 return false;
192 }
193 194 header = >vpci->header;
/xen/drivers/vpci/header.c: 239 in vpci_process_pending()
233 if ( !is_hardware_domain(v->domain) )
234 domain_crash(v->domain);
235 236 return false;
237 }
238 }
>>> CID 1592633: (LOCK_EVASION)
>>> Thread1 sets "pdev" to a new value. Now the two threads have an
>>> inconsistent view of "pdev" and updates to fields of "pdev" or
>>> fields correlated with "pdev" may be lost.
239 v->vpci.pdev = NULL;
240 241 spin_lock(>vpci->lock);
242 modify_decoding(pdev, v->vpci.cmd, v->vpci.rom_only);
243 spin_unlock(>vpci->lock);
244
Re: New Defects reported by Coverity Scan for XenProject
On 05.11.2023 10:58, scan-ad...@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",
> 97mfn_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 "jbeul...@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
On 12/06/2023 11:54 am, Jan Beulich wrote:
> On 11.06.2023 12:07, scan-ad...@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
On 11.06.2023 12:07, scan-ad...@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(_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) ? _obj->u.array : NULL".
> 220 for (i=0; ilen; i++) {
> 221 memset(, 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
Re: New Defects reported by Coverity Scan for XenProject
On 24.01.2021 11:35, scan-ad...@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, _dpci->digl_list,
> list )
> 1049 {
> 1050 list_del(>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
