Re: New Defects reported by Coverity Scan for QEMU

Re: New Defects reported by Coverity Scan for QEMU

[Dr. David Alan Gilbert]

(Resend with correct address)
Hi Dan, Leo,
  There are a few coverity warns from that last series:


Two moans about not checking mkdir in the tls tests:
> ** CID 1488871:  Error handling issues  (CHECKED_RETURN)
> /qemu/tests/qtest/migration-test.c: 782 in test_migrate_tls_x509_start_common()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 1488871:  Error handling issues  (CHECKED_RETURN)
> /qemu/tests/qtest/migration-test.c: 782 in test_migrate_tls_x509_start_common()
> 776         data->servercert = g_strdup_printf("%s/server-cert.pem", data->workdir);
> 777         if (args->clientcert) {
> 778             data->clientkey = g_strdup_printf("%s/client-key.pem", data->workdir);
> 779             data->clientcert = g_strdup_printf("%s/client-cert.pem", data->workdir);
> 780         }
> 781     
> >>>     CID 1488871:  Error handling issues  (CHECKED_RETURN)
> >>>     Calling "mkdir(data->workdir, 448U)" without checking return value. This library function may fail and return an error code.
> 782         mkdir(data->workdir, 0700);
> 783     
> 784         test_tls_init(data->keyfile);
> 785         g_assert(link(data->keyfile, data->serverkey) == 0);
> 786         if (args->clientcert) {
> 787             g_assert(link(data->keyfile, data->clientkey) == 0);
> 
> ** CID 1488870:    (CHECKED_RETURN)
> /qemu/tests/qtest/migration-test.c: 677 in test_migrate_tls_psk_start_common()
> /qemu/tests/qtest/migration-test.c: 670 in test_migrate_tls_psk_start_common()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 1488870:    (CHECKED_RETURN)
> /qemu/tests/qtest/migration-test.c: 677 in test_migrate_tls_psk_start_common()
> 671         test_tls_psk_init(data->pskfile);
> 672     
> 673         if (mismatch) {
> 674             data->workdiralt = g_strdup_printf("%s/tlscredspskalt0", tmpfs);
> 675             data->pskfilealt = g_strdup_printf("%s/%s", data->workdiralt,
> 676                                                QCRYPTO_TLS_CREDS_PSKFILE);
> >>>     CID 1488870:    (CHECKED_RETURN)
> >>>     Calling "mkdir(data->workdiralt, 448U)" without checking return value. This library function may fail and return an error code.
> 677             mkdir(data->workdiralt, 0700);
> 678             test_tls_psk_init_alt(data->pskfilealt);
> 679         }
> 680     
> 681         rsp = wait_command(from,
> 682                            "{ 'execute': 'object-add',"
> /qemu/tests/qtest/migration-test.c: 670 in test_migrate_tls_psk_start_common()
> 664             g_new0(struct TestMigrateTLSPSKData, 1);
> 665         QDict *rsp;
> 666     
> 667         data->workdir = g_strdup_printf("%s/tlscredspsk0", tmpfs);
> 668         data->pskfile = g_strdup_printf("%s/%s", data->workdir,
> 669                                         QCRYPTO_TLS_CREDS_PSKFILE);
> >>>     CID 1488870:    (CHECKED_RETURN)
> >>>     Calling "mkdir(data->workdir, 448U)" without checking return value. This library function may fail and return an error code.
> 670         mkdir(data->workdir, 0700);
> 671         test_tls_psk_init(data->pskfile);
> 672     
> 673         if (mismatch) {
> 674             data->workdiralt = g_strdup_printf("%s/tlscredspskalt0", tmpfs);
> 675             data->pskfilealt = g_strdup_printf("%s/%s", data->workdiralt,
> 
> ** CID 1488869:  Insecure data handling  (TAINTED_SCALAR)
> /qemu/io/channel-socket.c: 716 in qio_channel_socket_flush()



This one is more curious:
> *** CID 1488869:  Insecure data handling  (TAINTED_SCALAR)
> /qemu/io/channel-socket.c: 716 in qio_channel_socket_flush()
> 710         int ret = 1;
> 711     
> 712         msg.msg_control = control;
> 713         msg.msg_controllen = sizeof(control);
> 714         memset(control, 0, sizeof(control));
> 715     
> >>>     CID 1488869:  Insecure data handling  (TAINTED_SCALAR)
> >>>     Using tainted variable "sioc->zero_copy_sent" as a loop boundary.
> 716         while (sioc->zero_copy_sent < sioc->zero_copy_queued) {
> 717             received = recvmsg(sioc->fd, &msg, MSG_ERRQUEUE);
> 718             if (received < 0) {
> 719                 switch (errno) {
> 720                 case EAGAIN:
> 721                     /* Nothing on errqueue, wait until something is available */

it's not clear to me why it considers that 'insecure'; is that because
it's using values returned by the recvmsg ???

Dave

> 
> ** CID 1488868:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> /qemu/include/hw/cxl/cxl_component.h: 218 in cxl_decode_ig()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 1488868:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> /qemu/include/hw/cxl/cxl_component.h: 218 in cxl_decode_ig()
> 212     
> 213     uint8_t cxl_interleave_ways_enc(int iw, Error **errp);
> 214     uint8_t cxl_interleave_granularity_enc(uint64_t gran, Error **errp);
> 215     
> 216     static inline hwaddr cxl_decode_ig(int ig)
> 217     {
> >>>     CID 1488868:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> >>>     Potentially overflowing expression "1 << ig + 8" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "hwaddr" (64 bits, unsigned).
> 218         return 1 << (ig + 8);
> 219     }
> 220     
> 221     CXLComponentState *cxl_get_hb_cstate(PCIHostState *hb);
> 222     
> 
> 
> ________________________________________________________________________________________________________
> To view the defects in Coverity Scan visit, https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yrzEQNXe51mg-2FlKoEnRoarMq5nOxxfhqLUuo8HvG2S4Ew-3D-3DsJiM_-2BVwspb-2FvVsiDHi6TjJb1RCVMbxW4dUuL9sNVe8y5Hw33niByDzIZpGAOA5aYVSqv5jZRKaysoHO8HDAwcefdRpS6APFciD-2BwrlJOuA5BQE0BdpVQ-2F2N4H1eMXpy9YdBkXKlWx-2FEjNzp4PFxHatEl0DUHK-2BmMHOAPpvs5sC8wiJnoEK-2FOHDrJRemKeQ9jYmHtYSxFL21EDsvDKE-2FaIvXgh2BZ1DIuERrQlniBGfoVsYI-3D
> 
>   To manage Coverity Scan email notifications for "dgilbert@redhat.com", click https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yped04pjJnmXOsUBtKYNIXx81NaqhEuFta67QJjvrT4RaVMQaGq-2BvVMlKibSYlsRsVMlpoQjDNxxnuHxr4ePPs-2BGw9e2Rwvy7HI6fIypkgcFLOSiaVl1GR5WZgeKL5Lc28-3DX0rx_-2BVwspb-2FvVsiDHi6TjJb1RCVMbxW4dUuL9sNVe8y5Hw33niByDzIZpGAOA5aYVSqvTnKwL62mXPLveeP-2BWUfRx5fh6FkZ4ip8kt9FOWgTvKnwBEgRG9Hd6pRry4YHYry9Link-2B-2FJaxPuPjvtUPJC-2FjaH8m4iuyJBAq1vyM0bUUjuOwaUkIo9d-2F5qAkATC9CFkabYArjrgzBzYyi9I4oa04-2B1XLhr2wHE07h56XDN37Gw-3D
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: New Defects reported by Coverity Scan for QEMU

[Daniel P. Berrangé]

On Wed, May 18, 2022 at 09:15:12AM +0100, Dr. David Alan Gilbert wrote:
> Hi Dan, Leo,
>   There are a few coverity warns from that last series:
> 
> 
> Two moans about not checking mkdir in the tls tests:

Those mkdir()s can just be wrapped with an assert()

> > ** CID 1488871:  Error handling issues  (CHECKED_RETURN)
> > /qemu/tests/qtest/migration-test.c: 782 in test_migrate_tls_x509_start_common()
> > 
> > 
> > ________________________________________________________________________________________________________
> > *** CID 1488871:  Error handling issues  (CHECKED_RETURN)
> > /qemu/tests/qtest/migration-test.c: 782 in test_migrate_tls_x509_start_common()
> > 776         data->servercert = g_strdup_printf("%s/server-cert.pem", data->workdir);
> > 777         if (args->clientcert) {
> > 778             data->clientkey = g_strdup_printf("%s/client-key.pem", data->workdir);
> > 779             data->clientcert = g_strdup_printf("%s/client-cert.pem", data->workdir);
> > 780         }
> > 781     
> > >>>     CID 1488871:  Error handling issues  (CHECKED_RETURN)
> > >>>     Calling "mkdir(data->workdir, 448U)" without checking return value. This library function may fail and return an error code.
> > 782         mkdir(data->workdir, 0700);
> > 783     
> > 784         test_tls_init(data->keyfile);
> > 785         g_assert(link(data->keyfile, data->serverkey) == 0);
> > 786         if (args->clientcert) {
> > 787             g_assert(link(data->keyfile, data->clientkey) == 0);
> > 
> > ** CID 1488870:    (CHECKED_RETURN)
> > /qemu/tests/qtest/migration-test.c: 677 in test_migrate_tls_psk_start_common()
> > /qemu/tests/qtest/migration-test.c: 670 in test_migrate_tls_psk_start_common()
> > 
> > 
> > ________________________________________________________________________________________________________
> > *** CID 1488870:    (CHECKED_RETURN)
> > /qemu/tests/qtest/migration-test.c: 677 in test_migrate_tls_psk_start_common()
> > 671         test_tls_psk_init(data->pskfile);
> > 672     
> > 673         if (mismatch) {
> > 674             data->workdiralt = g_strdup_printf("%s/tlscredspskalt0", tmpfs);
> > 675             data->pskfilealt = g_strdup_printf("%s/%s", data->workdiralt,
> > 676                                                QCRYPTO_TLS_CREDS_PSKFILE);
> > >>>     CID 1488870:    (CHECKED_RETURN)
> > >>>     Calling "mkdir(data->workdiralt, 448U)" without checking return value. This library function may fail and return an error code.
> > 677             mkdir(data->workdiralt, 0700);
> > 678             test_tls_psk_init_alt(data->pskfilealt);
> > 679         }
> > 680     
> > 681         rsp = wait_command(from,
> > 682                            "{ 'execute': 'object-add',"
> > /qemu/tests/qtest/migration-test.c: 670 in test_migrate_tls_psk_start_common()
> > 664             g_new0(struct TestMigrateTLSPSKData, 1);
> > 665         QDict *rsp;
> > 666     
> > 667         data->workdir = g_strdup_printf("%s/tlscredspsk0", tmpfs);
> > 668         data->pskfile = g_strdup_printf("%s/%s", data->workdir,
> > 669                                         QCRYPTO_TLS_CREDS_PSKFILE);
> > >>>     CID 1488870:    (CHECKED_RETURN)
> > >>>     Calling "mkdir(data->workdir, 448U)" without checking return value. This library function may fail and return an error code.
> > 670         mkdir(data->workdir, 0700);
> > 671         test_tls_psk_init(data->pskfile);
> > 672     
> > 673         if (mismatch) {
> > 674             data->workdiralt = g_strdup_printf("%s/tlscredspskalt0", tmpfs);
> > 675             data->pskfilealt = g_strdup_printf("%s/%s", data->workdiralt,
> > 
> > ** CID 1488869:  Insecure data handling  (TAINTED_SCALAR)
> > /qemu/io/channel-socket.c: 716 in qio_channel_socket_flush()
> 
> 
> 
> This one is more curious:
> > *** CID 1488869:  Insecure data handling  (TAINTED_SCALAR)
> > /qemu/io/channel-socket.c: 716 in qio_channel_socket_flush()
> > 710         int ret = 1;
> > 711     
> > 712         msg.msg_control = control;
> > 713         msg.msg_controllen = sizeof(control);
> > 714         memset(control, 0, sizeof(control));
> > 715     
> > >>>     CID 1488869:  Insecure data handling  (TAINTED_SCALAR)
> > >>>     Using tainted variable "sioc->zero_copy_sent" as a loop boundary.
> > 716         while (sioc->zero_copy_sent < sioc->zero_copy_queued) {
> > 717             received = recvmsg(sioc->fd, &msg, MSG_ERRQUEUE);
> > 718             if (received < 0) {
> > 719                 switch (errno) {
> > 720                 case EAGAIN:
> > 721                     /* Nothing on errqueue, wait until something is available */
> 
> it's not clear to me why it considers that 'insecure'; is that because
> it's using values returned by the recvmsg ???

Yes, IIUC, coverity is applying he Perl-like concept of data
tainting here. The 'zero_copy_sent' field is incremented based
on the data from 'struct sock_extended_err' acquired from the
CMSG_DATA on a socket.

I expect Coverity does not understand that although the socket
peer is certainly untrustworthy, the CMSG_DATA we're processing
originated in the kernel and so that IS trustworthy.

Probably the only additional sanity checking we could do would be
to validate that we've not been given bogus data from the kenrel
that would result in a negative sent count.

     if ((serr->ee_info + 1) > serr->ee_data) {
         error_setg(errp, "Invalid sent packet count from kernel")
	 return -1;
     }

Perhaps that's what coverity wants us todo ? In practice that would
merely be protecting against kernel programming bug.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: New Defects reported by Coverity Scan for QEMU

[Peter Maydell]

On Wed, 18 May 2022 at 09:26, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
> This one is more curious:
> > *** CID 1488869:  Insecure data handling  (TAINTED_SCALAR)
> > /qemu/io/channel-socket.c: 716 in qio_channel_socket_flush()
> > 710         int ret = 1;
> > 711
> > 712         msg.msg_control = control;
> > 713         msg.msg_controllen = sizeof(control);
> > 714         memset(control, 0, sizeof(control));
> > 715
> > >>>     CID 1488869:  Insecure data handling  (TAINTED_SCALAR)
> > >>>     Using tainted variable "sioc->zero_copy_sent" as a loop boundary.
> > 716         while (sioc->zero_copy_sent < sioc->zero_copy_queued) {
> > 717             received = recvmsg(sioc->fd, &msg, MSG_ERRQUEUE);
> > 718             if (received < 0) {
> > 719                 switch (errno) {
> > 720                 case EAGAIN:
> > 721                     /* Nothing on errqueue, wait until something is available */
>
> it's not clear to me why it considers that 'insecure'; is that because
> it's using values returned by the recvmsg ???

Yes. The web UI is generally worth looking at for this kind of thing
as it has a lot more detail than the emailed summary. In particular
it shows the sequence of steps including where the tainted data
starts and how it propagates through other variables to get to the
point where it complains about it being used. In this case the
relevant steps are:

 10. tainted_argument: Calling function recvmsg taints argument msg.
 16. var_assign_var: Assigning: serr = (void *)cm->__cmsg_data. Both
are now tainted.
 19. lower_bounds: Casting narrower unsigned serr->ee_data -
serr->ee_info + 1U to wider signed type long effectively tests its
lower bound.
 20. var_assign_var: Compound assignment involving tainted variable
serr->ee_data - serr->ee_info + 1U to variable sioc->zero_copy_sent
taints sioc->zero_copy_sent.

More generally, there are quite a few "insecure data handling"
reports currently uncategorized in Coverity because I don't really
feel competent to judge whether they're legitimate or not a
problem for us. If anybody feels like taking on that task that
would be very helpful.

(Quite a lot of them are in slirp. I guess we could just bulk
close all of those on the grounds that slirp for us is now an
external module, assuming we trust the slirp folks to be on top
of their Coverity reports :-))

-- PMM