From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46763) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhvH6-0002MZ-Tv for qemu-devel@nongnu.org; Thu, 08 Sep 2016 05:05:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bhvH2-0004BQ-8z for qemu-devel@nongnu.org; Thu, 08 Sep 2016 05:05:35 -0400 Received: from 5.mo68.mail-out.ovh.net ([46.105.62.179]:53630) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhvH1-0004B1-Uy for qemu-devel@nongnu.org; Thu, 08 Sep 2016 05:05:32 -0400 Received: from player778.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo68.mail-out.ovh.net (Postfix) with ESMTP id 26668FF8C2C for ; Thu, 8 Sep 2016 11:05:31 +0200 (CEST) Date: Thu, 8 Sep 2016 11:05:22 +0200 From: Greg Kurz Message-ID: <20160908110522.097ab135@bahia> In-Reply-To: <87eg4uyibm.fsf@dusky.pond.sub.org> References: <147326875705.8546.11347276277137015855.stgit@bahia.lan> <147326876478.8546.16045138068342092499.stgit@bahia.lan> <87eg4uyibm.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: "Michael S. Tsirkin" , qemu-devel@nongnu.org, "Aneesh Kumar K.V" On Thu, 08 Sep 2016 09:14:05 +0200 Markus Armbruster wrote: > Greg Kurz 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 > > --- > > 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 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