Manos Pitsidianakis <manos.pitsidiana...@linaro.org> writes: > A fuzzer case discovered by Zheyu Ma causes an assert failure. > > Add a check before the assert, and respond with an error before moving > on to the next queue element. > > To reproduce the failure: > > cat << EOF | \ > qemu-system-x86_64 \ > -display none -machine accel=qtest -m 512M -machine q35 -nodefaults \ > -device virtio-iommu -qtest stdio > outl 0xcf8 0x80000804 > outw 0xcfc 0x06 > outl 0xcf8 0x80000820 > outl 0xcfc 0xe0004000 > write 0x10000e 0x1 0x01 > write 0xe0004020 0x4 0x00001000 > write 0xe0004028 0x4 0x00101000 > write 0xe000401c 0x1 0x01 > write 0x106000 0x1 0x05 > write 0x100001 0x1 0x60 > write 0x100002 0x1 0x10 > write 0x100009 0x1 0x04 > write 0x10000c 0x1 0x01 > write 0x100018 0x1 0x04 > write 0x10001c 0x1 0x02 > write 0x101003 0x1 0x01 > write 0xe0007001 0x1 0x00 > EOF > > Reported-by: Zheyu Ma <zheyum...@gmail.com> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2359 > Signed-off-by: Manos Pitsidianakis <manos.pitsidiana...@linaro.org> > --- > hw/virtio/virtio-iommu.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c > index 1326c6ec41..9b99def39f 100644 > --- a/hw/virtio/virtio-iommu.c > +++ b/hw/virtio/virtio-iommu.c > @@ -818,6 +818,18 @@ static void virtio_iommu_handle_command(VirtIODevice > *vdev, VirtQueue *vq) > out: > sz = iov_from_buf(elem->in_sg, elem->in_num, 0, > buf ? buf : &tail, output_size); > + if (unlikely(sz != output_size)) { > + tail.status = VIRTIO_IOMMU_S_DEVERR; > + /* We checked that tail can fit earlier */ > + output_size = sizeof(tail); > + g_free(buf); > + buf = NULL;
Hmm this is a similar pattern I noticed yesterday in: Message-ID: <20240527133140.218300-2-fro...@swemel.ru> Date: Mon, 27 May 2024 16:31:41 +0300 Subject: [PATCH] hw/net/virtio-net.c: fix crash in iov_copy() From: Dmitry Frolov <fro...@swemel.ru> And I wonder if the same comment applies. Could we clean-up the loop with autofrees to avoid making sure all the g_free() calls are properly lined up? > + sz = iov_from_buf(elem->in_sg, > + elem->in_num, > + 0, > + &tail, > + output_size); > + } Isn't this the next element? Could we continue; instead? > assert(sz == output_size); > > virtqueue_push(vq, elem, sz); > > base-commit: 80e8f0602168f451a93e71cbb1d59e93d745e62e -- Alex Bennée Virtualisation Tech Lead @ Linaro
