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

[PATCH 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 |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 0c7321c..64e0717 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -165,6 +165,9 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 	unsigned int copy, hdr_len, offset;
 	char *p;
 
+	if (len > MAX_SKB_FRAGS * PAGE_SIZE)
+		return NULL;
+
 	p = page_address(page);
 
 	/* copy small packet so we can reuse these pages for small data */
-- 
1.7.6.1

[PATCH 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 |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 64e0717..8d32c1e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -198,7 +198,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 	len -= copy;
 	offset += copy;
 
-	while (len) {
+	while (len && page) {
 		set_skb_frag(skb, page, offset, &len);
 		page = (struct page *)page->private;
 		offset = 0;
-- 
1.7.6.1

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

[Michael S. Tsirkin]

On Mon, Sep 26, 2011 at 08:41:08PM +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>

Interesting.  This is a theoretical issue, correct?
Not a crash you actually see.

This crash would mean device is giving us packets
that are way too large. Avoiding crashes even in the face of
a misbehaved device is a good idea, but should
we print a diagnostic to a system log?
Maybe rate-limited or print once to avoid filling
up the disk. Other places in driver print with pr_debug
I'm not sure that's right but better than nothing.

> ---
>  drivers/net/virtio_net.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 0c7321c..64e0717 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -165,6 +165,9 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>  	unsigned int copy, hdr_len, offset;
>  	char *p;
>  
> +	if (len > MAX_SKB_FRAGS * PAGE_SIZE)

unlikely()?

Also, this seems too aggressive: at this point len includes the header
and the linear part. The right place for this
test is probably where we fill in the frags, just before
while (len)

The whole can only happen when mergeable buffers
are disabled, right?


> +		return NULL;
> +
>  	p = page_address(page);
>  
>  	/* copy small packet so we can reuse these pages for small data */
> -- 
> 1.7.6.1

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

[Michael S. Tsirkin]

On Mon, Sep 26, 2011 at 08:41:09PM +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>

Hmm, another protection against a buggy device, right?
No problem with that, but let's discard the packet
and print a disgnostic, so that the user can discover
what happened.

> ---
>  drivers/net/virtio_net.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 64e0717..8d32c1e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -198,7 +198,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>  	len -= copy;
>  	offset += copy;
>  
> -	while (len) {
> +	while (len && page) {
>  		set_skb_frag(skb, page, offset, &len);
>  		page = (struct page *)page->private;
>  		offset = 0;
> -- 
> 1.7.6.1

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

[Sasha Levin]

On Mon, 2011-09-26 at 21:44 +0300, Michael S. Tsirkin wrote:
> On Mon, Sep 26, 2011 at 08:41:08PM +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>
> 
> Interesting.  This is a theoretical issue, correct?
> Not a crash you actually see.

Actually it was an actual crash caused when our virtio-net driver in kvm
tools did funny things and passed '(u32)-1' length as a buffer length to
the guest kernel.

> This crash would mean device is giving us packets
> that are way too large. Avoiding crashes even in the face of
> a misbehaved device is a good idea, but should
> we print a diagnostic to a system log?
> Maybe rate-limited or print once to avoid filling
> up the disk. Other places in driver print with pr_debug
> I'm not sure that's right but better than nothing.

Yup, I'll add some debug info.

> > ---
> >  drivers/net/virtio_net.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 0c7321c..64e0717 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -165,6 +165,9 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> >  	unsigned int copy, hdr_len, offset;
> >  	char *p;
> >  
> > +	if (len > MAX_SKB_FRAGS * PAGE_SIZE)
> 
> unlikely()?
> 
> Also, this seems too aggressive: at this point len includes the header
> and the linear part. The right place for this
> test is probably where we fill in the frags, just before
> while (len)
> 
> The whole can only happen when mergeable buffers
> are disabled, right?

>From what I understand it can happen whenever you're going to build a
skb longer than PAGE_SIZE.

-- 

Sasha.

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

[Pekka Enberg]

On Mon, Sep 26, 2011 at 10:37 PM, Sasha Levin <levinsasha928@gmail.com> wrote:
>> Interesting.  This is a theoretical issue, correct?
>> Not a crash you actually see.
>
> Actually it was an actual crash caused when our virtio-net driver in kvm
> tools did funny things and passed '(u32)-1' length as a buffer length to
> the guest kernel.

I'm not sure what Michael means with "theoretical issue" here. Can the guest
driver assume that the hypervisor doesn't attempt to do nasty things?

                          Pekka

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

[Michael S. Tsirkin]

On Mon, Sep 26, 2011 at 10:37:22PM +0300, Sasha Levin wrote:
> On Mon, 2011-09-26 at 21:44 +0300, Michael S. Tsirkin wrote:
> > On Mon, Sep 26, 2011 at 08:41:08PM +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>
> > 
> > Interesting.  This is a theoretical issue, correct?
> > Not a crash you actually see.
> 
> Actually it was an actual crash caused when our virtio-net driver in kvm
> tools did funny things and passed '(u32)-1' length as a buffer length to
> the guest kernel.
> 
> > This crash would mean device is giving us packets
> > that are way too large. Avoiding crashes even in the face of
> > a misbehaved device is a good idea, but should
> > we print a diagnostic to a system log?
> > Maybe rate-limited or print once to avoid filling
> > up the disk. Other places in driver print with pr_debug
> > I'm not sure that's right but better than nothing.
> 
> Yup, I'll add some debug info.
> 
> > > ---
> > >  drivers/net/virtio_net.c |    3 +++
> > >  1 files changed, 3 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 0c7321c..64e0717 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -165,6 +165,9 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> > >  	unsigned int copy, hdr_len, offset;
> > >  	char *p;
> > >  
> > > +	if (len > MAX_SKB_FRAGS * PAGE_SIZE)
> > 
> > unlikely()?
> > 
> > Also, this seems too aggressive: at this point len includes the header
> > and the linear part. The right place for this
> > test is probably where we fill in the frags, just before
> > while (len)
> > 
> > The whole can only happen when mergeable buffers
> > are disabled, right?
> 
> >From what I understand it can happen whenever you're going to build a
> skb longer than PAGE_SIZE.

Hmm how exactly?  With mergeable buffers this only gets
the length of the 1st chunk which is up to 4K unless the driver
is buggy ...

> -- 
> 
> Sasha.

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

[Michael S. Tsirkin]

On Mon, Sep 26, 2011 at 10:45:35PM +0300, Pekka Enberg wrote:
> On Mon, Sep 26, 2011 at 10:37 PM, Sasha Levin <levinsasha928@gmail.com> wrote:
> >> Interesting.  This is a theoretical issue, correct?
> >> Not a crash you actually see.
> >
> > Actually it was an actual crash caused when our virtio-net driver in kvm
> > tools did funny things and passed '(u32)-1' length as a buffer length to
> > the guest kernel.
> 
> I'm not sure what Michael means with "theoretical issue" here. Can the guest
> driver assume that the hypervisor doesn't attempt to do nasty things?
> 
>                           Pekka

IMO yes, hypervisor has full access to guest memory so it's a safe
assumption. But surviving in the face of hypervisor bugs
is laudable goal, bugs do happen.

-- 
MST

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

[Sasha Levin]

On Mon, 2011-09-26 at 22:45 +0300, Pekka Enberg wrote:
> On Mon, Sep 26, 2011 at 10:37 PM, Sasha Levin <levinsasha928@gmail.com> wrote:
> >> Interesting.  This is a theoretical issue, correct?
> >> Not a crash you actually see.
> >
> > Actually it was an actual crash caused when our virtio-net driver in kvm
> > tools did funny things and passed '(u32)-1' length as a buffer length to
> > the guest kernel.
> 
> I'm not sure what Michael means with "theoretical issue" here. Can the guest
> driver assume that the hypervisor doesn't attempt to do nasty things?

afaik if the hypervisor can access the vcpus and the memory of the
guest, this shouldn't be a security issue - more of a bug prevention
issue.

I guess it'll be interesting the other way around, when it's the guest
that passes this buggy information to the hypervisor.

-- 

Sasha.

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

[Pekka Enberg]

On Mon, Sep 26, 2011 at 10:45:35PM +0300, Pekka Enberg wrote:
>> I'm not sure what Michael means with "theoretical issue" here. Can the guest
>> driver assume that the hypervisor doesn't attempt to do nasty things?

On Mon, Sep 26, 2011 at 10:57 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> IMO yes, hypervisor has full access to guest memory so it's a safe
> assumption. But surviving in the face of hypervisor bugs
> is laudable goal, bugs do happen.

I was thinking of a compromised guest that is able to trick the hypervisor into
doing nasty things to other guests without taking over the hypervisor
completely. So for something like virtio networking, that's by
definition exposed
to rest of the world, I think it's very important not to be robust
against hypervisor
bugs.

In any case, we were able to trigger this particular case rather easily with our
buggy tool, so it's definitely worth fixing. ;-)

FWIW,

Acked-by: Pekka Enberg <penberg@kernel.org>

                                Pekka

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

[Sasha Levin]

On Mon, 2011-09-26 at 22:55 +0300, Michael S. Tsirkin wrote:
> On Mon, Sep 26, 2011 at 10:37:22PM +0300, Sasha Levin wrote:
> > On Mon, 2011-09-26 at 21:44 +0300, Michael S. Tsirkin wrote:
> > > On Mon, Sep 26, 2011 at 08:41:08PM +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>
> > > 
> > > Interesting.  This is a theoretical issue, correct?
> > > Not a crash you actually see.
> > 
> > Actually it was an actual crash caused when our virtio-net driver in kvm
> > tools did funny things and passed '(u32)-1' length as a buffer length to
> > the guest kernel.
> > 
> > > This crash would mean device is giving us packets
> > > that are way too large. Avoiding crashes even in the face of
> > > a misbehaved device is a good idea, but should
> > > we print a diagnostic to a system log?
> > > Maybe rate-limited or print once to avoid filling
> > > up the disk. Other places in driver print with pr_debug
> > > I'm not sure that's right but better than nothing.
> > 
> > Yup, I'll add some debug info.
> > 
> > > > ---
> > > >  drivers/net/virtio_net.c |    3 +++
> > > >  1 files changed, 3 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 0c7321c..64e0717 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -165,6 +165,9 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> > > >  	unsigned int copy, hdr_len, offset;
> > > >  	char *p;
> > > >  
> > > > +	if (len > MAX_SKB_FRAGS * PAGE_SIZE)
> > > 
> > > unlikely()?
> > > 
> > > Also, this seems too aggressive: at this point len includes the header
> > > and the linear part. The right place for this
> > > test is probably where we fill in the frags, just before
> > > while (len)
> > > 
> > > The whole can only happen when mergeable buffers
> > > are disabled, right?
> > 
> > >From what I understand it can happen whenever you're going to build a
> > skb longer than PAGE_SIZE.
> 
> Hmm how exactly?  With mergeable buffers this only gets
> the length of the 1st chunk which is up to 4K unless the driver
> is buggy ...

What about the case where TSO or ECN features are set? The code flow
suggests that mergeable would get ignored in that case:

	/* If we can receive ANY GSO packets, we must allocate large ones. */
	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6) ||
	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_ECN))
		vi->big_packets = true;

	[...]

	if (!vi->mergeable_rx_bufs && !vi->big_packets) {
		skb = buf;
		len -= sizeof(struct virtio_net_hdr);
		skb_trim(skb, len);
	} else {
		page = buf;
		skb = page_to_skb(vi, page, len);
		...


I haven't actually tested it with mergeable buffers enabled, but could
do it later today.
-- 

Sasha.

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

[Michael S. Tsirkin]

On Tue, Sep 27, 2011 at 09:44:02AM +0300, Sasha Levin wrote:
> On Mon, 2011-09-26 at 22:55 +0300, Michael S. Tsirkin wrote:
> > On Mon, Sep 26, 2011 at 10:37:22PM +0300, Sasha Levin wrote:
> > > On Mon, 2011-09-26 at 21:44 +0300, Michael S. Tsirkin wrote:
> > > > On Mon, Sep 26, 2011 at 08:41:08PM +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>
> > > > 
> > > > Interesting.  This is a theoretical issue, correct?
> > > > Not a crash you actually see.
> > > 
> > > Actually it was an actual crash caused when our virtio-net driver in kvm
> > > tools did funny things and passed '(u32)-1' length as a buffer length to
> > > the guest kernel.
> > > 
> > > > This crash would mean device is giving us packets
> > > > that are way too large. Avoiding crashes even in the face of
> > > > a misbehaved device is a good idea, but should
> > > > we print a diagnostic to a system log?
> > > > Maybe rate-limited or print once to avoid filling
> > > > up the disk. Other places in driver print with pr_debug
> > > > I'm not sure that's right but better than nothing.
> > > 
> > > Yup, I'll add some debug info.
> > > 
> > > > > ---
> > > > >  drivers/net/virtio_net.c |    3 +++
> > > > >  1 files changed, 3 insertions(+), 0 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > index 0c7321c..64e0717 100644
> > > > > --- a/drivers/net/virtio_net.c
> > > > > +++ b/drivers/net/virtio_net.c
> > > > > @@ -165,6 +165,9 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> > > > >  	unsigned int copy, hdr_len, offset;
> > > > >  	char *p;
> > > > >  
> > > > > +	if (len > MAX_SKB_FRAGS * PAGE_SIZE)
> > > > 
> > > > unlikely()?
> > > > 
> > > > Also, this seems too aggressive: at this point len includes the header
> > > > and the linear part. The right place for this
> > > > test is probably where we fill in the frags, just before
> > > > while (len)
> > > > 
> > > > The whole can only happen when mergeable buffers
> > > > are disabled, right?
> > > 
> > > >From what I understand it can happen whenever you're going to build a
> > > skb longer than PAGE_SIZE.
> > 
> > Hmm how exactly?  With mergeable buffers this only gets
> > the length of the 1st chunk which is up to 4K unless the driver
> > is buggy ...
> 
> What about the case where TSO or ECN features are set? The code flow
> suggests that mergeable would get ignored in that case:
> 
> 	/* If we can receive ANY GSO packets, we must allocate large ones. */
> 	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> 	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6) ||
> 	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_ECN))
> 		vi->big_packets = true;
> 
> 	[...]
> 
> 	if (!vi->mergeable_rx_bufs && !vi->big_packets) {
> 		skb = buf;
> 		len -= sizeof(struct virtio_net_hdr);
> 		skb_trim(skb, len);
> 	} else {
> 		page = buf;
> 		skb = page_to_skb(vi, page, len);
> 		...

Sorry I don't get it yet. Where is mergeable ignored here?

> I haven't actually tested it with mergeable buffers enabled, but could
> do it later today.

Please do.

> -- 
> 
> Sasha.

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

[Sasha Levin]

On Tue, 2011-09-27 at 10:00 +0300, Michael S. Tsirkin wrote:
> >               skb = page_to_skb(vi, page, len);
> >               ...
> 
> Sorry I don't get it yet. Where is mergeable ignored here?

The NULL deref happens in page_to_skb(), before merge buffers are
handled.

I'll test it and see if it's really the case.

-- 

Sasha.

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

[Michael S. Tsirkin]

On Tue, Sep 27, 2011 at 02:20:29PM +0300, Sasha Levin wrote:
> On Tue, 2011-09-27 at 10:00 +0300, Michael S. Tsirkin wrote:
> > >               skb = page_to_skb(vi, page, len);
> > >               ...
> > 
> > Sorry I don't get it yet. Where is mergeable ignored here?
> 
> The NULL deref happens in page_to_skb(), before merge buffers are
> handled.

The len here is a single buffer length, so for mergeable
buffers it is <= the size of the buffer we gave to hardware,
which is PAGE_SIZE.

        err = virtqueue_add_buf_single(vi->rvq, page_address(page),
                                       PAGE_SIZE, false, page, gfp);
 

Unless of course we are trying to work around broken hardware again,
which I don't have a problem with, but should probably
get appropriate comments in code and trigger a warning.

> I'll test it and see if it's really the case.
> 
> -- 
> 
> Sasha.

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

[Sasha Levin]

On Tue, 2011-09-27 at 15:37 +0300, Michael S. Tsirkin wrote:
> On Tue, Sep 27, 2011 at 02:20:29PM +0300, Sasha Levin wrote:
> > On Tue, 2011-09-27 at 10:00 +0300, Michael S. Tsirkin wrote:
> > > >               skb = page_to_skb(vi, page, len);
> > > >               ...
> > > 
> > > Sorry I don't get it yet. Where is mergeable ignored here?
> > 
> > The NULL deref happens in page_to_skb(), before merge buffers are
> > handled.
> 
> The len here is a single buffer length, so for mergeable
> buffers it is <= the size of the buffer we gave to hardware,
> which is PAGE_SIZE.
> 
>         err = virtqueue_add_buf_single(vi->rvq, page_address(page),
>                                        PAGE_SIZE, false, page, gfp);
>  
> 
> Unless of course we are trying to work around broken hardware again,
> which I don't have a problem with, but should probably
> get appropriate comments in code and trigger a warning.
> 
> > I'll test it and see if it's really the case.

I've verified it with VIRTIO_NET_F_MRG_RXBUF set, and we still get the
NULL deref.



-- 

Sasha.