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

[P J P]

  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