[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: New Defects reported by Coverity Scan for XenProject


  • To: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 12 Jun 2023 12:54:27 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=3jm9gLQLuVaj5UWJ70SJbkW9YAKVsO0ndI4ArXyURKs=; b=nsG4NsL+auZo+TG/V+0CF1YrfunId8rvtG9NdlwjepntEXa51DLaP/arSJgNaK+6tIw17QPePZlYl+108kJuClpSOVdp4NxSRrmO9IRUgIYcd2MJpU9hXZ5pVf2hdSshdUtl4glw++YF/BcBf+Tqf8UxOP37EqCqRnXDOQLj3sKrGBcNMPFPWSr+ssddtpGyN5e+h8YsXd1LIDUucGBVSZjmrUkBfzSsm2dVjBMBWoy0dg5L4n8F3AWZoNIL58XxWVfZg4XectswfEL8LUX72cGSkdBsFCFoYANwHRrs3WsQd9yXWrGAqazndnd/PqQti3j4N7wklRKlmhTt9US+Qg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=hmkudCXnqUSdNV1oow6HsfKb8kVJSCP/PVl0b8cHzIp1pxYYvkPYE93RYrcuaqW6zZx2nLZvo3pNijyNSqJ5jnbMlW7zvYVFzEJpGCqs0vFU9go41cWKQipIgzNFwGNxrtru4Kmi4G18vM3mDTmOOkylfLUPZRgdcJwYAKWatGsPIT6sRhVfOyV15TwkYWjhEhjK/U3YE0sKsUt6CbdPM4UPlCPjpTKUJixOPPQSFpxEKGJTRlbKnttpCfFECmM1tAoDRnoppEvh/jCI6Xq+FseqkVlebDniRq7WAxvKJx68KsOkYJLpHw0PglKIjZanZhFGuIlJhUhqD3F0LDgtnw==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Delivery-date: Mon, 12 Jun 2023 10:54:34 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.