[Qemu-devel] [PATCH v2] virtio-serial: Fix segfault on guest boot

[Qemu-devel] [PATCH v2] virtio-serial: Fix segfault on guest boot

[Luiz Capitulino]

If I start qemu with:

  # qemu -hda disks/test.img -enable-kvm -m 1G -snapshot \
         -device virtio-serial \
         -chardev socket,host=localhost,port=1234,server,nowait,id=foo \
         -device virtserialport,chardev=foo,name=org.qemu.guest_agent

I get a segfault when booting a Fedora 14 guest. The backtrace says:

  Program terminated with signal 11, Segmentation fault.
    #0  0x0000000000420850 in handle_control_message (vser=0x3732bd0, buf=0x2c173e0, len=8) at /home/lcapitulino/src/qmp-unstable/hw/virtio-serial-bus.c:335
    335     info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);

What's happening is VIRTIO_CONSOLE_DEVICE_READY is a message for the
whole device, not for an individual port. So port is NULL. This bug was
introduced by commit a15bb0d6a981de749452a5180fc8084d625671da.

This commit fixes that by making the port returned by find_port_by_id()
be used only by the VIRTIO_CONSOLE_PORT_READY and
VIRTIO_CONSOLE_PORT_OPEN messages.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/virtio-serial-bus.c |   24 ++++++++++++++----------
 1 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 9a12104..33a6f61 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -328,18 +328,11 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
     cpkt.event = lduw_p(&gcpkt->event);
     cpkt.value = lduw_p(&gcpkt->value);
 
-    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
-    if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY)
-        return;
-
-    info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
-
-    switch(cpkt.event) {
-    case VIRTIO_CONSOLE_DEVICE_READY:
+    if (cpkt.event == VIRTIO_CONSOLE_DEVICE_READY) {
         if (!cpkt.value) {
             error_report("virtio-serial-bus: Guest failure in adding device %s\n",
                          vser->bus.qbus.name);
-            break;
+            return;
         }
         /*
          * The device is up, we can now tell the device about all the
@@ -348,8 +341,19 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
         QTAILQ_FOREACH(port, &vser->ports, next) {
             send_control_event(port, VIRTIO_CONSOLE_PORT_ADD, 1);
         }
-        break;
+        return;
+    }
 
+    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
+    if (!port) {
+        error_report("virtio-serial-bus: Expected port id %d for device %s\n",
+                     ldl_p(&gcpkt->id), vser->bus.qbus.name);
+        return;
+    }
+
+    info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
+
+    switch(cpkt.event) {
     case VIRTIO_CONSOLE_PORT_READY:
         if (!cpkt.value) {
             error_report("virtio-serial-bus: Guest failure in adding port %u for device %s\n",
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH v2] virtio-serial: Fix segfault on guest boot

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> If I start qemu with:
>
>   # qemu -hda disks/test.img -enable-kvm -m 1G -snapshot \
>          -device virtio-serial \
>          -chardev socket,host=localhost,port=1234,server,nowait,id=foo \
>          -device virtserialport,chardev=foo,name=org.qemu.guest_agent
>
> I get a segfault when booting a Fedora 14 guest. The backtrace says:
>
>   Program terminated with signal 11, Segmentation fault.
>     #0  0x0000000000420850 in handle_control_message (vser=0x3732bd0, buf=0x2c173e0, len=8) at /home/lcapitulino/src/qmp-unstable/hw/virtio-serial-bus.c:335
>     335     info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
>
> What's happening is VIRTIO_CONSOLE_DEVICE_READY is a message for the
> whole device, not for an individual port. So port is NULL. This bug was
> introduced by commit a15bb0d6a981de749452a5180fc8084d625671da.
>
> This commit fixes that by making the port returned by find_port_by_id()
> be used only by the VIRTIO_CONSOLE_PORT_READY and
> VIRTIO_CONSOLE_PORT_OPEN messages.

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

Re: [Qemu-devel] [PATCH v2] virtio-serial: Fix segfault on guest boot

[Amit Shah]

On (Fri) 17 Jun 2011 [15:58:08], Luiz Capitulino wrote:
> If I start qemu with:
> 
>   # qemu -hda disks/test.img -enable-kvm -m 1G -snapshot \
>          -device virtio-serial \
>          -chardev socket,host=localhost,port=1234,server,nowait,id=foo \
>          -device virtserialport,chardev=foo,name=org.qemu.guest_agent
> 
> I get a segfault when booting a Fedora 14 guest. The backtrace says:
> 
>   Program terminated with signal 11, Segmentation fault.
>     #0  0x0000000000420850 in handle_control_message (vser=0x3732bd0, buf=0x2c173e0, len=8) at /home/lcapitulino/src/qmp-unstable/hw/virtio-serial-bus.c:335
>     335     info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
> 
> What's happening is VIRTIO_CONSOLE_DEVICE_READY is a message for the
> whole device, not for an individual port. So port is NULL. This bug was
> introduced by commit a15bb0d6a981de749452a5180fc8084d625671da.
> 
> This commit fixes that by making the port returned by find_port_by_id()
> be used only by the VIRTIO_CONSOLE_PORT_READY and
> VIRTIO_CONSOLE_PORT_OPEN messages.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  hw/virtio-serial-bus.c |   24 ++++++++++++++----------
>  1 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index 9a12104..33a6f61 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -328,18 +328,11 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
>      cpkt.event = lduw_p(&gcpkt->event);
>      cpkt.value = lduw_p(&gcpkt->value);
>  
> -    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
> -    if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY)
> -        return;
> -
> -    info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
> -
> -    switch(cpkt.event) {
> -    case VIRTIO_CONSOLE_DEVICE_READY:
> +    if (cpkt.event == VIRTIO_CONSOLE_DEVICE_READY) {

What we lose after this re-arrangement is the check that port is NULL
when this message is received.  i.e., a guest bug where port is set to
a valid value when this message arrives.  (I think I pointed this out
in a previous mail?)

>          if (!cpkt.value) {
>              error_report("virtio-serial-bus: Guest failure in adding device %s\n",
>                           vser->bus.qbus.name);
> -            break;
> +            return;
>          }
>          /*
>           * The device is up, we can now tell the device about all the
> @@ -348,8 +341,19 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
>          QTAILQ_FOREACH(port, &vser->ports, next) {
>              send_control_event(port, VIRTIO_CONSOLE_PORT_ADD, 1);
>          }
> -        break;
> +        return;
> +    }
>  
> +    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
> +    if (!port) {
> +        error_report("virtio-serial-bus: Expected port id %d for device %s\n",
> +                     ldl_p(&gcpkt->id), vser->bus.qbus.name);

It's actually 'unexpected port id...' :-)

Also, port id is %u, not %d.

Fixed these two and committed to virtio-serial tree.

Thanks,
		Amit

Re: [Qemu-devel] [PATCH v2] virtio-serial: Fix segfault on guest boot

[Luiz Capitulino]

On Wed, 22 Jun 2011 09:49:22 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Fri) 17 Jun 2011 [15:58:08], Luiz Capitulino wrote:
> > If I start qemu with:
> > 
> >   # qemu -hda disks/test.img -enable-kvm -m 1G -snapshot \
> >          -device virtio-serial \
> >          -chardev socket,host=localhost,port=1234,server,nowait,id=foo \
> >          -device virtserialport,chardev=foo,name=org.qemu.guest_agent
> > 
> > I get a segfault when booting a Fedora 14 guest. The backtrace says:
> > 
> >   Program terminated with signal 11, Segmentation fault.
> >     #0  0x0000000000420850 in handle_control_message (vser=0x3732bd0, buf=0x2c173e0, len=8) at /home/lcapitulino/src/qmp-unstable/hw/virtio-serial-bus.c:335
> >     335     info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
> > 
> > What's happening is VIRTIO_CONSOLE_DEVICE_READY is a message for the
> > whole device, not for an individual port. So port is NULL. This bug was
> > introduced by commit a15bb0d6a981de749452a5180fc8084d625671da.
> > 
> > This commit fixes that by making the port returned by find_port_by_id()
> > be used only by the VIRTIO_CONSOLE_PORT_READY and
> > VIRTIO_CONSOLE_PORT_OPEN messages.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  hw/virtio-serial-bus.c |   24 ++++++++++++++----------
> >  1 files changed, 14 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> > index 9a12104..33a6f61 100644
> > --- a/hw/virtio-serial-bus.c
> > +++ b/hw/virtio-serial-bus.c
> > @@ -328,18 +328,11 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
> >      cpkt.event = lduw_p(&gcpkt->event);
> >      cpkt.value = lduw_p(&gcpkt->value);
> >  
> > -    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
> > -    if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY)
> > -        return;
> > -
> > -    info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
> > -
> > -    switch(cpkt.event) {
> > -    case VIRTIO_CONSOLE_DEVICE_READY:
> > +    if (cpkt.event == VIRTIO_CONSOLE_DEVICE_READY) {
> 
> What we lose after this re-arrangement is the check that port is NULL
> when this message is received.  i.e., a guest bug where port is set to
> a valid value when this message arrives.  (I think I pointed this out
> in a previous mail?)

I'm not sure I follow you here, the current code doesn't return if
cpkt.event == VIRTIO_CONSOLE_DEVICE_READY:

    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
    if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY)
        return;

> 
> >          if (!cpkt.value) {
> >              error_report("virtio-serial-bus: Guest failure in adding device %s\n",
> >                           vser->bus.qbus.name);
> > -            break;
> > +            return;
> >          }
> >          /*
> >           * The device is up, we can now tell the device about all the
> > @@ -348,8 +341,19 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
> >          QTAILQ_FOREACH(port, &vser->ports, next) {
> >              send_control_event(port, VIRTIO_CONSOLE_PORT_ADD, 1);
> >          }
> > -        break;
> > +        return;
> > +    }
> >  
> > +    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
> > +    if (!port) {
> > +        error_report("virtio-serial-bus: Expected port id %d for device %s\n",
> > +                     ldl_p(&gcpkt->id), vser->bus.qbus.name);
> 
> It's actually 'unexpected port id...' :-)
> 
> Also, port id is %u, not %d.
> 
> Fixed these two and committed to virtio-serial tree.
> 
> Thanks,
> 		Amit
> 

Re: [Qemu-devel] [PATCH v2] virtio-serial: Fix segfault on guest boot

[Amit Shah]

On (Wed) 22 Jun 2011 [09:53:35], Luiz Capitulino wrote:
> On Wed, 22 Jun 2011 09:49:22 +0530
> Amit Shah <amit.shah@redhat.com> wrote:

> > >  
> > > -    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
> > > -    if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY)
> > > -        return;
> > > -
> > > -    info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
> > > -
> > > -    switch(cpkt.event) {
> > > -    case VIRTIO_CONSOLE_DEVICE_READY:
> > > +    if (cpkt.event == VIRTIO_CONSOLE_DEVICE_READY) {
> > 
> > What we lose after this re-arrangement is the check that port is NULL
> > when this message is received.  i.e., a guest bug where port is set to
> > a valid value when this message arrives.  (I think I pointed this out
> > in a previous mail?)
> 
> I'm not sure I follow you here, the current code doesn't return if
> cpkt.event == VIRTIO_CONSOLE_DEVICE_READY:
> 
>     port = find_port_by_id(vser, ldl_p(&gcpkt->id));
>     if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY)
>         return;

Ah; right.  Anyway it's a small thing, nothing to be worried about.

		Amit