* [Qemu-devel] [PATCH v2] virtio-net: Always check for guest header length
@ 2019-07-19 15:21 Oleinik, Alexander
2019-07-22 7:26 ` Stefan Hajnoczi
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: Oleinik, Alexander @ 2019-07-19 15:21 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Jason Wang, Oleinik, Alexander, bsd,
stefanha, pbonzini
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
change always copies the guest header into the mhdr buffer and checks its
length, even if mhdr is not needed.
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
Signed-off-by: Alexander Oleinik <alxndr@bu.edu>
---
hw/net/virtio-net.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index b9e1cd71cf..003436b53c 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2035,14 +2035,19 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
return -EINVAL;
}
- if (n->has_vnet_hdr) {
- if (iov_to_buf(out_sg, out_num, 0, &mhdr, n->guest_hdr_len) <
+ /*
+ * Even if !n->has_vnet_hdr and we dont need mhdr, we still need this
+ * to check that out_sg contains at least guest_hdr_len bytes
+ */
+ 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;
- }
+ virtio_error(vdev, "virtio-net header incorrect");
+ virtqueue_detach_element(q->tx_vq, elem, 0);
+ g_free(elem);
+ return -EINVAL;
+ }
+
+ if (n->has_vnet_hdr) {
if (n->needs_vnet_hdr_swap) {
virtio_net_hdr_swap(vdev, (void *) &mhdr);
sg2[0].iov_base = &mhdr;
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] virtio-net: Always check for guest header length
2019-07-19 15:21 [Qemu-devel] [PATCH v2] virtio-net: Always check for guest header length Oleinik, Alexander
@ 2019-07-22 7:26 ` Stefan Hajnoczi
2019-07-22 7:28 ` Stefan Hajnoczi
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2019-07-22 7:26 UTC (permalink / raw)
To: Oleinik, Alexander
Cc: Jason Wang, pbonzini, bsd, qemu-devel, Michael S. Tsirkin
[-- Attachment #1: Type: text/plain, Size: 1230 bytes --]
On Fri, Jul 19, 2019 at 03:21:15PM +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
> change always copies the guest header into the mhdr buffer and checks its
> length, even if mhdr is not needed.
>
> 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
>
> Signed-off-by: Alexander Oleinik <alxndr@bu.edu>
> ---
> hw/net/virtio-net.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] virtio-net: Always check for guest header length
2019-07-19 15:21 [Qemu-devel] [PATCH v2] virtio-net: Always check for guest header length Oleinik, Alexander
2019-07-22 7:26 ` Stefan Hajnoczi
@ 2019-07-22 7:28 ` Stefan Hajnoczi
2019-07-22 8:59 ` Michael S. Tsirkin
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2019-07-22 7:28 UTC (permalink / raw)
To: Oleinik, Alexander
Cc: Jason Wang, pbonzini, bsd, qemu-devel, Michael S. Tsirkin
[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]
On Fri, Jul 19, 2019 at 03:21:15PM +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
> change always copies the guest header into the mhdr buffer and checks its
> length, even if mhdr is not needed.
>
> 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
>
> Signed-off-by: Alexander Oleinik <alxndr@bu.edu>
> ---
> hw/net/virtio-net.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
Please include a changelog below the '---' line in future patches. For
example:
---
v2:
* Unconditionally copy guest vnet_hdr because the size must always be
checked
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] virtio-net: Always check for guest header length
2019-07-19 15:21 [Qemu-devel] [PATCH v2] virtio-net: Always check for guest header length Oleinik, Alexander
2019-07-22 7:26 ` Stefan Hajnoczi
2019-07-22 7:28 ` Stefan Hajnoczi
@ 2019-07-22 8:59 ` Michael S. Tsirkin
2019-07-22 9:41 ` Stefano Garzarella
2019-07-22 9:45 ` Stefano Garzarella
4 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2019-07-22 8:59 UTC (permalink / raw)
To: Oleinik, Alexander; +Cc: Jason Wang, pbonzini, bsd, qemu-devel, stefanha
On Fri, Jul 19, 2019 at 03:21:15PM +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
> change always copies the guest header into the mhdr buffer and checks its
> length, even if mhdr is not needed.
>
> 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
>
> Signed-off-by: Alexander Oleinik <alxndr@bu.edu>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/net/virtio-net.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index b9e1cd71cf..003436b53c 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2035,14 +2035,19 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> return -EINVAL;
> }
>
> - if (n->has_vnet_hdr) {
> - if (iov_to_buf(out_sg, out_num, 0, &mhdr, n->guest_hdr_len) <
> + /*
> + * Even if !n->has_vnet_hdr and we dont need mhdr, we still need this
> + * to check that out_sg contains at least guest_hdr_len bytes
> + */
> + 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;
> - }
> + virtio_error(vdev, "virtio-net header incorrect");
> + virtqueue_detach_element(q->tx_vq, elem, 0);
> + g_free(elem);
> + return -EINVAL;
> + }
> +
> + if (n->has_vnet_hdr) {
> if (n->needs_vnet_hdr_swap) {
> virtio_net_hdr_swap(vdev, (void *) &mhdr);
> sg2[0].iov_base = &mhdr;
> --
> 2.20.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] virtio-net: Always check for guest header length
2019-07-19 15:21 [Qemu-devel] [PATCH v2] virtio-net: Always check for guest header length Oleinik, Alexander
` (2 preceding siblings ...)
2019-07-22 8:59 ` Michael S. Tsirkin
@ 2019-07-22 9:41 ` Stefano Garzarella
2019-07-22 9:45 ` Stefano Garzarella
4 siblings, 0 replies; 6+ messages in thread
From: Stefano Garzarella @ 2019-07-22 9:41 UTC (permalink / raw)
To: Oleinik, Alexander
Cc: Michael S. Tsirkin, Jason Wang, qemu-devel, bsd, stefanha, pbonzini
On Fri, Jul 19, 2019 at 03:21:15PM +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
> change always copies the guest header into the mhdr buffer and checks its
> length, even if mhdr is not needed.
>
> 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
>
> Signed-off-by: Alexander Oleinik <alxndr@bu.edu>
> ---
> hw/net/virtio-net.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] virtio-net: Always check for guest header length
2019-07-19 15:21 [Qemu-devel] [PATCH v2] virtio-net: Always check for guest header length Oleinik, Alexander
` (3 preceding siblings ...)
2019-07-22 9:41 ` Stefano Garzarella
@ 2019-07-22 9:45 ` Stefano Garzarella
4 siblings, 0 replies; 6+ messages in thread
From: Stefano Garzarella @ 2019-07-22 9:45 UTC (permalink / raw)
To: Oleinik, Alexander
Cc: Michael S. Tsirkin, Jason Wang, qemu-devel, bsd, stefanha, pbonzini
On Fri, Jul 19, 2019 at 03:21:15PM +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
> change always copies the guest header into the mhdr buffer and checks its
> length, even if mhdr is not needed.
>
> 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
>
> Signed-off-by: Alexander Oleinik <alxndr@bu.edu>
> ---
> hw/net/virtio-net.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index b9e1cd71cf..003436b53c 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2035,14 +2035,19 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> return -EINVAL;
> }
>
> - if (n->has_vnet_hdr) {
> - if (iov_to_buf(out_sg, out_num, 0, &mhdr, n->guest_hdr_len) <
> + /*
> + * Even if !n->has_vnet_hdr and we dont need mhdr, we still need this
> + * to check that out_sg contains at least guest_hdr_len bytes
> + */
> + 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;
> - }
> + virtio_error(vdev, "virtio-net header incorrect");
> + virtqueue_detach_element(q->tx_vq, elem, 0);
> + g_free(elem);
> + return -EINVAL;
> + }
> +
> + if (n->has_vnet_hdr) {
> if (n->needs_vnet_hdr_swap) {
Should we merge the conditions? In this way:
if (n->has_vnet_hdr && n->needs_vnet_hdr_swap) {
Thanks,
Stefano
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-22 9:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-19 15:21 [Qemu-devel] [PATCH v2] virtio-net: Always check for guest header length Oleinik, Alexander
2019-07-22 7:26 ` Stefan Hajnoczi
2019-07-22 7:28 ` Stefan Hajnoczi
2019-07-22 8:59 ` Michael S. Tsirkin
2019-07-22 9:41 ` Stefano Garzarella
2019-07-22 9:45 ` Stefano Garzarella
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.