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

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

On Wed, 12 Jun 2024 11:56, Alex Bennée <alex.bennee@linaro.org> wrote:
>Manos Pitsidianakis <manos.pitsidianakis@linaro.org> writes:
>
>> 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?
>
>Yes logging guest errors are useful when attempting to work out if
>guests are buggy or QEMU is in the future.

Thanks Philippe and Alex, will send a v2 with a log print.