Xen devel - Aug 2010 - Re: [Bugme-new] [Bug 16529] New: xennet driver crashes when using with pseudowire aka l2tpv3

On 08/25/2010 03:31 PM, Andrew Morton wrote:
> (switched to email.  Please respond via emailed reply-to-all, not via the
> bugzilla web interface).
>
> On Fri, 6 Aug 2010 12:46:18 GMT
> bugzilla-daemon@bugzilla.kernel.org wrote:
>
>> https://bugzilla.kernel.org/show_bug.cgi?id=16529
>>
>>            Summary: xennet driver crashes when using with pseudowire
aka
>>                     l2tpv3
>>            Product: Networking
>>            Version: 2.5
>>     Kernel Version: 2.6.35
>>           Platform: All
>>         OS/Version: Linux
>>               Tree: Mainline
>>             Status: NEW
>>           Severity: normal
>>           Priority: P1
>>          Component: Other
>>         AssignedTo: acme@ghostprotocols.net
>>         ReportedBy: heil@terminal-consulting.de
>>         Regression: No
>>
>>
>> I tried to use use the new l2tpv3 implementation on two xen
domU''s but one of
>> them is crashing when the first l2tpv3 packet is received.
>>
>> As a great man mentioned: 
>> --
>> guessing that eth_type_trans() has tried to pull an ethernet header
from
>> the skb and has run off the end, which suggests an issue with the skb
>> that was passed up. Does the ethernet driver do proper alignment of
data
>> in its skbs? Perhaps it doesn''t allocate as much headroom as
other
>> drivers? Perhaps the L2TP code assumes things about the skb that
aren''t
>> valid..
>> --
>> Here is a link for the setup
>> http://www.pastebin.org/445975
>> and here a link with more details about the crash
http://pastebin.org/449221
Please attach these to the bug or something; pastebin is not working for
me at the moment.
>> According the the hints from James Chapmann we tried generic x86 with a
>> different nic driver like e1000 and this works without any problem. 
>>
>> Iam using OpenWrt to create the system images. To speed up the process
i would
>> prepare Xen images, make them available or whatever is wished because
>> pseudowire is already ready in OpenWrt Trunk so that every man also
with "price
>> sensitivy" hardware can use it.
>>
> Seems to be a problem with xennet afacit?
Possibly.  What''s a l2tpv3 and what does it do differently from other
protocols?

    J


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Wed, 2010-08-25 at 23:56 +0100, Jeremy Fitzhardinge wrote:
> >> Here is a link for the setup
> >> http://www.pastebin.org/445975
> >> and here a link with more details about the crash
http://pastebin.org/449221
> 
> Please attach these to the bug or something; pastebin is not working for
> me at the moment.
It''s really slow for me but got there eventually. I''ve
attached the
files.

It''s apparently hitting this BUG_ON in __skb_pull:
        
        static inline unsigned char *__skb_pull(struct sk_buff *skb, unsigned
int len)
        {
                skb->len -= len;
                BUG_ON(skb->len < skb->data_len);
                return skb->data += len;
        }

Ian.




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Le jeudi 26 août 2010 à 08:10 +0100, Ian Campbell a écrit
:
> On Wed, 2010-08-25 at 23:56 +0100, Jeremy Fitzhardinge wrote:
> 
> > >> Here is a link for the setup
> > >> http://www.pastebin.org/445975
> > >> and here a link with more details about the crash
http://pastebin.org/449221
> > 
> > Please attach these to the bug or something; pastebin is not working
for
> > me at the moment.
> 
> It''s really slow for me but got there eventually. I''ve
attached the
> files.
> 
> It''s apparently hitting this BUG_ON in __skb_pull:
>         
>         static inline unsigned char *__skb_pull(struct sk_buff *skb,
unsigned int len)
>         {
>                 skb->len -= len;
>                 BUG_ON(skb->len < skb->data_len);
>                 return skb->data += len;
>         }
> 
> Ian.
> 
Thanks a lot.

I''ll submit a patch to fix l2tp_eth_dev_recv() 

Its illegal to call dev_forward_skb() with a too small payload.




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Here is the patch, could you test it please ?

Thanks !

[PATCH] l2tp: test for malicious frames in l2tp_eth_dev_recv()

close https://bugzilla.kernel.org/show_bug.cgi?id=16529

Before calling dev_forward_skb(), we should make sure skb contains at
least an ethernet header, even if length included in upper layer said
so.

Reported-by: Thomas Heil <heil@terminal-consulting.de>
Reported-by: Ian Campbell <Ian.Campbell@eu.citrix.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/l2tp/l2tp_core.c |    2 +-
 net/l2tp/l2tp_eth.c  |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index 58c6c4c..0687c5c 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -132,7 +132,7 @@ static void l2tp_eth_dev_recv(struct l2tp_session *session,
struct sk_buff *skb,
 		printk("\n");
 	}
 
-	if (data_len < ETH_HLEN)
+	if (skb->len < ETH_HLEN)
 		goto error;
 
 	secpath_reset(skb);



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2010-08-26 at 09:03 +0100, Eric Dumazet wrote:
> Here is the patch, could you test it please ?
> 
> Thanks !
> 
> [PATCH] l2tp: test for malicious frames in l2tp_eth_dev_recv()
> 
> close https://bugzilla.kernel.org/show_bug.cgi?id=16529
> 
> Before calling dev_forward_skb(), we should make sure skb contains at
> least an ethernet header, even if length included in upper layer said
> so.
Does this imply that there is some problem with xen-netfront setting
skb->len or skb->data_len or something incorrectly? It''s not
clear where
data_len has come from in this context.

Ian.
> 
> Reported-by: Thomas Heil <heil@terminal-consulting.de>
> Reported-by: Ian Campbell <Ian.Campbell@eu.citrix.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  net/l2tp/l2tp_core.c |    2 +-
>  net/l2tp/l2tp_eth.c  |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
> index 58c6c4c..0687c5c 100644
> --- a/net/l2tp/l2tp_eth.c
> +++ b/net/l2tp/l2tp_eth.c
> @@ -132,7 +132,7 @@ static void l2tp_eth_dev_recv(struct l2tp_session
*session, struct sk_buff *skb,
>  		printk("\n");
>  	}
>  
> -	if (data_len < ETH_HLEN)
> +	if (skb->len < ETH_HLEN)
>  		goto error;
>  
>  	secpath_reset(skb);
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Le jeudi 26 août 2010 à 09:14 +0100, Ian Campbell a écrit
:
> On Thu, 2010-08-26 at 09:03 +0100, Eric Dumazet wrote:
> > Here is the patch, could you test it please ?
> > 
> > Thanks !
> > 
> > [PATCH] l2tp: test for malicious frames in l2tp_eth_dev_recv()
> > 
> > close https://bugzilla.kernel.org/show_bug.cgi?id=16529
> > 
> > Before calling dev_forward_skb(), we should make sure skb contains at
> > least an ethernet header, even if length included in upper layer said
> > so.
> 
> Does this imply that there is some problem with xen-netfront setting
> skb->len or skb->data_len or something incorrectly? It''s not
clear where
> data_len has come from in this context.
data_len is a 16bit field provided in a prior encapsulation header,
provided by user (untrusted source)

Some buggy or malicious software sent an invalid frame,


< encapsulation [len=1000] >  < ''runt'' eth frame
(len<14) >

Another fix would be to change l2tp_recv_dequeue_skb(), and check

L2TP_SKB_CB(skb)->length against skb->len, before calling 

(*session->recv_skb)(session, skb, length);


I prefer the one liner patch I sent you, as a minimum fix.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2010-08-26 at 09:34 +0100, Eric Dumazet wrote:
> Le jeudi 26 août 2010 à 09:14 +0100, Ian Campbell a écrit :
> > On Thu, 2010-08-26 at 09:03 +0100, Eric Dumazet wrote:
> > > Here is the patch, could you test it please ?
> > > 
> > > Thanks !
> > > 
> > > [PATCH] l2tp: test for malicious frames in l2tp_eth_dev_recv()
> > > 
> > > close https://bugzilla.kernel.org/show_bug.cgi?id=16529
> > > 
> > > Before calling dev_forward_skb(), we should make sure skb
contains at
> > > least an ethernet header, even if length included in upper layer
said
> > > so.
> > 
> > Does this imply that there is some problem with xen-netfront setting
> > skb->len or skb->data_len or something incorrectly?
It''s not clear where
> > data_len has come from in this context.
> 
> data_len is a 16bit field provided in a prior encapsulation header,
> provided by user (untrusted source)
> 
> Some buggy or malicious software sent an invalid frame,
> 
> 
> < encapsulation [len=1000] >  < ''runt'' eth frame
(len<14) >
> 
> Another fix would be to change l2tp_recv_dequeue_skb(), and check
> 
> L2TP_SKB_CB(skb)->length against skb->len, before calling 
> 
> (*session->recv_skb)(session, skb, length);
> 
> 
> I prefer the one liner patch I sent you, as a minimum fix.
Thanks, I just wanted to be sure we weren''t papering over a potential
issue in xen-netfront.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Le jeudi 26 août 2010 à 10:03 +0200, Eric Dumazet a écrit
:
> Here is the patch, could you test it please ?
> 
> Thanks !
> 
> [PATCH] l2tp: test for malicious frames in l2tp_eth_dev_recv()
> 
> close https://bugzilla.kernel.org/show_bug.cgi?id=16529
> 
> Before calling dev_forward_skb(), we should make sure skb contains at
> least an ethernet header, even if length included in upper layer said
> so.
> 
> Reported-by: Thomas Heil <heil@terminal-consulting.de>
> Reported-by: Ian Campbell <Ian.Campbell@eu.citrix.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  net/l2tp/l2tp_core.c |    2 +-
>  net/l2tp/l2tp_eth.c  |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
> index 58c6c4c..0687c5c 100644
> --- a/net/l2tp/l2tp_eth.c
> +++ b/net/l2tp/l2tp_eth.c
> @@ -132,7 +132,7 @@ static void l2tp_eth_dev_recv(struct l2tp_session
*session, struct sk_buff *skb,
>  		printk("\n");
>  	}
>  
> -	if (data_len < ETH_HLEN)
> +	if (skb->len < ETH_HLEN)
>  		goto error;
>  
>  	secpath_reset(skb);
> 
Hmm, reading this code again, I suspect a much better fix is to make
sure ''ethernet header'' is in skb head, not in a fragment.

Maybe frame is valid but only L2TP encapsulation in skb->header at this
point.

Thanks !

[PATCH] l2tp: test for ethernet header in l2tp_eth_dev_recv()

close https://bugzilla.kernel.org/show_bug.cgi?id=16529

Before calling dev_forward_skb(), we should make sure skb head contains
at least an ethernet header, even if length included in upper layer said
so. Use pskb_may_pull() to make sure this ethernet header is present in
skb head.

Reported-by: Thomas Heil <heil@terminal-consulting.de>
Reported-by: Ian Campbell <Ian.Campbell@eu.citrix.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/l2tp/l2tp_eth.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index 58c6c4c..1ae6976 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -132,7 +132,7 @@ static void l2tp_eth_dev_recv(struct l2tp_session *session,
struct sk_buff *skb,
 		printk("\n");
 	}
 
-	if (data_len < ETH_HLEN)
+	if (!pskb_may_pull(skb, sizeof(ETH_HLEN)))
 		goto error;
 
 	secpath_reset(skb);



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 26 Aug 2010 11:44:35 +0200
> [PATCH] l2tp: test for ethernet header in l2tp_eth_dev_recv()
> 
> close https://bugzilla.kernel.org/show_bug.cgi?id=16529
> 
> Before calling dev_forward_skb(), we should make sure skb head contains
> at least an ethernet header, even if length included in upper layer said
> so. Use pskb_may_pull() to make sure this ethernet header is present in
> skb head.
> 
> Reported-by: Thomas Heil <heil@terminal-consulting.de>
> Reported-by: Ian Campbell <Ian.Campbell@eu.citrix.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied, thanks Eric.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel