Re: New Defects reported by Coverity Scan for XenProject

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.06.2023 12:07, scan-admin@xxxxxxxxxxxx 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