From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50543) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmiKn-0001I5-Pv for qemu-devel@nongnu.org; Wed, 21 Sep 2016 10:17:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmiKi-0002lI-S8 for qemu-devel@nongnu.org; Wed, 21 Sep 2016 10:17:13 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:35521) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmiKi-0002ks-K5 for qemu-devel@nongnu.org; Wed, 21 Sep 2016 10:17:08 -0400 Received: from pps.filterd (m0098394.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id u8LEDn7T084802 for ; Wed, 21 Sep 2016 10:17:06 -0400 Received: from e06smtp13.uk.ibm.com (e06smtp13.uk.ibm.com [195.75.94.109]) by mx0a-001b2d01.pphosted.com with ESMTP id 25kkb670q9-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 21 Sep 2016 10:17:06 -0400 Received: from localhost by e06smtp13.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 21 Sep 2016 15:17:03 +0100 Received: from b06cxnps4074.portsmouth.uk.ibm.com (d06relay11.portsmouth.uk.ibm.com [9.149.109.196]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id 423EF219006C for ; Wed, 21 Sep 2016 15:16:21 +0100 (BST) Received: from d06av02.portsmouth.uk.ibm.com (d06av02.portsmouth.uk.ibm.com [9.149.37.228]) by b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u8LEH14j4522258 for ; Wed, 21 Sep 2016 14:17:01 GMT Received: from d06av02.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u8LEH08h008257 for ; Wed, 21 Sep 2016 08:17:00 -0600 Date: Wed, 21 Sep 2016 16:16:59 +0200 From: Cornelia Huck In-Reply-To: <147446364067.4880.17009801693705082626.stgit@bahia> References: <147446363181.4880.18104448248886932114.stgit@bahia> <147446364067.4880.17009801693705082626.stgit@bahia> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-Id: <20160921161659.7c8f40b9.cornelia.huck@de.ibm.com> Subject: Re: [Qemu-devel] [PATCH 1/7] virtio-9p: handle handle_9p_output() error List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: qemu-devel@nongnu.org, Kevin Wolf , "Michael S. Tsirkin" , Jason Wang , Max Reitz , "Aneesh Kumar K.V" , Stefan Hajnoczi , Paolo Bonzini On Wed, 21 Sep 2016 15:14:00 +0200 Greg Kurz wrote: > A broken guest may send a request with only non-empty out buffers > or only non-empty in buffers, virtqueue_pop() will then return a > VirtQueueElement with out_num == 0 or in_num == 0 respectively. > > All 9P requests are expected to start with the following 7-byte header: > > uint32_t size_le; > uint8_t id; > uint16_t tag_le; > > If iov_to_buf() fails to return these 7 bytes, then something is wrong in > the guest. > > In both cases, it is wrong to crash QEMU, since the root cause lies in the > guest. Let's switch the device to the broken state instead. > > Signed-off-by: Greg Kurz > --- > hw/9pfs/virtio-9p-device.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c > index 009b43f6d045..0f09bef13392 100644 > --- a/hw/9pfs/virtio-9p-device.c > +++ b/hw/9pfs/virtio-9p-device.c > @@ -56,13 +56,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 || elem->in_num == 0) { > + virtio_error(vdev, > + "The guest sent a VirtFS request without headers"); > + pdu_free(pdu); > + return; Make that 'break;' to be more consistent with the code right above? > + } > 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) { I always find 'sizeof foo' instead of 'sizeof(foo)' a bit jarring... > + virtio_error(vdev, "The guest sent a malformed VirtFS request: " > + "header size is %zd, should be 7", len); > + pdu_free(pdu); > + return; > + } > > pdu->size = le32_to_cpu(out.size_le); > > Looks good to me.