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

[Halil Pasic]

On 05/15/2018 02:07 PM, Peter Maydell wrote:
> On 15 May 2018 at 13:00, Halil Pasic <address@hidden> wrote:
> > To sum it up, my take on the whole is the diff below. I can convert
> > it to a proper patch if we agree that's the way to go.
> >
> > From: Halil Pasic <address@hidden>
> > Date: Tue, 15 May 2018 13:57:44 +0200
> > Subject: [PATCH] WIP: cleanup virtio notify
> >
> > Signed-off-by: Halil Pasic <address@hidden>
> > ---
> >   hw/s390x/virtio-ccw.c | 9 +++------
> >   1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > index e51fbefd23..22078605d1 100644
> > --- a/hw/s390x/virtio-ccw.c
> > +++ b/hw/s390x/virtio-ccw.c
> > @@ -1003,10 +1003,8 @@ static void virtio_ccw_notify(DeviceState *d,
> > uint16_t vector)
> >       SubchDev *sch = ccw_dev->sch;
> >       uint64_t indicators;
> >
> > -    /* queue indicators + secondary indicators */
> > -    if (vector >= VIRTIO_QUEUE_MAX + 64) {
> > -        return;
> > -    }
> > +    /* vector == VIRTIO_QUEUE_MAX means configuration change */
> > +    assert(vector <= VIRTIO_QUEUE_MAX);
> >
> >       if (vector < VIRTIO_QUEUE_MAX) {
> >           if (!dev->indicators) {
>
> This will not be sufficient to satisfy Coverity, because
> VIRTIO_QUEUE_MAX is 1024, and that is way too big to use as
> a shift count, and that's the only check that Coverity can see
> on the code path leading into the "1ULL << vector" code.
>
> If it's not possible to get here with a vector that's 64 or more,
> you need to
>      assert(vector < NR_CLASSIC_INDICATOR_BITS);
> somewhere.

Right. Here is the revised version. It's the second hunk.

Regards,
Halil

> thanks
> -- PMM

--------------------------------8<------------------------------------------------
From: Halil Pasic <address@hidden>
Date: Tue, 15 May 2018 13:57:44 +0200
Subject: [PATCH] WIP: cleanup virtio notify

Signed-off-by: Halil Pasic <address@hidden>
---
 hw/s390x/virtio-ccw.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 22df33b509..be433b0336 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1003,10 +1003,8 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t 
vector)
     SubchDev *sch = ccw_dev->sch;
     uint64_t indicators;

-    /* queue indicators + secondary indicators */
-    if (vector >= VIRTIO_QUEUE_MAX + 64) {
-        return;
-    }
+    /* vector == VIRTIO_QUEUE_MAX means configuration change */
+    assert(vector <= VIRTIO_QUEUE_MAX);

     if (vector < VIRTIO_QUEUE_MAX) {
         if (!dev->indicators) {
@@ -1029,6 +1027,7 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t 
vector)
                 css_adapter_interrupt(CSS_IO_ADAPTER_VIRTIO, dev->thinint_isc);
             }
         } else {
+            assert(vector < NR_CLASSIC_INDICATOR_BITS);
             indicators = address_space_ldq(&address_space_memory,
                                            dev->indicators->addr,
                                            MEMTXATTRS_UNSPECIFIED,
@@ -1042,12 +1041,11 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t 
vector)
         if (!dev->indicators2) {
             return;
         }
-        vector = 0;
         indicators = address_space_ldq(&address_space_memory,
                                        dev->indicators2->addr,
                                        MEMTXATTRS_UNSPECIFIED,
                                        NULL);
-        indicators |= 1ULL << vector;
+        indicators |= 1ULL;
         address_space_stq(&address_space_memory, dev->indicators2->addr,
                           indicators, MEMTXATTRS_UNSPECIFIED, NULL);
         css_conditional_io_interrupt(sch);