Re: [PATCH v2] virtio: fix reachable assertion due to stale value of cached region sizey

[Alexander Bulekov]

On 230302 1103, 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>

OSS-Fuzz confirmed this fixed an issue:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=52152

Never made a gitlab report for it because, it  seemed very similar to
some other issues
https://gitlab.com/qemu-project/qemu/-/issues/301
https://gitlab.com/qemu-project/qemu/-/issues/781
https://gitlab.com/qemu-project/qemu/-/issues/300

But apparently it was a different one.
Thank you
-Alex