On Tue, 7 Mar 2017 16:47:58 +0800
Jason Wang <jasow...@redhat.com> wrote:
> We don't destroy region cache during reset which can make the maps
> of previous driver leaked to a buggy or malicious driver that don't
> set vring address before starting to use the device. Fix this by
> destroy the region cache during reset and validate it before trying to
> use them. While at it, also validate address_space_cache_init() during
> virtio_init_region_cache() to make sure we have a correct region
> cache.
>
> Signed-off-by: Jason Wang <jasow...@redhat.com>
> ---
> hw/virtio/virtio.c | 88
> ++++++++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 76 insertions(+), 12 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 09f4cf4..90324f6 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -131,6 +131,7 @@ static void virtio_init_region_cache(VirtIODevice *vdev,
> int n)
> VRingMemoryRegionCaches *new;
> hwaddr addr, size;
> int event_size;
> + int64_t len;
>
> event_size = virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVENT_IDX)
> ? 2 : 0;
>
> @@ -140,21 +141,41 @@ static void virtio_init_region_cache(VirtIODevice
> *vdev, int n)
> }
> new = g_new0(VRingMemoryRegionCaches, 1);
> size = virtio_queue_get_desc_size(vdev, n);
> - address_space_cache_init(&new->desc, vdev->dma_as,
> - addr, size, false);
> + len = address_space_cache_init(&new->desc, vdev->dma_as,
> + addr, size, false);
> + if (len < size) {
> + virtio_error(vdev, "Cannot map desc");
> + goto err_desc;
> + }
>
> size = virtio_queue_get_used_size(vdev, n) + event_size;
> - address_space_cache_init(&new->used, vdev->dma_as,
> - vq->vring.used, size, true);
> + len = address_space_cache_init(&new->used, vdev->dma_as,
> + vq->vring.used, size, true);
> + if (len < size) {
> + virtio_error(vdev, "Cannot map used");
> + goto err_used;
> + }
>
> size = virtio_queue_get_avail_size(vdev, n) + event_size;
> - address_space_cache_init(&new->avail, vdev->dma_as,
> - vq->vring.avail, size, false);
> + len = address_space_cache_init(&new->avail, vdev->dma_as,
> + vq->vring.avail, size, false);
> + if (len < size) {
> + virtio_error(vdev, "Cannot map avail");
> + goto err_avail;
> + }
>
> atomic_rcu_set(&vq->vring.caches, new);
> if (old) {
> call_rcu(old, virtio_free_region_cache, rcu);
> }
> + return;
> +
> +err_avail:
> + address_space_cache_destroy(&new->used);
> +err_used:
> + address_space_cache_destroy(&new->desc);
> +err_desc:
> + g_free(new);
> }
I think it would be more readable if you moved adding this check (which
is a good idea) into a separate patch.
>
> /* virt queue functions */
> @@ -190,6 +211,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
> {
> VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
> hwaddr pa = offsetof(VRingAvail, flags);
> + if (!caches) {
> + virtio_error(vq->vdev, "Cannot map avail flags");
I'm not sure that virtio_error is the right thing here; ending up in
this function with !caches indicates an error in our logic. An assert
might be better (and I hope we can sort out all of those errors exposed
by the introduction of region caches for 2.9...)
> + return 0;
> + }
> return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
> }
>
> @@ -198,6 +223,10 @@ static inline uint16_t vring_avail_idx(VirtQueue *vq)
> {
> VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
> hwaddr pa = offsetof(VRingAvail, idx);
> + if (!caches) {
> + virtio_error(vq->vdev, "Cannot map avail idx");
> + return vq->shadow_avail_idx;
> + }
> vq->shadow_avail_idx = virtio_lduw_phys_cached(vq->vdev, &caches->avail,
> pa);
> return vq->shadow_avail_idx;
> }
> @@ -207,6 +236,10 @@ static inline uint16_t vring_avail_ring(VirtQueue *vq,
> int i)
> {
> VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
> hwaddr pa = offsetof(VRingAvail, ring[i]);
> + if (!caches) {
> + virtio_error(vq->vdev, "Cannot map avail ring");
> + return 0;
> + }
> return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
> }
>
> @@ -222,6 +255,10 @@ static inline void vring_used_write(VirtQueue *vq,
> VRingUsedElem *uelem,
> {
> VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
> hwaddr pa = offsetof(VRingUsed, ring[i]);
> + if (!caches) {
> + virtio_error(vq->vdev, "Cannot map used ring");
> + return;
> + }
> virtio_tswap32s(vq->vdev, &uelem->id);
> virtio_tswap32s(vq->vdev, &uelem->len);
> address_space_write_cached(&caches->used, pa, uelem,
> sizeof(VRingUsedElem));
> @@ -233,6 +270,10 @@ static uint16_t vring_used_idx(VirtQueue *vq)
> {
> VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
> hwaddr pa = offsetof(VRingUsed, idx);
> + if (!caches) {
> + virtio_error(vq->vdev, "Cannot map used ring");
> + return 0;
> + }
> return virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
> }
>
> @@ -241,6 +282,10 @@ static inline void vring_used_idx_set(VirtQueue *vq,
> uint16_t val)
> {
> VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
> hwaddr pa = offsetof(VRingUsed, idx);
> + if (!caches) {
> + virtio_error(vq->vdev, "Cannot map used idx");
> + return;
> + }
> virtio_stw_phys_cached(vq->vdev, &caches->used, pa, val);
> address_space_cache_invalidate(&caches->used, pa, sizeof(val));
> vq->used_idx = val;
> @@ -254,6 +299,10 @@ static inline void vring_used_flags_set_bit(VirtQueue
> *vq, int mask)
> hwaddr pa = offsetof(VRingUsed, flags);
> uint16_t flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
>
> + if (!caches) {
> + virtio_error(vq->vdev, "Cannot map used flags");
Regardless of whether using virtio_error here is fine: caches was
already dereferenced above...
> + return;
> + }
> virtio_stw_phys_cached(vdev, &caches->used, pa, flags | mask);
> address_space_cache_invalidate(&caches->used, pa, sizeof(flags));
> }
> @@ -266,6 +315,10 @@ static inline void vring_used_flags_unset_bit(VirtQueue
> *vq, int mask)
> hwaddr pa = offsetof(VRingUsed, flags);
> uint16_t flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, pa);
>
> + if (!caches) {
> + virtio_error(vq->vdev, "Cannot map used flags");
dito
> + return;
> + }
> virtio_stw_phys_cached(vdev, &caches->used, pa, flags & ~mask);
> address_space_cache_invalidate(&caches->used, pa, sizeof(flags));
> }
> @@ -280,6 +333,10 @@ static inline void vring_set_avail_event(VirtQueue *vq,
> uint16_t val)
> }
>
> caches = atomic_rcu_read(&vq->vring.caches);
> + if (!caches) {
> + virtio_error(vq->vdev, "Cannot map avail event");
> + return;
> + }
> pa = offsetof(VRingUsed, ring[vq->vring.num]);
> virtio_stw_phys_cached(vq->vdev, &caches->used, pa, val);
> address_space_cache_invalidate(&caches->used, pa, sizeof(val));
> @@ -552,7 +609,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned
> int *in_bytes,
>
> max = vq->vring.num;
> caches = atomic_rcu_read(&vq->vring.caches);
> - if (caches->desc.len < max * sizeof(VRingDesc)) {
> + if (!caches || caches->desc.len < max * sizeof(VRingDesc)) {
> virtio_error(vdev, "Cannot map descriptor ring");
> goto err;
> }
> @@ -819,7 +876,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
> i = head;
>
> caches = atomic_rcu_read(&vq->vring.caches);
> - if (caches->desc.len < max * sizeof(VRingDesc)) {
> + if (!caches || caches->desc.len < max * sizeof(VRingDesc)) {
> virtio_error(vdev, "Cannot map descriptor ring");
> goto done;
> }
> @@ -1117,6 +1174,15 @@ static enum virtio_device_endian
> virtio_current_cpu_endian(void)
> }
> }
>
> +static void virtio_virtqueue_reset_region_cache(struct VirtQueue *vq)
> +{
> + VRingMemoryRegionCaches *caches;
> +
> + caches = atomic_read(&vq->vring.caches);
> + atomic_set(&vq->vring.caches, NULL);
> + virtio_free_region_cache(caches);
Shouldn't this use rcu to free it? Unconditionally setting caches to
NULL feels wrong...
> +}
> +
> void virtio_reset(void *opaque)
> {
> VirtIODevice *vdev = opaque;
> @@ -1157,6 +1223,7 @@ void virtio_reset(void *opaque)
> vdev->vq[i].notification = true;
> vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
> vdev->vq[i].inuse = 0;
> + virtio_virtqueue_reset_region_cache(&vdev->vq[i]);
...especially as you call it in a reset context here.
> }
> }
>
> @@ -2451,13 +2518,10 @@ static void
> virtio_device_free_virtqueues(VirtIODevice *vdev)
> }
>
> for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> - VRingMemoryRegionCaches *caches;
> if (vdev->vq[i].vring.num == 0) {
> break;
> }
> - caches = atomic_read(&vdev->vq[i].vring.caches);
> - atomic_set(&vdev->vq[i].vring.caches, NULL);
> - virtio_free_region_cache(caches);
> + virtio_virtqueue_reset_region_cache(&vdev->vq[i]);
OTOH, immediate destruction may still be called for during device
finalization.
> }
> g_free(vdev->vq);
> }
