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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Fri, 17 Jun 2011 21:39:26 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Fri) 17 Jun 2011 [10:16:44], Luiz Capitulino wrote:
> > On Fri, 17 Jun 2011 12:17:36 +0530
> > Amit Shah <amit.shah@redhat.com> wrote:
> > 
> > > On (Thu) 16 Jun 2011 [13:38:49], 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);
> > > 
> > > Strange, I've not seen it so far in my testing (neither in the daily
> > > test runs of the virtio-serial testsuite).
> > > 
> > > > I've also bisected this and git points out to commit:
> > > > 
> > > >   commit a15bb0d6a981de749452a5180fc8084d625671da
> > > >   Author: Markus Armbruster <armbru@redhat.com>
> > > >   Date:   Wed May 25 14:21:13 2011 +0200
> > > > 
> > > >       virtio-serial: Drop redundant VirtIOSerialPort member info
> > > > 
> > > > I think what's happening is that the device is not initialized on a
> > > > VIRTIO_CONSOLE_DEVICE_READY event. Moving the DO_UPCAST() call to
> > > > the other events fixes the problem to me.
> > > > 
> > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > > ---
> > > >  hw/virtio-serial-bus.c |    4 ++--
> > > >  1 files changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> > > > index 9a12104..579f676 100644
> > > > --- a/hw/virtio-serial-bus.c
> > > > +++ b/hw/virtio-serial-bus.c
> > > > @@ -332,8 +332,6 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
> > > >      if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY)
> > > >          return;
> > > >  
> > > > -    info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
> > > > -
> > > 
> > > Ah - this missed the !port check.  It should be possible to do this in
> > > a 'if (port)' block instead of replicating in the individual case
> > > statements.
> > > 
> > > Thanks for the debugging and patch; please update with the above and
> > > I'll apply it to the virtio-serial tree.
> > 
> > What about moving the VIRTIO_CONSOLE_DEVICE_READY handling out of the
> > switch, like the patch below? This way the function is divided in a way
> > that related events are handled together.
> > 
> > I'll implement your first suggestion if you don't like this...
> > 
> > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> > index 579f676..5f96245 100644
> > --- a/hw/virtio-serial-bus.c
> > +++ b/hw/virtio-serial-bus.c
> > @@ -325,19 +325,12 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
> >          return;
> >      }
> >  
> > -    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;
> > -
> > -    switch(cpkt.event) {
> > -    case VIRTIO_CONSOLE_DEVICE_READY:
> > +    cpkt.event = lduw_p(&gcpkt->event);
> > +    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;
> > +            error_report("virtio-serial-bus: Guest failure in adding device %s\n", vser->bus.qbus.name);
> > +            return;
> 
> The line split should remain -- else it goes beyond 80 chars.

It's already beyond 80 chars to me.

> 
> >          }
> >          /*
> >           * The device is up, we can now tell the device about all the
> > @@ -346,8 +339,13 @@ 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;
> > +    }
> 
> Makes me think of one case (totally unrelated to what you found)where
> the guest can fool us: by sending multiple VIRTIO_CONSOLE_DEVICE_READY
> messages.

It will be handled just fine, no?

> 
> > +    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
> > +    assert(port != NULL);
> 
> I doubt if assert is the right thing: if the guest sends bad data, we
> shouldn't just kill it.  It's easier to ignore such data, and perhaps
> just log it.

Right.

> 
> > +
> > +    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",
> 
> I'm fine with this approach.
> 
> 		Amit
>