Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set

[Cornelia Huck <cohuck@redhat.com>]

On Wed, 29 Nov 2017 15:41:45 +0530 (IST)
P J P <ppandit@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