Re: New Defects reported by Coverity Scan for XenProject

[Andrew Cooper <andrew.cooper3@citrix.com>]

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