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
