[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: P J P
Subject: Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set
Date: Wed, 29 Nov 2017 15:41:45 +0530 (IST)

  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)) {
  if (!vdev->vq[n].vring.num) {

'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) {

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_update_rings

 -> virtio_queue_update_rings

 for (i = 0; i < num; i++) {
   if (vdev->vq[i].vring.desc) {

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 */

| 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.


Thank you.
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

reply via email to

[Prev in Thread] Current Thread [Next in Thread]