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 - 2026 Red Hat, Inc.