On Mon, 14 Sept 2026 at 10:17, Michael S. Tsirkin <[email protected]> wrote: > > On Mon, Sep 14, 2026 at 09:47:30AM +0100, Peter Maydell wrote: > > On Fri, 11 Sept 2026 at 10:01, Michael S. Tsirkin <[email protected]> wrote: > > > > > > When QEMU restores pending virtio requests from a migration stream, it > > > calls qemu_get_virtqueue_element(), which calls virtqueue_map() and > > > virtqueue_map_iovec() to re-map the saved DMA fragment addresses into > > > the destination address space. If the addresses in the migration stream > > > are corrupted, and as a result dma_memory_map() returns NULL or a > > > shorter-than-expected length, virtqueue_map_iovec() calls exit(1), > > > terminating the destination QEMU process instead of failing the > > > migration cleanly. > > > > > > Convert virtqueue_map_iovec() and virtqueue_map() from void to bool. > > > On failure, virtqueue_map_iovec() unmaps any entries it has already > > > mapped and returns false; virtqueue_map() similarly cleans up the in_sg > > > entries if out_sg mapping fails. qemu_get_virtqueue_element() now > > > checks the return value, frees the element and returns NULL on failure, > > > allowing the migration restore path to propagate a clean error. The > > > callers in virtio-blk, virtio-serial-bus and virtio-scsi that invoke > > > qemu_get_virtqueue_element() during load are updated to check for NULL > > > and return an error code. > > > > > > > Hi; Coverity points out an error in the error handling code > > added here (CID 1686764): > > > > > -static void virtqueue_map_iovec(VirtIODevice *vdev, struct iovec *sg, > > > +static bool virtqueue_map_iovec(VirtIODevice *vdev, struct iovec *sg, > > > hwaddr *addr, unsigned int num_sg, > > > bool is_write) > > > { > > > unsigned int i; > > > hwaddr len; > > > + DMADirection dir = is_write ? DMA_DIRECTION_FROM_DEVICE : > > > + DMA_DIRECTION_TO_DEVICE; > > > > > > for (i = 0; i < num_sg; i++) { > > > len = sg[i].iov_len; > > > - sg[i].iov_base = dma_memory_map(vdev->dma_as, > > > - addr[i], &len, is_write ? > > > - DMA_DIRECTION_FROM_DEVICE : > > > - DMA_DIRECTION_TO_DEVICE, > > > - MEMTXATTRS_UNSPECIFIED); > > > + sg[i].iov_base = dma_memory_map(vdev->dma_as, addr[i], &len, > > > + dir, MEMTXATTRS_UNSPECIFIED); > > > if (!sg[i].iov_base) { > > > error_report("virtio: error trying to map MMIO memory"); > > > - exit(1); > > > + goto err_undo_map; > > > > If dma_memory_map() fails on the first iteration, we will jump > > to err_undo_map, with i == 0.... > > > > > } > > > if (len != sg[i].iov_len) { > > > error_report("virtio: unexpected memory split"); > > > - exit(1); > > > + dma_memory_unmap(vdev->dma_as, sg[i].iov_base, len, dir, 0); > > > + goto err_undo_map; > > > } > > > } > > > + return true; > > > + > > > +err_undo_map: > > > + while (i-- > 0) { > > > > ...but since 'i' is an unsigned type, that will mean that "i--" > > here underflows to an extremely large value. This loop will > > then iterate many times unmapping random things. > > > I apologize, since when is 0 > 0 true?
Whoops, yes, I misread the code here. The loop counter will underflow, but we never use that underflowed result. Coverity complains just because it sees an underflow at all. I've reclassified the issue as false-positive; sorry for the noise. thanks -- PMM
