On Wed, 29 Nov 2017 15:41:45 +0530 (IST)
P J P <ppan...@redhat.com> wrote:
> 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 think the basic problem is still that you conflate two things:
- vring.num, which cannot be flipped between 0 and !0 by the guest
- vring.{desc,avail,used}, which can
IOW, if vring.num == 0, the guest cannot manipulate the queue; if
vring.desc == 0, the guest can.
>
> | 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;
> }
vring.num and vring.desc are really different things. You don't want
the guest to do anything with the queue if vring.num == 0, while you
just want to skip various processing if vring.desc == 0.
(virtio_load() does not need to care about vring.num, as it is not
triggered by the guest.)
>
> | 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?
Yes.
>
> | - 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?
Many functions have a likely/unlikely check, setup routines excepted.
>
> | - 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
