Patchwork [1/1] virtio-serial-bus: send_control_msg should not deal with cpkts

login
register
mail settings
Submitter Amit Shah
Date Dec. 13, 2012, 11:03 a.m.
Message ID <93bab39d85368c53633059501e58dc726d50c457.1355396592.git.amit.shah@redhat.com>
Download mbox | patch
Permalink /patch/205804/
State New
Headers show

Comments

Amit Shah - Dec. 13, 2012, 11:03 a.m.
Stuff the cpkt before calling send_control_msg().  it should not be
concerned about contents, just send across the buffer.

A few code refactorings recently have made mkaing this change easier
than earlier.

Coverity and clang have flagged this code several times in the past
(cpkt->id not set before send_control_event() passed it on to
send_control_msg()).  This will finally eliminate the false-positive.

CC: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)
Markus Armbruster - Dec. 17, 2012, 1:14 p.m.
Amit Shah <amit.shah@redhat.com> writes:

> Stuff the cpkt before calling send_control_msg().  it should not be
> concerned about contents, just send across the buffer.
>
> A few code refactorings recently have made mkaing this change easier
> than earlier.
>
> Coverity and clang have flagged this code several times in the past
> (cpkt->id not set before send_control_event() passed it on to
> send_control_msg()).  This will finally eliminate the false-positive.
>
> CC: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

Patch makes sense to me, and the Coverity defect report is gone.
However, it now worries find_port_by_id() in remove_port() could return
a null pointer, which is then dereferenced.  No idea why it didn't
report that before.  Obvious suppressor:

    diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
    index 47d0481..7ff7505 100644
    --- a/hw/virtio-serial-bus.c
    +++ b/hw/virtio-serial-bus.c
    @@ -826,6 +826,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
         vser->ports_map[i] &= ~(1U << (port_id % 32));

         port = find_port_by_id(vser, port_id);
    +    assert(port);
         /* Flush out any unconsumed buffers first */
         discard_vq_data(port->ovq, &port->vser->vdev);

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Amit Shah - Dec. 17, 2012, 4:34 p.m.
On (Mon) 17 Dec 2012 [14:14:17], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > Stuff the cpkt before calling send_control_msg().  it should not be
> > concerned about contents, just send across the buffer.
> >
> > A few code refactorings recently have made mkaing this change easier
> > than earlier.

Ugh, I'll fix the typo and incoherent language here before merging.

> > Coverity and clang have flagged this code several times in the past
> > (cpkt->id not set before send_control_event() passed it on to
> > send_control_msg()).  This will finally eliminate the false-positive.
> >
> > CC: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> 
> Patch makes sense to me, and the Coverity defect report is gone.

Thanks for checking!

> However, it now worries find_port_by_id() in remove_port() could return
> a null pointer, which is then dereferenced.  No idea why it didn't
> report that before.  Obvious suppressor:
> 
>     diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
>     index 47d0481..7ff7505 100644
>     --- a/hw/virtio-serial-bus.c
>     +++ b/hw/virtio-serial-bus.c
>     @@ -826,6 +826,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
>          vser->ports_map[i] &= ~(1U << (port_id % 32));
> 
>          port = find_port_by_id(vser, port_id);
>     +    assert(port);
>          /* Flush out any unconsumed buffers first */
>          discard_vq_data(port->ovq, &port->vser->vdev);

remove_port() is called by the hot-unplug qdev callback, and if the
port's missing from our tailq, something's gone wrong anyway.  So this
patch makes sense too.

> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks!

		Amit
Markus Armbruster - Dec. 17, 2012, 5:23 p.m.
Amit Shah <amit.shah@redhat.com> writes:

> On (Mon) 17 Dec 2012 [14:14:17], Markus Armbruster wrote:
>> Amit Shah <amit.shah@redhat.com> writes:
>> 
>> > Stuff the cpkt before calling send_control_msg().  it should not be
>> > concerned about contents, just send across the buffer.
>> >
>> > A few code refactorings recently have made mkaing this change easier
>> > than earlier.
>
> Ugh, I'll fix the typo and incoherent language here before merging.
>
>> > Coverity and clang have flagged this code several times in the past
>> > (cpkt->id not set before send_control_event() passed it on to
>> > send_control_msg()).  This will finally eliminate the false-positive.
>> >
>> > CC: Markus Armbruster <armbru@redhat.com>
>> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
>> 
>> Patch makes sense to me, and the Coverity defect report is gone.
>
> Thanks for checking!
>
>> However, it now worries find_port_by_id() in remove_port() could return
>> a null pointer, which is then dereferenced.  No idea why it didn't
>> report that before.  Obvious suppressor:
>> 
>>     diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
>>     index 47d0481..7ff7505 100644
>>     --- a/hw/virtio-serial-bus.c
>>     +++ b/hw/virtio-serial-bus.c
>>     @@ -826,6 +826,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
>>          vser->ports_map[i] &= ~(1U << (port_id % 32));
>> 
>>          port = find_port_by_id(vser, port_id);
>>     +    assert(port);
>>          /* Flush out any unconsumed buffers first */
>>          discard_vq_data(port->ovq, &port->vser->vdev);
>
> remove_port() is called by the hot-unplug qdev callback, and if the
> port's missing from our tailq, something's gone wrong anyway.  So this
> patch makes sense too.

Will you take care of that, or do you want me to post the patch?
Amit Shah - Dec. 17, 2012, 5:31 p.m.
On (Mon) 17 Dec 2012 [18:23:53], Markus Armbruster wrote:
> >> However, it now worries find_port_by_id() in remove_port() could return
> >> a null pointer, which is then dereferenced.  No idea why it didn't
> >> report that before.  Obvious suppressor:
> >> 
> >>     diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> >>     index 47d0481..7ff7505 100644
> >>     --- a/hw/virtio-serial-bus.c
> >>     +++ b/hw/virtio-serial-bus.c
> >>     @@ -826,6 +826,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
> >>          vser->ports_map[i] &= ~(1U << (port_id % 32));
> >> 
> >>          port = find_port_by_id(vser, port_id);
> >>     +    assert(port);
> >>          /* Flush out any unconsumed buffers first */
> >>          discard_vq_data(port->ovq, &port->vser->vdev);
> >
> > remove_port() is called by the hot-unplug qdev callback, and if the
> > port's missing from our tailq, something's gone wrong anyway.  So this
> > patch makes sense too.
> 
> Will you take care of that, or do you want me to post the patch?

I was going to, but if you want to, go ahead -- you already have the
patch ready :)

		Amit
Markus Armbruster - Dec. 18, 2012, 7:31 a.m.
Amit Shah <amit.shah@redhat.com> writes:

> On (Mon) 17 Dec 2012 [18:23:53], Markus Armbruster wrote:
>> >> However, it now worries find_port_by_id() in remove_port() could return
>> >> a null pointer, which is then dereferenced.  No idea why it didn't
>> >> report that before.  Obvious suppressor:
>> >> 
>> >>     diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
>> >>     index 47d0481..7ff7505 100644
>> >>     --- a/hw/virtio-serial-bus.c
>> >>     +++ b/hw/virtio-serial-bus.c
>> >>     @@ -826,6 +826,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
>> >>          vser->ports_map[i] &= ~(1U << (port_id % 32));
>> >> 
>> >>          port = find_port_by_id(vser, port_id);
>> >>     +    assert(port);
>> >>          /* Flush out any unconsumed buffers first */
>> >>          discard_vq_data(port->ovq, &port->vser->vdev);
>> >
>> > remove_port() is called by the hot-unplug qdev callback, and if the
>> > port's missing from our tailq, something's gone wrong anyway.  So this
>> > patch makes sense too.
>> 
>> Will you take care of that, or do you want me to post the patch?
>
> I was going to, but if you want to, go ahead -- you already have the
> patch ready :)

Happy to leave it to you.

Patch

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 155da58..47d0481 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -215,13 +215,12 @@  static void flush_queued_data(VirtIOSerialPort *port)
     do_flush_queued_data(port, port->ovq, &port->vser->vdev);
 }
 
-static size_t send_control_msg(VirtIOSerialPort *port, void *buf, size_t len)
+static size_t send_control_msg(VirtIOSerial *vser, void *buf, size_t len)
 {
     VirtQueueElement elem;
     VirtQueue *vq;
-    struct virtio_console_control *cpkt;
 
-    vq = port->vser->c_ivq;
+    vq = vser->c_ivq;
     if (!virtio_queue_ready(vq)) {
         return 0;
     }
@@ -229,25 +228,24 @@  static size_t send_control_msg(VirtIOSerialPort *port, void *buf, size_t len)
         return 0;
     }
 
-    cpkt = (struct virtio_console_control *)buf;
-    stl_p(&cpkt->id, port->id);
     memcpy(elem.in_sg[0].iov_base, buf, len);
 
     virtqueue_push(vq, &elem, len);
-    virtio_notify(&port->vser->vdev, vq);
+    virtio_notify(&vser->vdev, vq);
     return len;
 }
 
-static size_t send_control_event(VirtIOSerialPort *port, uint16_t event,
-                                 uint16_t value)
+static size_t send_control_event(VirtIOSerial *vser, uint32_t port_id,
+                                 uint16_t event, uint16_t value)
 {
     struct virtio_console_control cpkt;
 
+    stl_p(&cpkt.id, port_id);
     stw_p(&cpkt.event, event);
     stw_p(&cpkt.value, value);
 
-    trace_virtio_serial_send_control_event(port->id, event, value);
-    return send_control_msg(port, &cpkt, sizeof(cpkt));
+    trace_virtio_serial_send_control_event(port_id, event, value);
+    return send_control_msg(vser, &cpkt, sizeof(cpkt));
 }
 
 /* Functions for use inside qemu to open and read from/write to ports */
@@ -259,7 +257,7 @@  int virtio_serial_open(VirtIOSerialPort *port)
     }
     /* Send port open notification to the guest */
     port->host_connected = true;
-    send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 1);
+    send_control_event(port->vser, port->id, VIRTIO_CONSOLE_PORT_OPEN, 1);
 
     return 0;
 }
@@ -274,7 +272,7 @@  int virtio_serial_close(VirtIOSerialPort *port)
     port->throttled = false;
     discard_vq_data(port->ovq, &port->vser->vdev);
 
-    send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
+    send_control_event(port->vser, port->id, VIRTIO_CONSOLE_PORT_OPEN, 0);
 
     return 0;
 }
@@ -363,7 +361,7 @@  static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
          * ports we have here.
          */
         QTAILQ_FOREACH(port, &vser->ports, next) {
-            send_control_event(port, VIRTIO_CONSOLE_PORT_ADD, 1);
+            send_control_event(vser, port->id, VIRTIO_CONSOLE_PORT_ADD, 1);
         }
         return;
     }
@@ -394,10 +392,11 @@  static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
          * up to hvc.
          */
         if (vsc->is_console) {
-            send_control_event(port, VIRTIO_CONSOLE_CONSOLE_PORT, 1);
+            send_control_event(vser, port->id, VIRTIO_CONSOLE_CONSOLE_PORT, 1);
         }
 
         if (port->name) {
+            stl_p(&cpkt.id, port->id);
             stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME);
             stw_p(&cpkt.value, 1);
 
@@ -408,12 +407,12 @@  static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
             memcpy(buffer + sizeof(cpkt), port->name, strlen(port->name));
             buffer[buffer_len - 1] = 0;
 
-            send_control_msg(port, buffer, buffer_len);
+            send_control_msg(vser, buffer, buffer_len);
             g_free(buffer);
         }
 
         if (port->host_connected) {
-            send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 1);
+            send_control_event(vser, port->id, VIRTIO_CONSOLE_PORT_OPEN, 1);
         }
 
         /*
@@ -650,7 +649,7 @@  static void virtio_serial_post_load_timer_cb(void *opaque)
              * We have to let the guest know of the host connection
              * status change
              */
-            send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN,
+            send_control_event(s, port->id, VIRTIO_CONSOLE_PORT_OPEN,
                                port->host_connected);
         }
     }
@@ -815,9 +814,7 @@  static void mark_port_added(VirtIOSerial *vser, uint32_t port_id)
 static void add_port(VirtIOSerial *vser, uint32_t port_id)
 {
     mark_port_added(vser, port_id);
-
-    send_control_event(find_port_by_id(vser, port_id),
-                       VIRTIO_CONSOLE_PORT_ADD, 1);
+    send_control_event(vser, port_id, VIRTIO_CONSOLE_PORT_ADD, 1);
 }
 
 static void remove_port(VirtIOSerial *vser, uint32_t port_id)
@@ -832,7 +829,7 @@  static void remove_port(VirtIOSerial *vser, uint32_t port_id)
     /* Flush out any unconsumed buffers first */
     discard_vq_data(port->ovq, &port->vser->vdev);
 
-    send_control_event(port, VIRTIO_CONSOLE_PORT_REMOVE, 1);
+    send_control_event(vser, port->id, VIRTIO_CONSOLE_PORT_REMOVE, 1);
 }
 
 static int virtser_port_qdev_init(DeviceState *qdev)