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

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

[Sasha Levin]

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 <rusty@rustcorp.com.au>
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 <levinsasha928@gmail.com>
---
 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

[PATCH v2 2/2] virtio-net: Prevent NULL dereference

[Sasha Levin]

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

Cc: Rusty Russell <rusty@rustcorp.com.au>
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 <levinsasha928@gmail.com>
---
 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

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

[David Miller]

From: Sasha Levin <levinsasha928@gmail.com>
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 <rusty@rustcorp.com.au>
> 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 <levinsasha928@gmail.com>

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

Thanks.

Re: [PATCH v2 2/2] virtio-net: Prevent NULL dereference

[Michael S. Tsirkin]

On Wed, Sep 28, 2011 at 05:40:55PM +0300, Sasha Levin wrote:
> This patch prevents a NULL dereference when the user has passed a length
> longer than an actual buffer to virtio-net.
> 
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> 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 <levinsasha928@gmail.com>
> ---
>  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.

> +	 */	
> +	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

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

[Michael S. Tsirkin]

On Mon, Oct 03, 2011 at 02:19:51PM -0400, David Miller wrote:
> From: Sasha Levin <levinsasha928@gmail.com>
> 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 <rusty@rustcorp.com.au>
> > 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 <levinsasha928@gmail.com>
> 
> 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

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

[Michael S. Tsirkin]

On Wed, Sep 28, 2011 at 05:40:54PM +0300, Sasha Levin wrote:
> 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 <rusty@rustcorp.com.au>
> 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 <levinsasha928@gmail.com>
> ---
>  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.

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

Re: [PATCH v2 2/2] virtio-net: Prevent NULL dereference

[Michael S. Tsirkin]

On Wed, Sep 28, 2011 at 05:40:55PM +0300, Sasha Levin wrote:
> This patch prevents a NULL dereference when the user has passed a length
> longer than an actual buffer to virtio-net.
> 
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> 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 <levinsasha928@gmail.com>
> ---
>  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) {

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

would make the logic clearer. But see below

>  		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);
>

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

Re: [PATCH v2 2/2] virtio-net: Prevent NULL dereference

[Sasha Levin]

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:
> > This patch prevents a NULL dereference when the user has passed a length
> > longer than an actual buffer to virtio-net.
> > 
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > 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 <levinsasha928@gmail.com>
> > ---
> >  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.

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

[Sasha Levin]

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:
> > 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 <rusty@rustcorp.com.au>
> > 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 <levinsasha928@gmail.com>
> > ---
> >  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.

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

[Michael S. Tsirkin]

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:
> > On Wed, Sep 28, 2011 at 05:40:54PM +0300, Sasha Levin wrote:
> > > 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 <rusty@rustcorp.com.au>
> > > 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 <levinsasha928@gmail.com>
> > > ---
> > >  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.

Re: [PATCH v2 2/2] virtio-net: Prevent NULL dereference

[Michael S. Tsirkin]

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:
> > On Wed, Sep 28, 2011 at 05:40:55PM +0300, Sasha Levin wrote:
> > > This patch prevents a NULL dereference when the user has passed a length
> > > longer than an actual buffer to virtio-net.
> > > 
> > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > 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 <levinsasha928@gmail.com>
> > > ---
> > >  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.

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

[David Miller]

From: Sasha Levin <levinsasha928@gmail.com>
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 <rusty@rustcorp.com.au>
> 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 <levinsasha928@gmail.com>

Applied to net-next

Re: [PATCH v2 2/2] virtio-net: Prevent NULL dereference

[David Miller]

From: Sasha Levin <levinsasha928@gmail.com>
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 <rusty@rustcorp.com.au>
> 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 <levinsasha928@gmail.com>

Waiting for a respin of this patch with clarified comments.

Re: [PATCH v2 2/2] virtio-net: Prevent NULL dereference

[Sasha Levin]

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.