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