From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38835) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eJzLV-0001r0-1F for qemu-devel@nongnu.org; Wed, 29 Nov 2017 05:12:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eJzLQ-0005ey-2r for qemu-devel@nongnu.org; Wed, 29 Nov 2017 05:12:01 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36186) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eJzLP-0005eL-Qo for qemu-devel@nongnu.org; Wed, 29 Nov 2017 05:11:56 -0500 Date: Wed, 29 Nov 2017 15:41:45 +0530 (IST) From: P J P In-Reply-To: <20171128130012.1466d231.cohuck@redhat.com> Message-ID: References: <20171124183446.7308-1-ppandit@redhat.com> <20171124183446.7308-2-ppandit@redhat.com> <20171127111532.GI16527@stefanha-x1.localdomain> <20171128101154.64b49cb0.cohuck@redhat.com> <20171128103757.GD17146@stefanha-x1.localdomain> <20171128130012.1466d231.cohuck@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Subject: Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck Cc: Stefan Hajnoczi , Qemu Developers , zhangboxian , Paolo Bonzini Hello Cornelia, +-- On Tue, 28 Nov 2017, Cornelia Huck wrote --+ | What is "unfit for use"? Unfit for use because we see checks like if (!virtio_queue_get_num(vdev, n)) { continue; ... if (!vdev->vq[n].vring.num) { return; 'virtio_queue_set_rings' sets 'vring.desc' as vdev->vq[n].vring.desc = desc; and calls virtio_init_region_cache(vdev, n); which returns if vq->vring.desc is zero(0). addr = vq->vring.desc; if (!addr) { return; } Same in virtio_queue_set_addr() -> virtio_queue_update_rings(). It seems that for 'vq' instance to be useful, vring.num, vring.desc etc. fields need to be set properly. Unless an unused/free 'vq' is being accessed to set these fields. | I'm not quite sure what you want to achieve with this patch. I assume | you want to fix the issue that a guest may provide invalid values for | align etc. which can cause qemu to crash or behave badly. True. In the process I'm trying to figure out if a usable 'vq' instance could be decided in once place, than having repeating checks, if possible. Ex. 'virtio_queue_update_rings' is called as virtio_queue_set_addr -> virtio_queue_update_rings virtio_queue_set_align -> virtio_queue_update_rings virtio_load for (i = 0; i < num; i++) { if (vdev->vq[i].vring.desc) { ... virtio_queue_update_rings Of these, virtio_load checks that 'vring.desc' is non-zero(0). Current patch adds couple checks to the other two callers above. And again, virtio_queue_update_rings would check if (!vring->num || !vring->desc || !vring->align) { /* not yet setup -> nothing to do */ return; } | If so, you need to do different things for the different points above. | - The guest should not muck around with a non-existing queue (num == 0) | in any case, so this should be fenced for any manipulation triggered | by the guest. I guess done by !virtio_queue_get_num() check above? | - Processing a non-setup queue (desc == 0; also applies to the other | buffers for virtio-1) should be skipped. However, _setting_ desc etc. | to 0 from the guest is fine (as long as it follows the other | constraints of the spec). Okay. Though its non-zero(0) value is preferred? | - Setting alignment to 0 only applies to legacy + virtio-mmio. I would | not overengineer fencing this. A simple check in update_rings should | be enough. Okay.x Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F