| Message ID | 20100623095118.GA9796@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
"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.
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.
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
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 */
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(-)