Re: [PATCH] virtio: update MemoryRegionCaches when guest set bad features

[Paolo Bonzini]

On 19/09/20 10:27, Li Qiang wrote:
> Current the 'virtio_set_features' only update the 'MemorRegionCaches'
> when the 'virtio_set_features_nocheck' return '0' which means it is
> not bad features. However the guest can still trigger the access of the
> used vring after set bad features. In this situation it will cause assert
> failure in 'ADDRESS_SPACE_ST_CACHED'.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1890333
> Fixes: db812c4073c7 ("virtio: update MemoryRegionCaches when guest negotiates 
> features")
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  hw/virtio/virtio.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index e983025217..4441ae5ed4 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2963,17 +2963,16 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t 
> val)
>          return -EINVAL;
>      }
>      ret = virtio_set_features_nocheck(vdev, val);
> -    if (!ret) {
> -        if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
> -            /* VIRTIO_RING_F_EVENT_IDX changes the size of the caches.  */
> -            int i;
> -            for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> -                if (vdev->vq[i].vring.num != 0) {
> -                    virtio_init_region_cache(vdev, i);
> -                }
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
> +        /* VIRTIO_RING_F_EVENT_IDX changes the size of the caches.  */
> +        int i;
> +        for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> +            if (vdev->vq[i].vring.num != 0) {
> +                virtio_init_region_cache(vdev, i);
>              }
>          }
> -
> +    }
> +    if (!ret) {
>          if (!virtio_device_started(vdev, vdev->status) &&
>              !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>              vdev->start_on_kick = true;
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>