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 :|