[PATCH v2 1/2] virtio-net: Verify page list size before fitting into skb

This patch verifies that the length of a buffer stored in a linked list
of pages is small enough to fit into a skb.

If the size is larger than a max size of a skb, it means that we shouldn't
go ahead building skbs anyway since we won't be able to send the buffer as
the user requested.

Cc: Rusty Russell <redacted>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Sasha Levin <redacted>
---
 drivers/net/virtio_net.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 0c7321c..bde0dec 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -195,6 +195,19 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 	len -= copy;
 	offset += copy;
 
+	/*
+	 * Verify that we can indeed put this data into a skb.
+	 * This is here to handle cases when the device erroneously
+	 * tries to receive more than is possible. This is usually
+	 * the case of a broken device.
+	 */
+	if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) {
+		if (net_ratelimit())
+			pr_debug("%s: too much data\n", skb->dev->name);
+		dev_kfree_skb(skb);
+		return NULL;
+	}
+
 	while (len) {
 		set_skb_frag(skb, page, offset, &len);
 		page = (struct page *)page->private;

-- 
1.7.6.1

This patch prevents a NULL dereference when the user has passed a length
longer than an actual buffer to virtio-net.

Cc: Rusty Russell <redacted>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Sasha Levin <redacted>
---
 drivers/net/virtio_net.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index bde0dec..4a53d2a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -208,12 +208,22 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 		return NULL;
 	}
 
-	while (len) {
+	while (len && page) {
 		set_skb_frag(skb, page, offset, &len);
 		page = (struct page *)page->private;
 		offset = 0;
 	}
 
+	/*
+	 * This is the case where we ran out of pages in our linked list, but
+	 * supposedly have more data to read.
+	 */	
+	if (len > 0) {
+		pr_debug("%s: missing data to assemble skb\n", skb->dev->name);
+		dev_kfree_skb(skb);
+		return NULL;
+	}
+
 	if (page)
 		give_pages(vi, page);
 

-- 
1.7.6.1

From: Sasha Levin <redacted>
Date: Wed, 28 Sep 2011 17:40:54 +0300

This patch verifies that the length of a buffer stored in a linked list
of pages is small enough to fit into a skb.

If the size is larger than a max size of a skb, it means that we shouldn't
go ahead building skbs anyway since we won't be able to send the buffer as
the user requested.

Cc: Rusty Russell <redacted>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Sasha Levin <redacted>

Michael, tell me what's happening with these two virtio-net bug fixes.

Thanks.

On Wed, Sep 28, 2011 at 05:40:55PM +0300, Sasha Levin wrote:

Again, let's clarify that this only happens with broken devices.

+	 */	
+	if (len > 0) {
+		pr_debug("%s: missing data to assemble skb\n", skb->dev->name);
+		dev_kfree_skb(skb);
+		return NULL;
+	}
+
 	if (page)
 		give_pages(vi, page);
 
-- 
1.7.6.1

On Mon, Oct 03, 2011 at 02:19:51PM -0400, David Miller wrote:

From: Sasha Levin <redacted>
Date: Wed, 28 Sep 2011 17:40:54 +0300

quoted

This patch verifies that the length of a buffer stored in a linked list
of pages is small enough to fit into a skb.

If the size is larger than a max size of a skb, it means that we shouldn't
go ahead building skbs anyway since we won't be able to send the buffer as
the user requested.

Cc: Rusty Russell <redacted>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Sasha Levin <redacted>

Michael, tell me what's happening with these two virtio-net bug fixes.

Thanks.

Thanks for the reminder.
They are more enhancements than bugfixes, in that both help debug buggy
hypervisors. So net-next material.
Patch 1 is ok patch 2 would benefit from a slightly clearer comment.

-- 
MST

On Wed, Sep 28, 2011 at 05:40:54PM +0300, Sasha Levin wrote:

BTW, receive_mergeable does
                        pr_debug("%s: packet too long\n", skb->dev->name);
                        skb->dev->stats.rx_length_errors++;

which makes sense.

 	while (len) {
 		set_skb_frag(skb, page, offset, &len);
 		page = (struct page *)page->private;
-- 
1.7.6.1

On Wed, Sep 28, 2011 at 05:40:55PM +0300, Sasha Levin wrote:

I think if (unlikely(!page))
		goto err_page;

would make the logic clearer. But see below

I thought about this some more. If length was too large host is
right now writing into pages that we have freed.
That is very bad, and I don't know what do do with it,
really not worth prettifying that IMO, NULL pointer is the least of our
worries.

But, with mergeable buffers at least, and that is the main mode anyway,
there is always a single page per buf, right?
So I think we should change the code,
and for mergeable buffers we shall only verify that
length <= PAGE_SIZE.

IOW copy a bit of code from page_to_skb(vi, page, len)
to receive_mergeable, maybe add a common function
if we need to avoid duplication.

  

-- 
1.7.6.1

On Mon, 2011-10-03 at 20:40 +0200, Michael S. Tsirkin wrote:

On Wed, Sep 28, 2011 at 05:40:55PM +0300, Sasha Levin wrote:

quoted

This patch prevents a NULL dereference when the user has passed a length
longer than an actual buffer to virtio-net.

Cc: Rusty Russell <redacted>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Sasha Levin <redacted>
---
 drivers/net/virtio_net.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index bde0dec..4a53d2a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -208,12 +208,22 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 		return NULL;
 	}
 
-	while (len) {
+	while (len && page) {
 		set_skb_frag(skb, page, offset, &len);
 		page = (struct page *)page->private;
 		offset = 0;
 	}
 
+	/*
+	 * This is the case where we ran out of pages in our linked list, but
+	 * supposedly have more data to read.

Again, let's clarify that this only happens with broken devices.

I think that the code within the if() makes it clear that it isn't the
regular path.

-- 

Sasha.

On Mon, 2011-10-03 at 21:04 +0200, Michael S. Tsirkin wrote:

On Wed, Sep 28, 2011 at 05:40:54PM +0300, Sasha Levin wrote:

quoted

This patch verifies that the length of a buffer stored in a linked list
of pages is small enough to fit into a skb.

If the size is larger than a max size of a skb, it means that we shouldn't
go ahead building skbs anyway since we won't be able to send the buffer as
the user requested.

Cc: Rusty Russell <redacted>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Sasha Levin <redacted>
---
 drivers/net/virtio_net.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 0c7321c..bde0dec 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -195,6 +195,19 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 	len -= copy;
 	offset += copy;
 
+	/*
+	 * Verify that we can indeed put this data into a skb.
+	 * This is here to handle cases when the device erroneously
+	 * tries to receive more than is possible. This is usually
+	 * the case of a broken device.
+	 */
+	if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) {
+		if (net_ratelimit())
+			pr_debug("%s: too much data\n", skb->dev->name);
+		dev_kfree_skb(skb);
+		return NULL;
+	}
+

BTW, receive_mergeable does
                        pr_debug("%s: packet too long\n", skb->dev->name);
                        skb->dev->stats.rx_length_errors++;

which makes sense.

Do you think we should increase rx_length_errors here as well?

-- 

Sasha.

On Wed, Oct 05, 2011 at 03:50:54PM +0200, Sasha Levin wrote:

On Mon, 2011-10-03 at 21:04 +0200, Michael S. Tsirkin wrote:

quoted

On Wed, Sep 28, 2011 at 05:40:54PM +0300, Sasha Levin wrote:

quoted

This patch verifies that the length of a buffer stored in a linked list
of pages is small enough to fit into a skb.

If the size is larger than a max size of a skb, it means that we shouldn't
go ahead building skbs anyway since we won't be able to send the buffer as
the user requested.

Cc: Rusty Russell <redacted>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Sasha Levin <redacted>
---
 drivers/net/virtio_net.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 0c7321c..bde0dec 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -195,6 +195,19 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 	len -= copy;
 	offset += copy;
 
+	/*
+	 * Verify that we can indeed put this data into a skb.
+	 * This is here to handle cases when the device erroneously
+	 * tries to receive more than is possible. This is usually
+	 * the case of a broken device.
+	 */
+	if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) {
+		if (net_ratelimit())
+			pr_debug("%s: too much data\n", skb->dev->name);
+		dev_kfree_skb(skb);
+		return NULL;
+	}
+

BTW, receive_mergeable does
                        pr_debug("%s: packet too long\n", skb->dev->name);
                        skb->dev->stats.rx_length_errors++;

which makes sense.

Do you think we should increase rx_length_errors here as well?

this is all debugging tool for devices/drivers, right?
so maybe not worth the noise.

-- 

Sasha.

On Wed, Oct 05, 2011 at 03:50:14PM +0200, Sasha Levin wrote:

On Mon, 2011-10-03 at 20:40 +0200, Michael S. Tsirkin wrote:

quoted

On Wed, Sep 28, 2011 at 05:40:55PM +0300, Sasha Levin wrote:

quoted

This patch prevents a NULL dereference when the user has passed a length
longer than an actual buffer to virtio-net.

Cc: Rusty Russell <redacted>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Sasha Levin <redacted>
---
 drivers/net/virtio_net.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index bde0dec..4a53d2a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -208,12 +208,22 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 		return NULL;
 	}
 
-	while (len) {
+	while (len && page) {
 		set_skb_frag(skb, page, offset, &len);
 		page = (struct page *)page->private;
 		offset = 0;
 	}
 
+	/*
+	 * This is the case where we ran out of pages in our linked list, but
+	 * supposedly have more data to read.

Again, let's clarify that this only happens with broken devices.

I think that the code within the if() makes it clear that it isn't the
regular path.

It doesn't make it clear that this never happens in absence of bugs.

-- 

Sasha.

From: Sasha Levin <redacted>
Date: Wed, 28 Sep 2011 17:40:54 +0300

This patch verifies that the length of a buffer stored in a linked list
of pages is small enough to fit into a skb.

If the size is larger than a max size of a skb, it means that we shouldn't
go ahead building skbs anyway since we won't be able to send the buffer as
the user requested.

Cc: Rusty Russell <redacted>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Sasha Levin <redacted>

Applied to net-next

From: Sasha Levin <redacted>
Date: Wed, 28 Sep 2011 17:40:55 +0300

This patch prevents a NULL dereference when the user has passed a length
longer than an actual buffer to virtio-net.

Cc: Rusty Russell <redacted>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Sasha Levin <redacted>

Waiting for a respin of this patch with clarified comments.

On Mon, 2011-10-03 at 21:05 +0200, Michael S. Tsirkin wrote:

I thought about this some more. If length was too large host is
right now writing into pages that we have freed.
That is very bad, and I don't know what do do with it,
really not worth prettifying that IMO, NULL pointer is the least of our
worries.

But, with mergeable buffers at least, and that is the main mode anyway,
there is always a single page per buf, right?
So I think we should change the code,
and for mergeable buffers we shall only verify that
length <= PAGE_SIZE.

In receive_mergeable() we already verify both 'page' being non-null and
length:

	if (len > PAGE_SIZE)
		len = PAGE_SIZE;

IOW copy a bit of code from page_to_skb(vi, page, len)
to receive_mergeable, maybe add a common function
if we need to avoid duplication.

-- 

Sasha.