[Qemu-devel] [PATCH] virtio-net: correct header length math

[Qemu-devel] [PATCH] virtio-net: correct header length math

[Michael S. Tsirkin]

We were requesting too much when checking buffer
length: size already includes host header length.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio-net.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 06ba481..2646c87 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -527,17 +527,18 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
 {
     VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
     struct virtio_net_hdr_mrg_rxbuf *mhdr = NULL;
-    size_t hdr_len, offset, i;
+    size_t guest_hdr_len, offset, i, host_hdr_len;
 
     if (!virtio_net_can_receive(&n->nic->nc))
         return -1;
 
     /* hdr_len refers to the header we supply to the guest */
-    hdr_len = n->mergeable_rx_bufs ?
+    guest_hdr_len = n->mergeable_rx_bufs ?
         sizeof(struct virtio_net_hdr_mrg_rxbuf) : sizeof(struct virtio_net_hdr);
 
 
-    if (!virtio_net_has_buffers(n, size + hdr_len))
+    host_hdr_len = n->has_vnet_hdr ? sizeof(struct virtio_net_hdr) : 0;
+    if (!virtio_net_has_buffers(n, size + guest_hdr_len - host_hdr_len))
         return 0;
 
     if (!receive_filter(n, buf, size))
@@ -556,9 +557,10 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
             virtqueue_pop(n->rx_vq, &elem) == 0) {
             if (i == 0)
                 return -1;
-            fprintf(stderr, "virtio-net truncating packet: "
-                    "offset %zd, size %zd, hdr_len %zd\n",
-                    offset, size, hdr_len);
+            fprintf(stderr, "virtio-net truncating packet: i %d mergeable %d "
+                    "offset %zd, size %zd, guest hdr %zd, host hdr %zd\n",
+		    i, n->mergeable_rx_bufs,
+                    offset, size, guest_hdr_len, host_hdr_len);
             exit(1);
         }
 
@@ -567,7 +569,7 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
             exit(1);
         }
 
-        if (!n->mergeable_rx_bufs && elem.in_sg[0].iov_len != hdr_len) {
+        if (!n->mergeable_rx_bufs && elem.in_sg[0].iov_len != guest_hdr_len) {
             fprintf(stderr, "virtio-net header not in first element\n");
             exit(1);
         }
@@ -579,8 +581,8 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
                 mhdr = (struct virtio_net_hdr_mrg_rxbuf *)sg[0].iov_base;
 
             offset += receive_header(n, sg, elem.in_num,
-                                     buf + offset, size - offset, hdr_len);
-            total += hdr_len;
+                                     buf + offset, size - offset, guest_hdr_len);
+            total += guest_hdr_len;
         }
 
         /* copy in packet.  ugh */
-- 
1.7.1.12.g42b7f

[Qemu-devel] Re: [PATCH] virtio-net: correct header length math

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> We were requesting too much when checking buffer
> length: size already includes host header length.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

All changes here are  ok, but we are still missing more checks.

> ---
>  hw/virtio-net.c |   20 +++++++++++---------
>  1 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 06ba481..2646c87 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -527,17 +527,18 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
>  {
>      VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
>      struct virtio_net_hdr_mrg_rxbuf *mhdr = NULL;
> -    size_t hdr_len, offset, i;
> +    size_t guest_hdr_len, offset, i, host_hdr_len;
>  
>      if (!virtio_net_can_receive(&n->nic->nc))
>          return -1;
>  
>      /* hdr_len refers to the header we supply to the guest */
> -    hdr_len = n->mergeable_rx_bufs ?
> +    guest_hdr_len = n->mergeable_rx_bufs ?
>          sizeof(struct virtio_net_hdr_mrg_rxbuf) : sizeof(struct virtio_net_hdr);
>  
>  

nitpit: empty extra line

> -    if (!virtio_net_has_buffers(n, size + hdr_len))
> +    host_hdr_len = n->has_vnet_hdr ? sizeof(struct virtio_net_hdr) : 0;
> +    if (!virtio_net_has_buffers(n, size + guest_hdr_len - host_hdr_len))
>          return 0;
>  
>      if (!receive_filter(n, buf, size))

size is not used by receive_filter.  We are assuming that size is at
least 16 + sizeof(struct virtio_net_hdr).

    while (offset < size) {

we are still testing offset with size, but we read headers from there
also :(

Later, Juan.

[Qemu-devel] Re: [PATCH] virtio-net: correct header length math

[Michael S. Tsirkin]

On Wed, Jun 23, 2010 at 05:57:13PM +0200, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > We were requesting too much when checking buffer
> > length: size already includes host header length.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> All changes here are  ok, but we are still missing more checks.
> 
> > ---
> >  hw/virtio-net.c |   20 +++++++++++---------
> >  1 files changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > index 06ba481..2646c87 100644
> > --- a/hw/virtio-net.c
> > +++ b/hw/virtio-net.c
> > @@ -527,17 +527,18 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
> >  {
> >      VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
> >      struct virtio_net_hdr_mrg_rxbuf *mhdr = NULL;
> > -    size_t hdr_len, offset, i;
> > +    size_t guest_hdr_len, offset, i, host_hdr_len;
> >  
> >      if (!virtio_net_can_receive(&n->nic->nc))
> >          return -1;
> >  
> >      /* hdr_len refers to the header we supply to the guest */
> > -    hdr_len = n->mergeable_rx_bufs ?
> > +    guest_hdr_len = n->mergeable_rx_bufs ?
> >          sizeof(struct virtio_net_hdr_mrg_rxbuf) : sizeof(struct virtio_net_hdr);
> >  
> >  
> 
> nitpit: empty extra line
> 
> > -    if (!virtio_net_has_buffers(n, size + hdr_len))
> > +    host_hdr_len = n->has_vnet_hdr ? sizeof(struct virtio_net_hdr) : 0;
> > +    if (!virtio_net_has_buffers(n, size + guest_hdr_len - host_hdr_len))
> >          return 0;
> >  
> >      if (!receive_filter(n, buf, size))
> 
> size is not used by receive_filter.  We are assuming that size is at
> least 16 + sizeof(struct virtio_net_hdr).

True, I guess receive_filter should be fixed.

> 
>     while (offset < size) {
> 
> we are still testing offset with size, but we read headers from there
> also :(

>From where? As far as I can tell we always read size bytes from buf. No?

> Later, Juan.

[Qemu-devel] Re: [PATCH] virtio-net: correct header length math

[Amit Shah]

On (Wed) Jun 23 2010 [12:51:18], Michael S. Tsirkin wrote:
> We were requesting too much when checking buffer
> length: size already includes host header length.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/virtio-net.c |   20 +++++++++++---------
>  1 files changed, 11 insertions(+), 9 deletions(-)

Couple of minor comments. But I'll still provide the

ACK

> @@ -556,9 +557,10 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
>              virtqueue_pop(n->rx_vq, &elem) == 0) {
>              if (i == 0)
>                  return -1;
> -            fprintf(stderr, "virtio-net truncating packet: "
> -                    "offset %zd, size %zd, hdr_len %zd\n",
> -                    offset, size, hdr_len);
> +            fprintf(stderr, "virtio-net truncating packet: i %d mergeable %d "
> +                    "offset %zd, size %zd, guest hdr %zd, host hdr %zd\n",
> +		    i, n->mergeable_rx_bufs,
> +                    offset, size, guest_hdr_len, host_hdr_len);

Again, tab usage and 'guest hdr len' and 'host hdr len' make better
sense than 'guest hdr' and 'host hdr'

>              exit(1);

Can we drop the packet(s) instead of exiting?

		Amit

[Qemu-devel] Re: [PATCH] virtio-net: correct header length math

[Michael S. Tsirkin]

On Thu, Jun 24, 2010 at 02:12:13PM +0530, Amit Shah wrote:
> On (Wed) Jun 23 2010 [12:51:18], Michael S. Tsirkin wrote:
> > We were requesting too much when checking buffer
> > length: size already includes host header length.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/virtio-net.c |   20 +++++++++++---------
> >  1 files changed, 11 insertions(+), 9 deletions(-)
> 
> Couple of minor comments. But I'll still provide the
> 
> ACK
> 
> > @@ -556,9 +557,10 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
> >              virtqueue_pop(n->rx_vq, &elem) == 0) {
> >              if (i == 0)
> >                  return -1;
> > -            fprintf(stderr, "virtio-net truncating packet: "
> > -                    "offset %zd, size %zd, hdr_len %zd\n",
> > -                    offset, size, hdr_len);
> > +            fprintf(stderr, "virtio-net truncating packet: i %d mergeable %d "
> > +                    "offset %zd, size %zd, guest hdr %zd, host hdr %zd\n",
> > +		    i, n->mergeable_rx_bufs,
> > +                    offset, size, guest_hdr_len, host_hdr_len);
> 
> Again, tab usage and 'guest hdr len' and 'host hdr len' make better
> sense than 'guest hdr' and 'host hdr'
> 
> >              exit(1);
> 
> Can we drop the packet(s) instead of exiting?
> 
> 		Amit

This is a guest bug in the same way as moving indexes incorrectly.
But since we do have such buggy guests in the field (old virtio),
I guess we should.

-- 
MST