* [PATCH] virtio: refresh vring region cache after updating a virtqueue size
@ 2023-03-02 10:14 Carlos López
2023-03-06 11:36 ` Thomas Huth
2023-03-09 10:43 ` Cornelia Huck
0 siblings, 2 replies; 6+ messages in thread
From: Carlos López @ 2023-03-02 10:14 UTC (permalink / raw)
To: qemu-devel
Cc: Carlos López, Cornelia Huck, Halil Pasic, Eric Farman,
Michael S. Tsirkin, Richard Henderson, David Hildenbrand,
Ilya Leoshkevich, Christian Borntraeger, Thomas Huth,
open list:virtio-ccw
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio: refresh vring region cache after updating a virtqueue size
2023-03-02 10:14 [PATCH] virtio: refresh vring region cache after updating a virtqueue size Carlos López
@ 2023-03-06 11:36 ` Thomas Huth
2023-03-09 10:43 ` Cornelia Huck
1 sibling, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2023-03-06 11:36 UTC (permalink / raw)
To: Carlos López, qemu-devel, Michael S. Tsirkin
Cc: Cornelia Huck, Halil Pasic, Eric Farman, Richard Henderson,
David Hildenbrand, Ilya Leoshkevich, Christian Borntraeger,
open list:virtio-ccw
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio: refresh vring region cache after updating a virtqueue size
2023-03-02 10:14 [PATCH] virtio: refresh vring region cache after updating a virtqueue size Carlos López
2023-03-06 11:36 ` Thomas Huth
@ 2023-03-09 10:43 ` Cornelia Huck
2023-03-10 11:29 ` Michael S. Tsirkin
2023-03-13 10:19 ` Carlos López
1 sibling, 2 replies; 6+ messages in thread
From: Cornelia Huck @ 2023-03-09 10:43 UTC (permalink / raw)
To: Carlos López, qemu-devel
Cc: Carlos López, Halil Pasic, Eric Farman, Michael S. Tsirkin,
Richard Henderson, David Hildenbrand, Ilya Leoshkevich,
Christian Borntraeger, Thomas Huth, open list:virtio-ccw
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);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio: refresh vring region cache after updating a virtqueue size
2023-03-09 10:43 ` Cornelia Huck
@ 2023-03-10 11:29 ` Michael S. Tsirkin
2023-03-13 10:19 ` Carlos López
1 sibling, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2023-03-10 11:29 UTC (permalink / raw)
To: Cornelia Huck
Cc: Carlos López, qemu-devel, Halil Pasic, Eric Farman,
Richard Henderson, David Hildenbrand, Ilya Leoshkevich,
Christian Borntraeger, Thomas Huth, open list:virtio-ccw
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);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio: refresh vring region cache after updating a virtqueue size
2023-03-09 10:43 ` Cornelia Huck
2023-03-10 11:29 ` Michael S. Tsirkin
@ 2023-03-13 10:19 ` Carlos López
2023-03-15 13:11 ` Cornelia Huck
1 sibling, 1 reply; 6+ messages in thread
From: Carlos López @ 2023-03-13 10:19 UTC (permalink / raw)
To: Cornelia Huck, qemu-devel
Cc: Halil Pasic, Eric Farman, Michael S. Tsirkin, Richard Henderson,
David Hildenbrand, Ilya Leoshkevich, Christian Borntraeger,
Thomas Huth, open list:virtio-ccw
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio: refresh vring region cache after updating a virtqueue size
2023-03-13 10:19 ` Carlos López
@ 2023-03-15 13:11 ` Cornelia Huck
0 siblings, 0 replies; 6+ messages in thread
From: Cornelia Huck @ 2023-03-15 13:11 UTC (permalink / raw)
To: Carlos López, qemu-devel
Cc: Halil Pasic, Eric Farman, Michael S. Tsirkin, Richard Henderson,
David Hildenbrand, Ilya Leoshkevich, Christian Borntraeger,
Thomas Huth, open list:virtio-ccw
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-15 13:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-02 10:14 [PATCH] virtio: refresh vring region cache after updating a virtqueue size Carlos López
2023-03-06 11:36 ` Thomas Huth
2023-03-09 10:43 ` Cornelia Huck
2023-03-10 11:29 ` Michael S. Tsirkin
2023-03-13 10:19 ` Carlos López
2023-03-15 13:11 ` Cornelia Huck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.