[PATCH] bridge: Reset IPCB on forward non-local packets in br_handle_frame_finish()

[PATCH] bridge: Reset IPCB on forward non-local packets in br_handle_frame_finish()

[Vasily Averin]

if dst is not local br_handle_frame_finish() does not clone original skb and
forgets to reset IPCB before return to IP stack. it can lead to stack corruption
in icmp_send()

Signed-off-by: Vasily Averin <vvs@sw.ru>
---
 net/bridge/br_input.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index f06ee39..6be8d00 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -93,10 +93,11 @@ int br_handle_frame_finish(struct sk_buff *skb)
 			skb2 = skb;

 		br->dev->stats.multicast++;
-	} else if ((dst = __br_fdb_get(br, dest)) && dst->is_local) {
+	} else if ((dst = __br_fdb_get(br, dest)) != NULL) {
 		skb2 = skb;
 		/* Do not forward the packet since it's local. */
-		skb = NULL;
+		if (dst->is_local) {
+			skb = NULL;
 	}

 	if (skb) {
-- 1.7.5.4

Re: [PATCH] bridge: Reset IPCB on forward non-local packets in br_handle_frame_finish()

[Vasily Averin]

On 11/02/2011 11:08 PM, Vasily Averin wrote:
> if dst is not local br_handle_frame_finish() does not clone original skb and
> forgets to reset IPCB before return to IP stack. it can lead to stack corruption
> in icmp_send()

example of stack corruption
http://bugzilla.openvz.org/show_bug.cgi?id=2016

> Signed-off-by: Vasily Averin <vvs@sw.ru>
> ---
>  net/bridge/br_input.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index f06ee39..6be8d00 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -93,10 +93,11 @@ int br_handle_frame_finish(struct sk_buff *skb)
>  			skb2 = skb;
> 
>  		br->dev->stats.multicast++;
> -	} else if ((dst = __br_fdb_get(br, dest)) && dst->is_local) {
> +	} else if ((dst = __br_fdb_get(br, dest)) != NULL) {
>  		skb2 = skb;
>  		/* Do not forward the packet since it's local. */
> -		skb = NULL;
> +		if (dst->is_local) {
> +			skb = NULL;
>  	}
> 
>  	if (skb) {
> -- 1.7.5.4

Re: [PATCH] bridge: Reset IPCB on forward non-local packets in br_handle_frame_finish()

[Stephen Hemminger]

On Wed, 02 Nov 2011 23:08:57 +0400
Vasily Averin <vvs@parallels.com> wrote:

> if dst is not local br_handle_frame_finish() does not clone original skb and
> forgets to reset IPCB before return to IP stack. it can lead to stack corruption
> in icmp_send()
> 
> Signed-off-by: Vasily Averin <vvs@sw.ru>
> ---
>  net/bridge/br_input.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index f06ee39..6be8d00 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -93,10 +93,11 @@ int br_handle_frame_finish(struct sk_buff *skb)
>  			skb2 = skb;
> 
>  		br->dev->stats.multicast++;
> -	} else if ((dst = __br_fdb_get(br, dest)) && dst->is_local) {
> +	} else if ((dst = __br_fdb_get(br, dest)) != NULL) {
>  		skb2 = skb;
>  		/* Do not forward the packet since it's local. */
> -		skb = NULL;
> +		if (dst->is_local) {
> +			skb = NULL;
>  	}
> 
>  	if (skb) {

What kernel version are you using? There were several previous fixes
in br_netfilter to deal with this type of issue over the last year.

Re: [PATCH] bridge: Reset IPCB on forward non-local packets in br_handle_frame_finish()

[Daniel Robbins]

On Wed, Nov 2, 2011 at 1:31 PM, Stephen Hemminger <shemminger@vyatta.com> wrote:
>
> What kernel version are you using? There were several previous fixes
> in br_netfilter to deal with this type of issue over the last year.

It's a kernel from this obscure company called Red Hat, and they
include it in this product called Red Hat Enterprise Linux 6.

Specifically, 042stab37.1 :)

-Daniel

Re: [PATCH] bridge: Reset IPCB on forward non-local packets in br_handle_frame_finish()

[Vasily Averin]

On 11/02/2011 11:31 PM, Stephen Hemminger wrote:
> On Wed, 02 Nov 2011 23:08:57 +0400
> Vasily Averin <vvs@parallels.com> wrote:
> 
>> if dst is not local br_handle_frame_finish() does not clone original skb and
>> forgets to reset IPCB before return to IP stack. it can lead to stack corruption
>> in icmp_send()

> What kernel version are you using? There were several previous fixes
> in br_netfilter to deal with this type of issue over the last year.

Originally it was noticed on RHEL6-based kernel

You are right, in mainline this issue was fixed in br_nf_forward_ip() long time ago.

thank you,
	Vasily Averin

Re: [PATCH] bridge: Reset IPCB on forward non-local packets in br_handle_frame_finish()

[David Miller]

From: Vasily Averin <vvs@parallels.com>
Date: Wed, 02 Nov 2011 23:08:57 +0400

> if dst is not local br_handle_frame_finish() does not clone original skb and
> forgets to reset IPCB before return to IP stack. it can lead to stack corruption
> in icmp_send()
> 
> Signed-off-by: Vasily Averin <vvs@sw.ru>

Nothing is worse than posting a patch that doesn't even compile.

And I really mean _nothing_.

Re: [Bridge] [PATCH] bridge: Reset IPCB on forward non-local packets in br_handle_frame_finish()

[Stephen Hemminger]

On Wed, 2 Nov 2011 13:45:29 -0600
Daniel Robbins <drobbins@funtoo.org> wrote:

> On Wed, Nov 2, 2011 at 1:31 PM, Stephen Hemminger <shemminger@vyatta.com> wrote:
> >
> > What kernel version are you using? There were several previous fixes
> > in br_netfilter to deal with this type of issue over the last year.
> 
> It's a kernel from this obscure company called Red Hat, and they
> include it in this product called Red Hat Enterprise Linux 6.
> 
> Specifically, 042stab37.1 :)
> 
> -Daniel

The pay for their tech support and have them fix it.
BTW - the problem was fixed upstream a long time ago, the upstream
kernel moves fast and RHEL probably froze many months before their
release.

Re: [PATCH] bridge: Reset IPCB on forward non-local packets in br_handle_frame_finish()

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 2 Nov 2011 13:14:45 -0700

> On Wed, 2 Nov 2011 13:45:29 -0600
> Daniel Robbins <drobbins@funtoo.org> wrote:
> 
>> On Wed, Nov 2, 2011 at 1:31 PM, Stephen Hemminger <shemminger@vyatta.com> wrote:
>> >
>> > What kernel version are you using? There were several previous fixes
>> > in br_netfilter to deal with this type of issue over the last year.
>> 
>> It's a kernel from this obscure company called Red Hat, and they
>> include it in this product called Red Hat Enterprise Linux 6.
>> 
>> Specifically, 042stab37.1 :)
>> 
>> -Daniel
> 
> The pay for their tech support and have them fix it.
> BTW - the problem was fixed upstream a long time ago, the upstream
> kernel moves fast and RHEL probably froze many months before their
> release.

So the original patch not only wasn't build tested, the submitter also
didn't even check if upstream even _had_ the bug to begin with.

Re: [PATCH] bridge: Reset IPCB on forward non-local packets in br_handle_frame_finish()

[Daniel Robbins]

On Wed, Nov 2, 2011 at 2:14 PM, Stephen Hemminger <shemminger@vyatta.com> wrote:
>
> The pay for their tech support and have them fix it.
> BTW - the problem was fixed upstream a long time ago, the upstream
> kernel moves fast and RHEL probably froze many months before their
> release.

OK, thanks for the info. I moved my server to Linux ~3.1.0 and will be
tracking mainline instead of using RHEL from this point forward. I am
sick of dealing with the stale networking code in RHEL kernels.

Thanks and Regards,

Daniel