| Message ID | 20190719151920.22586-1-alxndr@bu.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] virtio-net: Always check for guest header length | expand |
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>
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
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
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>
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
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;
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(-)