[Qemu-devel] [PATCH 0/1] Add check for header length in virtio-net-tx

[Qemu-devel] [PATCH 0/1] Add check for header length in virtio-net-tx

[Oleinik, Alexander]

While fuzzing the virtio-net tx vq, I ran into an assertion failure due
to iov_copy offsets larger than the total iov size. Though there is
a check to cover this, it does not execute when !n->has_vnet_hdr. This
patch tries to fix this. 

The call stack for the assertion failure:

#8 in __assert_fail (libc.so.6+0x300f1)
#9 in iov_copy iov.c:266:5
#10 in virtio_net_flush_tx virtio-net.c:2073:23
#11 in virtio_net_tx_bh virtio-net.c:2197:11
#12 in aio_bh_poll async.c:118:13
#13 in aio_dispatch aio-posix.c:460:5
#14 in aio_ctx_dispatch async.c:261:5
#15 in g_main_context_dispatch (libglib-2.0.so.0+0x4df2d)
#16 in glib_pollfds_poll main-loop.c:213:9
#17 in os_host_main_loop_wait main-loop.c:236
#18 in main_loop_wait main-loop.c:512
#19 in virtio_net_tx_fuzz virtio-net-fuzz.c:160:3

Thanks
-Alex

Alexander Oleinik (1):
  virtio-net: check guest header length is valid

 hw/net/virtio-net.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/1] virtio-net: check guest header length is valid

[Oleinik, Alexander]

virtio-net checks that the "out" sg is longer than the guest header, but
this check can be skipped if has_net_hdr is 0. Also perform this check
if host_hdr_len != guest_hdr_len

Signed-off-by: Alexander Oleinik <alxndr@bu.edu>
---
 hw/net/virtio-net.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index b9e1cd71cf..46d715b4f5 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2064,7 +2064,18 @@ 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) {
-            unsigned sg_num = iov_copy(sg, ARRAY_SIZE(sg),
+            unsigned sg_num;
+
+            if (!n->has_vnet_hdr) {
+                if (iov_to_buf(out_sg, out_num, 0, &mhdr, n->guest_hdr_len) <
+                    n->guest_hdr_len) {
+                    virtio_error(vdev, "virtio-net header incorrect");
+                    virtqueue_detach_element(q->tx_vq, elem, 0);
+                    g_free(elem);
+                    return -EINVAL;
+                }
+            }
+            sg_num = iov_copy(sg, ARRAY_SIZE(sg),
                                        out_sg, out_num,
                                        0, n->host_hdr_len);
             sg_num += iov_copy(sg + sg_num, ARRAY_SIZE(sg) - sg_num,
-- 
2.20.1

Re: [Qemu-devel] [PATCH 1/1] virtio-net: check guest header length is valid

[Michael S. Tsirkin]

On Tue, Jul 16, 2019 at 03:38:09AM +0000, Oleinik, Alexander wrote:
> virtio-net checks that the "out" sg is longer than the guest header, but
> this check can be skipped if has_net_hdr is 0. Also perform this check
> if host_hdr_len != guest_hdr_len

This explanation is way less clear than what you have in 0/1.
I suggest you just move the log here.

> 
> Signed-off-by: Alexander Oleinik <alxndr@bu.edu>
> ---
>  hw/net/virtio-net.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index b9e1cd71cf..46d715b4f5 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2064,7 +2064,18 @@ 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) {
> -            unsigned sg_num = iov_copy(sg, ARRAY_SIZE(sg),
> +            unsigned sg_num;
> +
> +            if (!n->has_vnet_hdr) {
> +                if (iov_to_buf(out_sg, out_num, 0, &mhdr, n->guest_hdr_len) <
> +                    n->guest_hdr_len) {
> +                    virtio_error(vdev, "virtio-net header incorrect");
> +                    virtqueue_detach_element(q->tx_vq, elem, 0);
> +                    g_free(elem);
> +                    return -EINVAL;
> +                }
> +            }

This code is duplicated from above. I also suspect some parts are missing
here (such as header endian-ness swap).
Pls find a way not to duplicate the code.


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

Re: [Qemu-devel] [PATCH 0/1] Add check for header length in virtio-net-tx

[Michael S. Tsirkin]

On Tue, Jul 16, 2019 at 03:38:00AM +0000, Oleinik, Alexander wrote:
> While fuzzing the virtio-net tx vq, I ran into an assertion failure due
> to iov_copy offsets larger than the total iov size. Though there is
> a check to cover this, it does not execute when !n->has_vnet_hdr. This
> patch tries to fix this. 
> 
> The call stack for the assertion failure:
> 
> #8 in __assert_fail (libc.so.6+0x300f1)
> #9 in iov_copy iov.c:266:5
> #10 in virtio_net_flush_tx virtio-net.c:2073:23
> #11 in virtio_net_tx_bh virtio-net.c:2197:11
> #12 in aio_bh_poll async.c:118:13
> #13 in aio_dispatch aio-posix.c:460:5
> #14 in aio_ctx_dispatch async.c:261:5
> #15 in g_main_context_dispatch (libglib-2.0.so.0+0x4df2d)
> #16 in glib_pollfds_poll main-loop.c:213:9
> #17 in os_host_main_loop_wait main-loop.c:236
> #18 in main_loop_wait main-loop.c:512
> #19 in virtio_net_tx_fuzz virtio-net-fuzz.c:160:3
> 
> Thanks
> -Alex

I think I'd rather introduce a variant of iov_copy
that returns the remaining offset.

the code that duplicates this for byte-swap seems to
also have this problem.


> Alexander Oleinik (1):
>   virtio-net: check guest header length is valid
> 
>  hw/net/virtio-net.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> -- 
> 2.20.1