Re: [PATCH v1] virtio-iommu: add error check before assert
On Wed, 12 Jun 2024 11:56, Alex Bennée wrote:
Manos Pitsidianakis writes:
On Tue, 11 Jun 2024 at 18:01, Philippe Mathieu-Daudé 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 0x8804
> outw 0xcfc 0x06
> outl 0xcf8 0x8820
> outl 0xcfc 0xe0004000
> write 0x1e 0x1 0x01
> write 0xe0004020 0x4 0x1000
> write 0xe0004028 0x4 0x00101000
> write 0xe000401c 0x1 0x01
> write 0x106000 0x1 0x05
> write 0x11 0x1 0x60
> write 0x12 0x1 0x10
> write 0x19 0x1 0x04
> write 0x1c 0x1 0x01
> write 0x100018 0x1 0x04
> write 0x10001c 0x1 0x02
> write 0x101003 0x1 0x01
> write 0xe0007001 0x1 0x00
> EOF
>
> Reported-by: Zheyu Ma
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2359
> Signed-off-by: Manos Pitsidianakis
> ---
> 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.
Re: [PATCH v1] virtio-iommu: add error check before assert
On Wed, 12 Jun 2024 12:46, Alex Bennée wrote:
Manos Pitsidianakis 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 0x8804
outw 0xcfc 0x06
outl 0xcf8 0x8820
outl 0xcfc 0xe0004000
write 0x1e 0x1 0x01
write 0xe0004020 0x4 0x1000
write 0xe0004028 0x4 0x00101000
write 0xe000401c 0x1 0x01
write 0x106000 0x1 0x05
write 0x11 0x1 0x60
write 0x12 0x1 0x10
write 0x19 0x1 0x04
write 0x1c 0x1 0x01
write 0x100018 0x1 0x04
write 0x10001c 0x1 0x02
write 0x101003 0x1 0x01
write 0xe0007001 0x1 0x00
EOF
Reported-by: Zheyu Ma
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2359
Signed-off-by: Manos Pitsidianakis
---
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
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?
The virtio-net.c patch adds an iov_size check for the virt queue element
to make sure it can fit a header len. In this function,
virtio_iommu_handle_command, a similar check is performed after popping
the element after the queue. That's what the "we checked that tail can
fit earlier" comment refers to. Is this what you were referring to by
any chance?
+sz = iov_from_buf(elem->in_sg,
+ elem->in_num,
+ 0,
+ &tail,
+ output_size);
+}
Isn't this the next element? Could we continue; instead?
It's not, the element is popped on the beginning of the for loop. I
think we should not continue because we have written a VIRTIO error
value for the guest and have to give it back as a response.
assert(sz == output_size);
virtqueue_push(vq, elem, sz);
base-commit: 80e8f0602168f451a93e71cbb1d59e93d745e62e
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
Re: [PATCH v1] virtio-iommu: add error check before assert
Manos Pitsidianakis 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 0x8804
> outw 0xcfc 0x06
> outl 0xcf8 0x8820
> outl 0xcfc 0xe0004000
> write 0x1e 0x1 0x01
> write 0xe0004020 0x4 0x1000
> write 0xe0004028 0x4 0x00101000
> write 0xe000401c 0x1 0x01
> write 0x106000 0x1 0x05
> write 0x11 0x1 0x60
> write 0x12 0x1 0x10
> write 0x19 0x1 0x04
> write 0x1c 0x1 0x01
> write 0x100018 0x1 0x04
> write 0x10001c 0x1 0x02
> write 0x101003 0x1 0x01
> write 0xe0007001 0x1 0x00
> EOF
>
> Reported-by: Zheyu Ma
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2359
> Signed-off-by: Manos Pitsidianakis
> ---
> 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
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
Re: [PATCH v1] virtio-iommu: add error check before assert
Manos Pitsidianakis writes:
> On Tue, 11 Jun 2024 at 18:01, Philippe Mathieu-Daudé
> 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 0x8804
>> > outw 0xcfc 0x06
>> > outl 0xcf8 0x8820
>> > outl 0xcfc 0xe0004000
>> > write 0x1e 0x1 0x01
>> > write 0xe0004020 0x4 0x1000
>> > write 0xe0004028 0x4 0x00101000
>> > write 0xe000401c 0x1 0x01
>> > write 0x106000 0x1 0x05
>> > write 0x11 0x1 0x60
>> > write 0x12 0x1 0x10
>> > write 0x19 0x1 0x04
>> > write 0x1c 0x1 0x01
>> > write 0x100018 0x1 0x04
>> > write 0x10001c 0x1 0x02
>> > write 0x101003 0x1 0x01
>> > write 0xe0007001 0x1 0x00
>> > EOF
>> >
>> > Reported-by: Zheyu Ma
>> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2359
>> > Signed-off-by: Manos Pitsidianakis
>> > ---
>> > 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.
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
Re: [PATCH v1] virtio-iommu: add error check before assert
On Tue, 11 Jun 2024 at 18:01, Philippe Mathieu-Daudé 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 0x8804
> > outw 0xcfc 0x06
> > outl 0xcf8 0x8820
> > outl 0xcfc 0xe0004000
> > write 0x1e 0x1 0x01
> > write 0xe0004020 0x4 0x1000
> > write 0xe0004028 0x4 0x00101000
> > write 0xe000401c 0x1 0x01
> > write 0x106000 0x1 0x05
> > write 0x11 0x1 0x60
> > write 0x12 0x1 0x10
> > write 0x19 0x1 0x04
> > write 0x1c 0x1 0x01
> > write 0x100018 0x1 0x04
> > write 0x10001c 0x1 0x02
> > write 0x101003 0x1 0x01
> > write 0xe0007001 0x1 0x00
> > EOF
> >
> > Reported-by: Zheyu Ma
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2359
> > Signed-off-by: Manos Pitsidianakis
> > ---
> > 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?
Re: [PATCH v1] virtio-iommu: add error check before assert
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 0x8804
outw 0xcfc 0x06
outl 0xcf8 0x8820
outl 0xcfc 0xe0004000
write 0x1e 0x1 0x01
write 0xe0004020 0x4 0x1000
write 0xe0004028 0x4 0x00101000
write 0xe000401c 0x1 0x01
write 0x106000 0x1 0x05
write 0x11 0x1 0x60
write 0x12 0x1 0x10
write 0x19 0x1 0x04
write 0x1c 0x1 0x01
write 0x100018 0x1 0x04
write 0x10001c 0x1 0x02
write 0x101003 0x1 0x01
write 0xe0007001 0x1 0x00
EOF
Reported-by: Zheyu Ma
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2359
Signed-off-by: Manos Pitsidianakis
---
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?
+tail.status = VIRTIO_IOMMU_S_DEVERR;
+/* We checked that tail can fit earlier */
+output_size = sizeof(tail);
+g_free(buf);
+buf = NULL;
+sz = iov_from_buf(elem->in_sg,
+ elem->in_num,
+ 0,
+ &tail,
+ output_size);
+}
assert(sz == output_size);
virtqueue_push(vq, elem, sz);
base-commit: 80e8f0602168f451a93e71cbb1d59e93d745e62e
[PATCH v1] virtio-iommu: add error check before assert
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 0x8804
outw 0xcfc 0x06
outl 0xcf8 0x8820
outl 0xcfc 0xe0004000
write 0x1e 0x1 0x01
write 0xe0004020 0x4 0x1000
write 0xe0004028 0x4 0x00101000
write 0xe000401c 0x1 0x01
write 0x106000 0x1 0x05
write 0x11 0x1 0x60
write 0x12 0x1 0x10
write 0x19 0x1 0x04
write 0x1c 0x1 0x01
write 0x100018 0x1 0x04
write 0x10001c 0x1 0x02
write 0x101003 0x1 0x01
write 0xe0007001 0x1 0x00
EOF
Reported-by: Zheyu Ma
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2359
Signed-off-by: Manos Pitsidianakis
---
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;
+sz = iov_from_buf(elem->in_sg,
+ elem->in_num,
+ 0,
+ &tail,
+ output_size);
+}
assert(sz == output_size);
virtqueue_push(vq, elem, sz);
base-commit: 80e8f0602168f451a93e71cbb1d59e93d745e62e
--
γαῖα πυρί μιχθήτω
