hw/s390x/virtio-ccw.c | 1 + hw/virtio/virtio-mmio.c | 5 ++--- hw/virtio/virtio-pci.c | 1 + 3 files changed, 4 insertions(+), 3 deletions(-)
When a virtqueue size is changed by the guest via
virtio_queue_set_num(), its region cache is not automatically updated.
If the size was increased, this could lead to accessing the cache out
of bounds. For example, in vring_get_used_event():
static inline uint16_t vring_get_used_event(VirtQueue *vq)
{
return vring_avail_ring(vq, vq->vring.num);
}
static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
{
VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
hwaddr pa = offsetof(VRingAvail, ring[i]);
if (!caches) {
return 0;
}
return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
}
vq->vring.num will be greater than caches->avail.len, which will
trigger a failed assertion down the call path of
virtio_lduw_phys_cached().
Fix this by calling virtio_queue_update_rings() after
virtio_queue_set_num() if we are not already calling
virtio_queue_set_rings().
Signed-off-by: Carlos López <clopez@suse.de>
---
hw/s390x/virtio-ccw.c | 1 +
hw/virtio/virtio-mmio.c | 5 ++---
hw/virtio/virtio-pci.c | 1 +
3 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index e33e5207ab..89891ac58a 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -237,6 +237,7 @@ static int virtio_ccw_set_vqs(SubchDev *sch, VqInfoBlock *info,
return -EINVAL;
}
virtio_queue_set_num(vdev, index, num);
+ virtio_queue_update_rings(vdev, index);
} else if (virtio_queue_get_num(vdev, index) > num) {
/* Fail if we don't have a big enough queue. */
return -EINVAL;
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 23ba625eb6..c74822308f 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -350,10 +350,9 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
case VIRTIO_MMIO_QUEUE_NUM:
trace_virtio_mmio_queue_write(value, VIRTQUEUE_MAX_SIZE);
virtio_queue_set_num(vdev, vdev->queue_sel, value);
+ virtio_queue_update_rings(vdev, vdev->queue_sel);
- if (proxy->legacy) {
- virtio_queue_update_rings(vdev, vdev->queue_sel);
- } else {
+ if (!proxy->legacy) {
proxy->vqs[vdev->queue_sel].num = value;
}
break;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 247325c193..a0a2f2c965 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1554,6 +1554,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
proxy->vqs[vdev->queue_sel].num = val;
virtio_queue_set_num(vdev, vdev->queue_sel,
proxy->vqs[vdev->queue_sel].num);
+ virtio_queue_update_rings(vdev, vdev->queue_sel);
break;
case VIRTIO_PCI_COMMON_Q_MSIX:
vector = virtio_queue_vector(vdev, vdev->queue_sel);
--
2.35.3
On Thu, Mar 02 2023, Carlos López <clopez@suse.de> wrote: > When a virtqueue size is changed by the guest via > virtio_queue_set_num(), its region cache is not automatically updated. > If the size was increased, this could lead to accessing the cache out > of bounds. For example, in vring_get_used_event(): > > static inline uint16_t vring_get_used_event(VirtQueue *vq) > { > return vring_avail_ring(vq, vq->vring.num); > } > > static inline uint16_t vring_avail_ring(VirtQueue *vq, int i) > { > VRingMemoryRegionCaches *caches = vring_get_region_caches(vq); > hwaddr pa = offsetof(VRingAvail, ring[i]); > > if (!caches) { > return 0; > } > > return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa); > } > > vq->vring.num will be greater than caches->avail.len, which will > trigger a failed assertion down the call path of > virtio_lduw_phys_cached(). > > Fix this by calling virtio_queue_update_rings() after > virtio_queue_set_num() if we are not already calling > virtio_queue_set_rings(). Don't we instead need to call virtio_init_region_cache() to update the caches? virtio_queue_set_rings() will calculate avail and used from desc, which looks wrong for modern devices. > > Signed-off-by: Carlos López <clopez@suse.de> > --- > hw/s390x/virtio-ccw.c | 1 + > hw/virtio/virtio-mmio.c | 5 ++--- > hw/virtio/virtio-pci.c | 1 + > 3 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index e33e5207ab..89891ac58a 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -237,6 +237,7 @@ static int virtio_ccw_set_vqs(SubchDev *sch, VqInfoBlock *info, > return -EINVAL; > } > virtio_queue_set_num(vdev, index, num); > + virtio_queue_update_rings(vdev, index); Note that this is the non-legacy path. > } else if (virtio_queue_get_num(vdev, index) > num) { > /* Fail if we don't have a big enough queue. */ > return -EINVAL; > diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c > index 23ba625eb6..c74822308f 100644 > --- a/hw/virtio/virtio-mmio.c > +++ b/hw/virtio/virtio-mmio.c > @@ -350,10 +350,9 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value, > case VIRTIO_MMIO_QUEUE_NUM: > trace_virtio_mmio_queue_write(value, VIRTQUEUE_MAX_SIZE); > virtio_queue_set_num(vdev, vdev->queue_sel, value); > + virtio_queue_update_rings(vdev, vdev->queue_sel); > > - if (proxy->legacy) { > - virtio_queue_update_rings(vdev, vdev->queue_sel); > - } else { > + if (!proxy->legacy) { > proxy->vqs[vdev->queue_sel].num = value; > } > break; > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 247325c193..a0a2f2c965 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1554,6 +1554,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr, > proxy->vqs[vdev->queue_sel].num = val; > virtio_queue_set_num(vdev, vdev->queue_sel, > proxy->vqs[vdev->queue_sel].num); > + virtio_queue_update_rings(vdev, vdev->queue_sel); > break; > case VIRTIO_PCI_COMMON_Q_MSIX: > vector = virtio_queue_vector(vdev, vdev->queue_sel);
On 9/3/23 11:43, Cornelia Huck wrote: > On Thu, Mar 02 2023, Carlos López <clopez@suse.de> wrote: >> Fix this by calling virtio_queue_update_rings() after >> virtio_queue_set_num() if we are not already calling >> virtio_queue_set_rings(). > > Don't we instead need to call virtio_init_region_cache() to update the > caches? virtio_queue_set_rings() will calculate avail and used from > desc, which looks wrong for modern devices. I take it you meant virtio_queue_update_rings() instead of virtio_queue_set_rings()? Otherwise I'm not sure what you mean. If this is the case sure - there is this same kind of logic in virtio_load(): /* * VIRTIO-1 devices migrate desc, used, and avail ring addresses so * only the region cache needs to be set up. Legacy devices need * to calculate used and avail ring addresses based on the desc * address. */ if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) { virtio_init_region_cache(vdev, i); } else { virtio_queue_update_rings(vdev, i); } This will require making virtio_init_region_cache() non static of course. >> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c >> index e33e5207ab..89891ac58a 100644 >> --- a/hw/s390x/virtio-ccw.c >> +++ b/hw/s390x/virtio-ccw.c >> @@ -237,6 +237,7 @@ static int virtio_ccw_set_vqs(SubchDev *sch, VqInfoBlock *info, >> return -EINVAL; >> } >> virtio_queue_set_num(vdev, index, num); >> + virtio_queue_update_rings(vdev, index); > > Note that this is the non-legacy path. > So if I understand correctly, in virtio_mmio_write() we check via proxy->legacy, and in virtio_ccw_set_vqs() we are in the non-legacy path. What about virtio_pci_common_write()? -- Carlos López Security Engineer SUSE Software Solutions
On Mon, Mar 13 2023, Carlos López <clopez@suse.de> wrote: > On 9/3/23 11:43, Cornelia Huck wrote: >> On Thu, Mar 02 2023, Carlos López <clopez@suse.de> wrote: >>> Fix this by calling virtio_queue_update_rings() after >>> virtio_queue_set_num() if we are not already calling >>> virtio_queue_set_rings(). >> >> Don't we instead need to call virtio_init_region_cache() to update the >> caches? virtio_queue_set_rings() will calculate avail and used from >> desc, which looks wrong for modern devices. > > I take it you meant virtio_queue_update_rings() instead of > virtio_queue_set_rings()? Otherwise I'm not sure what you mean. I think I had been looking at the code for too long :( > > If this is the case sure - there is this same kind of logic in > virtio_load(): > > /* > * VIRTIO-1 devices migrate desc, used, and avail ring > addresses so > * only the region cache needs to be set up. Legacy > devices need > * to calculate used and avail ring addresses based on the desc > * address. > */ > if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) { > virtio_init_region_cache(vdev, i); > } else { > virtio_queue_update_rings(vdev, i); > } Yes, I think we need to follow the same logic. > This will require making virtio_init_region_cache() non static of course. > >>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c >>> index e33e5207ab..89891ac58a 100644 >>> --- a/hw/s390x/virtio-ccw.c >>> +++ b/hw/s390x/virtio-ccw.c >>> @@ -237,6 +237,7 @@ static int virtio_ccw_set_vqs(SubchDev *sch, VqInfoBlock *info, >>> return -EINVAL; >>> } >>> virtio_queue_set_num(vdev, index, num); >>> + virtio_queue_update_rings(vdev, index); >> >> Note that this is the non-legacy path. >> > So if I understand correctly, in virtio_mmio_write() we check via > proxy->legacy, and in virtio_ccw_set_vqs() we are in the non-legacy > path. What about virtio_pci_common_write()? IIUC, only modern drivers will write to the modern bar.
On Thu, Mar 09, 2023 at 11:43:46AM +0100, Cornelia Huck wrote: > On Thu, Mar 02 2023, Carlos López <clopez@suse.de> wrote: > > > When a virtqueue size is changed by the guest via > > virtio_queue_set_num(), its region cache is not automatically updated. > > If the size was increased, this could lead to accessing the cache out > > of bounds. For example, in vring_get_used_event(): > > > > static inline uint16_t vring_get_used_event(VirtQueue *vq) > > { > > return vring_avail_ring(vq, vq->vring.num); > > } > > > > static inline uint16_t vring_avail_ring(VirtQueue *vq, int i) > > { > > VRingMemoryRegionCaches *caches = vring_get_region_caches(vq); > > hwaddr pa = offsetof(VRingAvail, ring[i]); > > > > if (!caches) { > > return 0; > > } > > > > return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa); > > } > > > > vq->vring.num will be greater than caches->avail.len, which will > > trigger a failed assertion down the call path of > > virtio_lduw_phys_cached(). > > > > Fix this by calling virtio_queue_update_rings() after > > virtio_queue_set_num() if we are not already calling > > virtio_queue_set_rings(). > > Don't we instead need to call virtio_init_region_cache() to update the > caches? virtio_queue_set_rings() will calculate avail and used from > desc, which looks wrong for modern devices. Carlos? > > > > Signed-off-by: Carlos López <clopez@suse.de> > > --- > > hw/s390x/virtio-ccw.c | 1 + > > hw/virtio/virtio-mmio.c | 5 ++--- > > hw/virtio/virtio-pci.c | 1 + > > 3 files changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > > index e33e5207ab..89891ac58a 100644 > > --- a/hw/s390x/virtio-ccw.c > > +++ b/hw/s390x/virtio-ccw.c > > @@ -237,6 +237,7 @@ static int virtio_ccw_set_vqs(SubchDev *sch, VqInfoBlock *info, > > return -EINVAL; > > } > > virtio_queue_set_num(vdev, index, num); > > + virtio_queue_update_rings(vdev, index); > > Note that this is the non-legacy path. > > > } else if (virtio_queue_get_num(vdev, index) > num) { > > /* Fail if we don't have a big enough queue. */ > > return -EINVAL; > > diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c > > index 23ba625eb6..c74822308f 100644 > > --- a/hw/virtio/virtio-mmio.c > > +++ b/hw/virtio/virtio-mmio.c > > @@ -350,10 +350,9 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value, > > case VIRTIO_MMIO_QUEUE_NUM: > > trace_virtio_mmio_queue_write(value, VIRTQUEUE_MAX_SIZE); > > virtio_queue_set_num(vdev, vdev->queue_sel, value); > > + virtio_queue_update_rings(vdev, vdev->queue_sel); > > > > - if (proxy->legacy) { > > - virtio_queue_update_rings(vdev, vdev->queue_sel); > > - } else { > > + if (!proxy->legacy) { > > proxy->vqs[vdev->queue_sel].num = value; > > } > > break; > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > index 247325c193..a0a2f2c965 100644 > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -1554,6 +1554,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr, > > proxy->vqs[vdev->queue_sel].num = val; > > virtio_queue_set_num(vdev, vdev->queue_sel, > > proxy->vqs[vdev->queue_sel].num); > > + virtio_queue_update_rings(vdev, vdev->queue_sel); > > break; > > case VIRTIO_PCI_COMMON_Q_MSIX: > > vector = virtio_queue_vector(vdev, vdev->queue_sel);
On 02/03/2023 11.14, Carlos López wrote: > When a virtqueue size is changed by the guest via > virtio_queue_set_num(), its region cache is not automatically updated. > If the size was increased, this could lead to accessing the cache out > of bounds. For example, in vring_get_used_event(): > > static inline uint16_t vring_get_used_event(VirtQueue *vq) > { > return vring_avail_ring(vq, vq->vring.num); > } > > static inline uint16_t vring_avail_ring(VirtQueue *vq, int i) > { > VRingMemoryRegionCaches *caches = vring_get_region_caches(vq); > hwaddr pa = offsetof(VRingAvail, ring[i]); > > if (!caches) { > return 0; > } > > return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa); > } > > vq->vring.num will be greater than caches->avail.len, which will > trigger a failed assertion down the call path of > virtio_lduw_phys_cached(). > > Fix this by calling virtio_queue_update_rings() after > virtio_queue_set_num() if we are not already calling > virtio_queue_set_rings(). > > Signed-off-by: Carlos López <clopez@suse.de> > --- > hw/s390x/virtio-ccw.c | 1 + > hw/virtio/virtio-mmio.c | 5 ++--- > hw/virtio/virtio-pci.c | 1 + > 3 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index e33e5207ab..89891ac58a 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -237,6 +237,7 @@ static int virtio_ccw_set_vqs(SubchDev *sch, VqInfoBlock *info, > return -EINVAL; > } > virtio_queue_set_num(vdev, index, num); > + virtio_queue_update_rings(vdev, index); > } else if (virtio_queue_get_num(vdev, index) > num) { > /* Fail if we don't have a big enough queue. */ > return -EINVAL; FWIW, s390 part: Acked-by: Thomas Huth <thuth@redhat.com>
© 2016 - 2024 Red Hat, Inc.