On Thu, 08 Sep 2016 09:14:05 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> Greg Kurz <gr...@kaod.org> 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 <gr...@kaod.org>
> > ---
> > 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 <p...@fedoraproject.org>
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
