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

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

On Tue, 11 Jun 2024 at 18:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 11/6/24 14:23, Manos Pitsidianakis wrote:
> > 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)) {
>
> Is this a normal guest behavior? Should we log it as GUEST_ERROR?

It's not, it'd be a virtio spec (implementation) mis-use by the guest.
the Internal device error (VIRTIO_IOMMU_S_DEVERR) would be logged by
the kernel; should we log it as well?