Re: [Qemu-devel] virtio-ccw.c vs larger VIRTIO_QUEUE_MAX (coverity warning CID 1390619)

[Cornelia Huck <cohuck@redhat.com>]

On Mon, 14 May 2018 19:12:27 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> Hi; Coverity has I think enabled a new warning recently, which
> is triggering on virtio_ccw_notify() in hw/s390x/virtio-ccw.c
> (CID 1390619).
> 
> This function does
>     indicators |= 1ULL << vector;
> but the code is guarded only by
>     if (vector < VIRTIO_QUEUE_MAX) {
> 
> That used to be OK when VIRTIO_QUEUE_MAX was 64, but in
> commit b829c2a98f1 it was raised to 1024, and this is no longer
> a useful guard. The commit message for b829c2a98f1 suggests that
> this is a "can't happen" case -- is that so? 

That is outdated; we bumped max virtqueues for ccw in b1914b824ade.

> If so then the
> else {} part of the code and an earlier check on
> "if (vector >= VIRTIO_QUEUE_MAX + 64)" are dead code.
> However it looks like the device_plugged method is also
> checking VIRTIO_QUEUE_MAX, rather than 64.
> 
> If this is a false positive, then an assert() in
> virtio_ccw_notify() and cleaning up the dead code would
> help placate coverity.

It is a false positive as far as I can see; this is the notification
code for classical interrupts, and we fence off more than 64 virtqueues
already when the guest tries to set it up (introduced in 797b608638c5).
As that code flow is basically impossible to deduce by a static code
checker, adding an assert() seems like a good idea. Halil, what do you
think?

> 
> (Other odd code in that function:
>     vector = 0;
>     [...]
>     indicators |= 1ULL << vector;
> is that really supposed to ignore the input vector number?)

Yes; this as a configuration change notification done via secondary
indicators (different from either the classical indicators or the
adapter interrupt indicators). We always set the same bit, and it is
always triggered by doing a notification with a number one above the
maximum possible virtqueue number. (I agree that it does look odd, we
should maybe add a comment.)