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? > > + dma_memory_unmap(vdev->dma_as, sg[i].iov_base, sg[i].iov_len, > > + dir, 0); > > + } > > + return false; > > } > > thanks > -- PMM
