Re: [PATCH v1] virtio-iommu: add error check before assert

["Alex Bennée" <alex.bennee@linaro.org>]

Manos Pitsidianakis <manos.pitsidianakis@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 <zheyuma97@gmail.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2359
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@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-frolov@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 <frolov@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