[v1] hw/net/virtio-net.c: fix crash in iov_copy()

[PATCH] hw/net/virtio-net.c: fix crash in iov_copy()

Posted by Dmitry Frolov 1 year, 10 months ago

A crash found while fuzzing device virtio-net-socket-check-used.
Assertion "offset == 0" in iov_copy() fails if less than guest_hdr_len bytes
were transmited.

Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
---
 hw/net/virtio-net.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 24e5e7d347..603b80a50a 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2783,6 +2783,12 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
          */
         assert(n->host_hdr_len <= n->guest_hdr_len);
         if (n->host_hdr_len != n->guest_hdr_len) {
+            if (iov_size(out_sg, out_num) < n->guest_hdr_len) {
+                virtio_error(vdev, "virtio-net header is invalid");
+                virtqueue_detach_element(q->tx_vq, elem, 0);
+                g_free(elem);
+                return -EINVAL;
+            }
             unsigned sg_num = iov_copy(sg, ARRAY_SIZE(sg),
                                        out_sg, out_num,
                                        0, n->host_hdr_len);
-- 
2.43.0

Re: [PATCH] hw/net/virtio-net.c: fix crash in iov_copy()

Posted by Дмитрий Фролов 1 year, 10 months ago

ping

https://patchew.org/QEMU/20240527133140.218300-2-frolov@swemel.ru/

On 27.05.2024 16:31, Dmitry Frolov wrote:
> A crash found while fuzzing device virtio-net-socket-check-used.
> Assertion "offset == 0" in iov_copy() fails if less than guest_hdr_len bytes
> were transmited.
>
> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> ---
>   hw/net/virtio-net.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 24e5e7d347..603b80a50a 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2783,6 +2783,12 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>            */
>           assert(n->host_hdr_len <= n->guest_hdr_len);
>           if (n->host_hdr_len != n->guest_hdr_len) {
> +            if (iov_size(out_sg, out_num) < n->guest_hdr_len) {
> +                virtio_error(vdev, "virtio-net header is invalid");
> +                virtqueue_detach_element(q->tx_vq, elem, 0);
> +                g_free(elem);
> +                return -EINVAL;
> +            }
>               unsigned sg_num = iov_copy(sg, ARRAY_SIZE(sg),
>                                          out_sg, out_num,
>                                          0, n->host_hdr_len);

Re: [PATCH] hw/net/virtio-net.c: fix crash in iov_copy()

Posted by Alex Bennée 1 year, 10 months ago

Дмитрий Фролов <frolov@swemel.ru> writes:

> ping
>
> https://patchew.org/QEMU/20240527133140.218300-2-frolov@swemel.ru/
>
> On 27.05.2024 16:31, Dmitry Frolov wrote:
>> A crash found while fuzzing device virtio-net-socket-check-used.
>> Assertion "offset == 0" in iov_copy() fails if less than guest_hdr_len bytes
>> were transmited.
>>
>> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
>> ---
>>   hw/net/virtio-net.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 24e5e7d347..603b80a50a 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -2783,6 +2783,12 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>>            */
>>           assert(n->host_hdr_len <= n->guest_hdr_len);
>>           if (n->host_hdr_len != n->guest_hdr_len) {
>> +            if (iov_size(out_sg, out_num) < n->guest_hdr_len) {
>> +                virtio_error(vdev, "virtio-net header is invalid");
>> +                virtqueue_detach_element(q->tx_vq, elem, 0);
>> +                g_free(elem);
>> +                return -EINVAL;
>> +            }

Isn't this basically another case for goto detach?

Although the use of goto's here is a bit of a code smell. I wonder if
there is any way to better structure this function and take care of the
auto-freeing of elements?

>>               unsigned sg_num = iov_copy(sg, ARRAY_SIZE(sg),
>>                                          out_sg, out_num,
>>                                          0, n->host_hdr_len);

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro