[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] virtio: fix reachable assertion due to stale value of cac
From: |
Michael S. Tsirkin |
Subject: |
Re: [PATCH v2] virtio: fix reachable assertion due to stale value of cached region size |
Date: |
Tue, 7 Mar 2023 12:00:35 -0500 |
On Thu, Mar 02, 2023 at 11:03:59AM +0100, Carlos López wrote:
> In virtqueue_{split,packed}_get_avail_bytes() descriptors are read
> in a loop via MemoryRegionCache regions and calls to
> vring_{split,packed}_desc_read() - these take a region cache and the
> index of the descriptor to be read.
>
> For direct descriptors we use a cache provided by the caller, whose
> size matches that of the virtqueue vring. We limit the number of
> descriptors we can read by the size of that vring:
>
> max = vq->vring.num;
> ...
> MemoryRegionCache *desc_cache = &caches->desc;
>
> For indirect descriptors, we initialize a new cache and limit the
> number of descriptors by the size of the intermediate descriptor:
>
> len = address_space_cache_init(&indirect_desc_cache,
> vdev->dma_as,
> desc.addr, desc.len, false);
> desc_cache = &indirect_desc_cache;
> ...
> max = desc.len / sizeof(VRingDesc);
>
> However, the first initialization of `max` is done outside the loop
> where we process guest descriptors, while the second one is done
> inside. This means that a sequence of an indirect descriptor followed
> by a direct one will leave a stale value in `max`. If the second
> descriptor's `next` field is smaller than the stale value, but
> greater than the size of the virtqueue ring (and thus the cached
> region), a failed assertion will be triggered in
> address_space_read_cached() down the call chain.
>
> Fix this by initializing `max` inside the loop in both functions.
>
> Fixes: 9796d0ac8fb0 ("virtio: use address_space_map/unmap to access
> descriptors")
> Signed-off-by: Carlos López <clopez@suse.de>
> ---
> v2: simplify patch by removing variable cleanups
>
> hw/virtio/virtio.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index f35178f5fc..98c4819fcc 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1069,7 +1069,7 @@ static void virtqueue_split_get_avail_bytes(VirtQueue
> *vq,
> VRingMemoryRegionCaches *caches)
> {
> VirtIODevice *vdev = vq->vdev;
> - unsigned int max, idx;
> + unsigned int idx;
> unsigned int total_bufs, in_total, out_total;
> MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
> int64_t len = 0;
> @@ -1078,13 +1078,12 @@ static void virtqueue_split_get_avail_bytes(VirtQueue
> *vq,
> idx = vq->last_avail_idx;
> total_bufs = in_total = out_total = 0;
>
> - max = vq->vring.num;
> -
> while ((rc = virtqueue_num_heads(vq, idx)) > 0) {
> MemoryRegionCache *desc_cache = &caches->desc;
> unsigned int num_bufs;
> VRingDesc desc;
> unsigned int i;
> + unsigned int max = vq->vring.num;
>
> num_bufs = total_bufs;
>
> @@ -1206,7 +1205,7 @@ static void virtqueue_packed_get_avail_bytes(VirtQueue
> *vq,
> VRingMemoryRegionCaches *caches)
> {
> VirtIODevice *vdev = vq->vdev;
> - unsigned int max, idx;
> + unsigned int idx;
> unsigned int total_bufs, in_total, out_total;
> MemoryRegionCache *desc_cache;
> MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
> @@ -1218,14 +1217,14 @@ static void
> virtqueue_packed_get_avail_bytes(VirtQueue *vq,
> wrap_counter = vq->last_avail_wrap_counter;
> total_bufs = in_total = out_total = 0;
>
> - max = vq->vring.num;
> -
> for (;;) {
> unsigned int num_bufs = total_bufs;
> unsigned int i = idx;
> int rc;
> + unsigned int max = vq->vring.num;
>
> desc_cache = &caches->desc;
> +
OK and let's not add cosmetics with a bugfix unnecessarily?
> vring_packed_desc_read(vdev, &desc, desc_cache, idx, true);
> if (!is_desc_avail(desc.flags, wrap_counter)) {
> break;
> --
> 2.35.3