Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Greg Kurz]

On Thu, 08 Sep 2016 09:14:05 +0200
Markus Armbruster <address@hidden> wrote:

> Greg Kurz <address@hidden> writes:
> 
> > Calling assert() really makes sense when hitting a genuine bug, which calls
> > for a fix in QEMU. However, when something goes wrong because the guest
> > sends a malformed message, it is better to write down a more meaningul
> > error message and exit.
> >
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> >  hw/9pfs/virtio-9p-device.c |   20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> > index 009b43f6d045..67059182645a 100644
> > --- a/hw/9pfs/virtio-9p-device.c
> > +++ b/hw/9pfs/virtio-9p-device.c
> > @@ -19,6 +19,7 @@
> >  #include "coth.h"
> >  #include "hw/virtio/virtio-access.h"
> >  #include "qemu/iov.h"
> > +#include "qemu/error-report.h"
> >  
> >  void virtio_9p_push_and_notify(V9fsPDU *pdu)
> >  {
> > @@ -35,6 +36,11 @@ void virtio_9p_push_and_notify(V9fsPDU *pdu)
> >      virtio_notify(VIRTIO_DEVICE(v), v->vq);
> >  }
> >  
> > +static void virtio_9p_error(const char *msg)
> > +{
> > +    error_report("The virtio-9p driver in the guest has an issue: %s", 
> > msg);
> > +}
> > +
> >  static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
> >  {
> >      V9fsVirtioState *v = (V9fsVirtioState *)vdev;
> > @@ -56,13 +62,23 @@ static void handle_9p_output(VirtIODevice *vdev, 
> > VirtQueue *vq)
> >              break;
> >          }
> >  
> > -        BUG_ON(elem->out_num == 0 || elem->in_num == 0);
> > +        if (elem->out_num == 0) {
> > +            virtio_9p_error("missing VirtFS request's header");
> > +            exit(1);
> > +        }  
> 
> Can the guest trigger this?
> 

Yes it can in theory if it pushes an empty buffer... but this "recent"
commit changed the outcome:

commit 1e7aed70144b4673fc26e73062064b6724795e5f
Author: Prasad J Pandit <address@hidden>
Date:   Wed Jul 27 21:07:56 2016 +0530

    virtio: check vring descriptor buffer length

And now, the error is caught in virtqueue_map_desc():

    if (!sz) {
        error_report("virtio: zero sized buffers are not allowed");
        exit(1);
    }

So I guess we should keep the BUG_ON() then.

BTW, there are similar checks in virtio-blk and virtio-net leading to a QEMU
exit... which seem to be obsoleted by the above commit. I'll have a closer
look.

> > +        if (elem->in_num == 0) {
> > +            virtio_9p_error("missing VirtFS reply's header");
> > +            exit(1);
> > +        }  
> 
> Same question.
> 

Same answer. :)

> >          QEMU_BUILD_BUG_ON(sizeof out != 7);
> >  
> >          v->elems[pdu->idx] = elem;
> >          len = iov_to_buf(elem->out_sg, elem->out_num, 0,
> >                           &out, sizeof out);
> > -        BUG_ON(len != sizeof out);
> > +        if (len != sizeof out) {
> > +            virtio_9p_error("malformed VirtFS request");
> > +            exit(1);
> > +        }  
> 
> Same question.
> 

Here this is different: the guest can put a bogus len in the vring_desc
structure, and this doesn't get checked earlier.

> >  
> >          pdu->size = le32_to_cpu(out.size_le);
> >    

Cheers.

--
Greg