[PATCH 0/3] Action API fixes

[PATCH 0/3] Action API fixes

[Thomas Graf]

Dave,

Fixes for some rather serious action API bugs. Please apply.

 net/sched/act_api.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

[PATCH 1/3] [PKT_SCHED]: Fix illegal memory dereferences when dumping actions

[Thomas Graf]

[-- Attachment #1: act_fix_dump_null_deref --]
[-- Type: text/plain, Size: 1707 bytes --]

The TCA_ACT_KIND attribute is used without checking its
availability when dumping actions therefore leading to a
value of 0x4 being dereferenced.

The use of strcmp() in tc_lookup_action_n() isn't safe
when fed with string from an attribute without enforcing
proper NUL termination.

Both bugs can be triggered with malformed netlink message
and don't require any privileges.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Index: net-2.6.git/net/sched/act_api.c
===================================================================
--- net-2.6.git.orig/net/sched/act_api.c
+++ net-2.6.git/net/sched/act_api.c
@@ -776,7 +776,7 @@ replay:
 	return ret;
 }
 
-static char *
+static struct rtattr *
 find_dump_kind(struct nlmsghdr *n)
 {
 	struct rtattr *tb1, *tb2[TCA_ACT_MAX+1];
@@ -804,7 +804,7 @@ find_dump_kind(struct nlmsghdr *n)
 		return NULL;
 	kind = tb2[TCA_ACT_KIND-1];
 
-	return (char *) RTA_DATA(kind);
+	return kind;
 }
 
 static int
@@ -817,16 +817,15 @@ tc_dump_action(struct sk_buff *skb, stru
 	struct tc_action a;
 	int ret = 0;
 	struct tcamsg *t = (struct tcamsg *) NLMSG_DATA(cb->nlh);
-	char *kind = find_dump_kind(cb->nlh);
+	struct rtattr *kind = find_dump_kind(cb->nlh);
 
 	if (kind == NULL) {
 		printk("tc_dump_action: action bad kind\n");
 		return 0;
 	}
 
-	a_o = tc_lookup_action_n(kind);
+	a_o = tc_lookup_action(kind);
 	if (a_o == NULL) {
-		printk("failed to find %s\n", kind);
 		return 0;
 	}
 
@@ -834,7 +833,7 @@ tc_dump_action(struct sk_buff *skb, stru
 	a.ops = a_o;
 
 	if (a_o->walk == NULL) {
-		printk("tc_dump_action: %s !capable of dumping table\n", kind);
+		printk("tc_dump_action: %s !capable of dumping table\n", a_o->kind);
 		goto rtattr_failure;
 	}
 

--

Re: [PATCH 1/3] [PKT_SCHED]: Fix illegal memory dereferences when dumping actions

[Patrick McHardy]

Thomas Graf wrote:
> The TCA_ACT_KIND attribute is used without checking its
> availability when dumping actions therefore leading to a
> value of 0x4 being dereferenced.
> 
> The use of strcmp() in tc_lookup_action_n() isn't safe
> when fed with string from an attribute without enforcing
> proper NUL termination.
> 
> Both bugs can be triggered with malformed netlink message
> and don't require any privileges.
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> 
> Index: net-2.6.git/net/sched/act_api.c
> ===================================================================
> --- net-2.6.git.orig/net/sched/act_api.c
> +++ net-2.6.git/net/sched/act_api.c
> @@ -776,7 +776,7 @@ replay:
>  	return ret;
>  }
>  
> -static char *
> +static struct rtattr *
>  find_dump_kind(struct nlmsghdr *n)
>  {
>  	struct rtattr *tb1, *tb2[TCA_ACT_MAX+1];
> @@ -804,7 +804,7 @@ find_dump_kind(struct nlmsghdr *n)
>  		return NULL;
>  	kind = tb2[TCA_ACT_KIND-1];
>  
> -	return (char *) RTA_DATA(kind);
> +	return kind;
>  }
>  
>  static int
> @@ -817,16 +817,15 @@ tc_dump_action(struct sk_buff *skb, stru
>  	struct tc_action a;
>  	int ret = 0;
>  	struct tcamsg *t = (struct tcamsg *) NLMSG_DATA(cb->nlh);
> -	char *kind = find_dump_kind(cb->nlh);
> +	struct rtattr *kind = find_dump_kind(cb->nlh);
>  
>  	if (kind == NULL) {
>  		printk("tc_dump_action: action bad kind\n");
>  		return 0;
>  	}
>  
> -	a_o = tc_lookup_action_n(kind);
> +	a_o = tc_lookup_action(kind);
>  	if (a_o == NULL) {
> -		printk("failed to find %s\n", kind);
>  		return 0;
>  	}
>  
> @@ -834,7 +833,7 @@ tc_dump_action(struct sk_buff *skb, stru
>  	a.ops = a_o;
>  
>  	if (a_o->walk == NULL) {
> -		printk("tc_dump_action: %s !capable of dumping table\n", kind);
> +		printk("tc_dump_action: %s !capable of dumping table\n", a_o->kind);
>  		goto rtattr_failure;
>  	}

Can't we just get rid of these printks? This seems like a good
opportunity.

Re: [PATCH 1/3] [PKT_SCHED]: Fix illegal memory dereferences when dumping actions

[jamal]

On Wed, 2006-05-07 at 01:42 +0200, Patrick McHardy wrote:
> Thomas Graf wrote:
> >  	if (a_o->walk == NULL) {
> > -		printk("tc_dump_action: %s !capable of dumping table\n", kind);
> > +		printk("tc_dump_action: %s !capable of dumping table\n", a_o->kind);
> >  		goto rtattr_failure;
> >  	}
> 
> Can't we just get rid of these printks? This seems like a good
> opportunity.
> 

perhaps convert to DPRINTKs instead

cheers,
jamal

Re: [PATCH 1/3] [PKT_SCHED]: Fix illegal memory dereferences when dumping actions

[Thomas Graf]

* Patrick McHardy <kaber@trash.net> 2006-07-05 01:42
> Thomas Graf wrote:
> > @@ -834,7 +833,7 @@ tc_dump_action(struct sk_buff *skb, stru
> >  	a.ops = a_o;
> >  
> >  	if (a_o->walk == NULL) {
> > -		printk("tc_dump_action: %s !capable of dumping table\n", kind);
> > +		printk("tc_dump_action: %s !capable of dumping table\n", a_o->kind);
> >  		goto rtattr_failure;
> >  	}
> 
> Can't we just get rid of these printks? This seems like a good
> opportunity.

I agree, since this patch is definitely a stable candidate I
wanted it to stay as small as possible.

Re: [PATCH 1/3] [PKT_SCHED]: Fix illegal memory dereferences when dumping actions

[jamal]

On Wed, 2006-05-07 at 00:00 +0200, Thomas Graf wrote:
> plain text document attachment (act_fix_dump_null_deref)
> The TCA_ACT_KIND attribute is used without checking its
> availability when dumping actions therefore leading to a
> value of 0x4 being dereferenced.
> 
> The use of strcmp() in tc_lookup_action_n() isn't safe
> when fed with string from an attribute without enforcing
> proper NUL termination.
> 
> Both bugs can be triggered with malformed netlink message
> and don't require any privileges.
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> 

Good catch.

Acked-by: Jamal Hadi Salim <hadi@cyberus.ca>


cheers,
jamal

[PATCH 2/3] [PKT_SCHED]: Return ENOENT if action module is unavailable

[Thomas Graf]

[-- Attachment #1: act_fix_init_ret_val --]
[-- Type: text/plain, Size: 367 bytes --]

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Index: net-2.6.git/net/sched/act_api.c
===================================================================
--- net-2.6.git.orig/net/sched/act_api.c
+++ net-2.6.git/net/sched/act_api.c
@@ -305,6 +305,7 @@ struct tc_action *tcf_action_init_1(stru
 			goto err_mod;
 		}
 #endif
+		*err = -ENOENT;
 		goto err_out;
 	}
 

--

Re: [PATCH 2/3] [PKT_SCHED]: Return ENOENT if action module is unavailable

[jamal]

On Wed, 2006-05-07 at 00:00 +0200, Thomas Graf wrote:
> plain text document attachment (act_fix_init_ret_val)
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> 
> Index: net-2.6.git/net/sched/act_api.c
> ===================================================================
> --- net-2.6.git.orig/net/sched/act_api.c
> +++ net-2.6.git/net/sched/act_api.c
> @@ -305,6 +305,7 @@ struct tc_action *tcf_action_init_1(stru
>  			goto err_mod;
>  		}
>  #endif
> +		*err = -ENOENT;
>  		goto err_out;
>  	}
>  

Ok, this falls under the LinuxWay(tm). Quick inspection of the qdisc
code reveals the same bug. The cls side seems fine - but i didnt spend
more than 30 secs. So why dont you fix the qdisc one while you are at
it?

Acked-by: Jamal Hadi Salim <hadi@cyberus.ca>

[PATCH 3/3] [PKT_SCHED]: Fix error handling while dumping actions

[Thomas Graf]

[-- Attachment #1: act_fix_dump_err_handling --]
[-- Type: text/plain, Size: 932 bytes --]

"return -err" and blindly inheriting the error code in the netlink
failure exception handler causes errors codes to be returned as
positive value therefore making them being ignored by the caller.

May lead to sending out incomplete netlink messages.

Signed-off-by: Thomas Graf <tgraf@suug.ch>


Index: net-2.6.git/net/sched/act_api.c
===================================================================
--- net-2.6.git.orig/net/sched/act_api.c
+++ net-2.6.git/net/sched/act_api.c
@@ -250,15 +250,17 @@ tcf_action_dump(struct sk_buff *skb, str
 		RTA_PUT(skb, a->order, 0, NULL);
 		err = tcf_action_dump_1(skb, a, bind, ref);
 		if (err < 0)
-			goto rtattr_failure;
+			goto errout;
 		r->rta_len = skb->tail - (u8*)r;
 	}
 
 	return 0;
 
 rtattr_failure:
+	err = -EINVAL;
+errout:
 	skb_trim(skb, b - skb->data);
-	return -err;
+	return err;
 }
 
 struct tc_action *tcf_action_init_1(struct rtattr *rta, struct rtattr *est,

--

Re: [PATCH 3/3] [PKT_SCHED]: Fix error handling while dumping actions

[jamal]

I need to stare at this one for longer than 1 minute and i dont have
time right now; it does look strange (I am unsure what my thoughts were
at that point with -err - or maybe that was a change made by someone
else). 
I dont have time until tommorow - but i would  think the better fix will
be to change "return -err" to  "return -1"?

cheers,
jamal 

On Wed, 2006-05-07 at 00:00 +0200, Thomas Graf wrote:
> plain text document attachment (act_fix_dump_err_handling)
> "return -err" and blindly inheriting the error code in the netlink
> failure exception handler causes errors codes to be returned as
> positive value therefore making them being ignored by the caller.
> 
> May lead to sending out incomplete netlink messages.
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> 
> 
> Index: net-2.6.git/net/sched/act_api.c
> ===================================================================
> --- net-2.6.git.orig/net/sched/act_api.c
> +++ net-2.6.git/net/sched/act_api.c
> @@ -250,15 +250,17 @@ tcf_action_dump(struct sk_buff *skb, str
>  		RTA_PUT(skb, a->order, 0, NULL);
>  		err = tcf_action_dump_1(skb, a, bind, ref);
>  		if (err < 0)
> -			goto rtattr_failure;
> +			goto errout;
>  		r->rta_len = skb->tail - (u8*)r;
>  	}
>  
>  	return 0;
>  
>  rtattr_failure:
> +	err = -EINVAL;
> +errout:
>  	skb_trim(skb, b - skb->data);
> -	return -err;
> +	return err;
>  }
>  
>  struct tc_action *tcf_action_init_1(struct rtattr *rta, struct rtattr *est,
> 
> --
> 
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/3] [PKT_SCHED]: Fix error handling while dumping actions

[jamal]

Thomas, 

Please resubmit this patch with changing -err to -1 (i.e a one liner)
I went back to about 2.6.10 and this is in there. I looked at my notes
and i cant see any reasoning to explain this. So it is a bug.
Is there a way to check whether this was a result of some other earlier
change or was always there?

I would say patch #1 deserves to go into stable; the others for later.
And thank you for your efforts.

cheers,
jamal

On Tue, 2006-04-07 at 21:47 -0400, jamal wrote:
> I need to stare at this one for longer than 1 minute and i dont have
> time right now; it does look strange (I am unsure what my thoughts were
> at that point with -err - or maybe that was a change made by someone
> else). 
> I dont have time until tommorow - but i would  think the better fix will
> be to change "return -err" to  "return -1"?
> 
> cheers,
> jamal 
> 
> On Wed, 2006-05-07 at 00:00 +0200, Thomas Graf wrote:
> > plain text document attachment (act_fix_dump_err_handling)
> > "return -err" and blindly inheriting the error code in the netlink
> > failure exception handler causes errors codes to be returned as
> > positive value therefore making them being ignored by the caller.
> > 
> > May lead to sending out incomplete netlink messages.
> > 
> > Signed-off-by: Thomas Graf <tgraf@suug.ch>
> > 
> > 
> > Index: net-2.6.git/net/sched/act_api.c
> > ===================================================================
> > --- net-2.6.git.orig/net/sched/act_api.c
> > +++ net-2.6.git/net/sched/act_api.c
> > @@ -250,15 +250,17 @@ tcf_action_dump(struct sk_buff *skb, str
> >  		RTA_PUT(skb, a->order, 0, NULL);
> >  		err = tcf_action_dump_1(skb, a, bind, ref);
> >  		if (err < 0)
> > -			goto rtattr_failure;
> > +			goto errout;
> >  		r->rta_len = skb->tail - (u8*)r;
> >  	}
> >  
> >  	return 0;
> >  
> >  rtattr_failure:
> > +	err = -EINVAL;
> > +errout:
> >  	skb_trim(skb, b - skb->data);
> > -	return -err;
> > +	return err;
> >  }
> >  
> >  struct tc_action *tcf_action_init_1(struct rtattr *rta, struct rtattr *est,
> > 

Re: [PATCH 3/3] [PKT_SCHED]: Fix error handling while dumping actions

[Thomas Graf]

* jamal <hadi@cyberus.ca> 2006-07-05 09:35
> Please resubmit this patch with changing -err to -1 (i.e a one liner)
> I went back to about 2.6.10 and this is in there. I looked at my notes
> and i cant see any reasoning to explain this. So it is a bug.

Fine, if you think that's better.

> Is there a way to check whether this was a result of some other earlier
> change or was always there?

It was there from the very beginning:

http://kernel.org/git/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commitdiff;h=e96ec383ffef30843d34fe5766677dbffe0ca9a8

Re: [PATCH 3/3] [PKT_SCHED]: Fix error handling while dumping actions

[jamal]

On Wed, 2006-05-07 at 15:54 +0200, Thomas Graf wrote:
> * jamal <hadi@cyberus.ca> 2006-07-05 09:35
> > Please resubmit this patch with changing -err to -1 (i.e a one liner)
> > I went back to about 2.6.10 and this is in there. I looked at my notes
> > and i cant see any reasoning to explain this. So it is a bug.
> 
> Fine, if you think that's better.
> 

It is consistent with the others. err is not used afterwards.

> > Is there a way to check whether this was a result of some other earlier
> > change or was always there?
> 
> It was there from the very beginning:
> 
> http://kernel.org/git/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commitdiff;h=e96ec383ffef30843d34fe5766677dbffe0ca9a8

really fscked. So good eye.
For the future, can you tell me how to find to traceback that far?
Even within git: I know git-what-changed gives me logs on a file - but
how do i correlate that log to the exact change?

cheers,
jamal

Re: [PATCH 0/3] Action API fixes

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Wed, 05 Jul 2006 00:05:04 +0200

> Fixes for some rather serious action API bugs. Please apply.

All applied, I'll push to -stable, thanks Thomas.

Re: [PATCH 0/3] Action API fixes

[jamal]

On Wed, 2006-05-07 at 20:47 -0700, David Miller wrote:
> From: Thomas Graf <tgraf@suug.ch>
> Date: Wed, 05 Jul 2006 00:05:04 +0200
> 
> > Fixes for some rather serious action API bugs. Please apply.
> 
> All applied, I'll push to -stable, thanks Thomas.

Dave, Did you actually read my comments on the patches? 
One patch deserved to go to stable and one was the wrong fix.

Should i now send a patch against the one?

cheers,
jamal

Re: [PATCH 0/3] Action API fixes

[jamal]

[-- Attachment #1: Type: text/plain, Size: 709 bytes --]

On Thu, 2006-06-07 at 08:03 -0400, jamal wrote:
> On Wed, 2006-05-07 at 20:47 -0700, David Miller wrote:
> > From: Thomas Graf <tgraf@suug.ch>
> > Date: Wed, 05 Jul 2006 00:05:04 +0200
> > 
> > > Fixes for some rather serious action API bugs. Please apply.
> > 
> > All applied, I'll push to -stable, thanks Thomas.
> 
> Dave, Did you actually read my comments on the patches? 
> One patch deserved to go to stable and one was the wrong fix.
> 
> Should i now send a patch against the one?

The proper patch is attached as a replacement for 
"[PATCH 3/3] [PKT_SCHED]: Fix error handling while dumping action"
from Thomas. 
If you have already submitted it, then i will send a patch against it.

cheers,
jamal

[-- Attachment #2: nerr-act --]
[-- Type: text/plain, Size: 462 bytes --]


Fix dumping possibly returning a positive code
Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>
---

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 2ffa11c..0477c7a 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -259,7 +259,7 @@ tcf_action_dump(struct sk_buff *skb, str
 
 rtattr_failure:
 	skb_trim(skb, b - skb->data);
-	return -err;
+	return -1;
 }
 
 struct tc_action *tcf_action_init_1(struct rtattr *rta, struct rtattr *est,

Re: [PATCH 0/3] Action API fixes

[Thomas Graf]

* jamal <hadi@cyberus.ca> 2006-07-06 08:27
> 
> The proper patch is attached as a replacement for 
> "[PATCH 3/3] [PKT_SCHED]: Fix error handling while dumping action"
> from Thomas. 
> If you have already submitted it, then i will send a patch against it.

I don't understand why you're picky about this but if you really
want to then do it properly and remove the now unused err variable,
don't leave obscure code behind again.

Re: [PATCH 0/3] Action API fixes

[jamal]

On Thu, 2006-06-07 at 14:45 +0200, Thomas Graf wrote:
> * jamal <hadi@cyberus.ca> 2006-07-06 08:27
> > 
> > The proper patch is attached as a replacement for 
> > "[PATCH 3/3] [PKT_SCHED]: Fix error handling while dumping action"
> > from Thomas. 
> > If you have already submitted it, then i will send a patch against it.
> 
> I don't understand why you're picky about this but if you really
> want to then do it properly and remove the now unused err variable,
> don't leave obscure code behind again.

For heavens sake, do we have to argue even about this? 
look at the code calmly and rationally and tell me if your fix was even
close to right. It was a bug. All bugs are obscure.

cheers,
jamal

Re: [PATCH 0/3] Action API fixes

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Thu, 6 Jul 2006 14:45:39 +0200

> * jamal <hadi@cyberus.ca> 2006-07-06 08:27
> > 
> > The proper patch is attached as a replacement for 
> > "[PATCH 3/3] [PKT_SCHED]: Fix error handling while dumping action"
> > from Thomas. 
> > If you have already submitted it, then i will send a patch against it.
> 
> I don't understand why you're picky about this but if you really
> want to then do it properly and remove the now unused err variable,
> don't leave obscure code behind again.

I don't either, I think Thomas's patch was correct and that's
why his is the one I applied.

Sometimes it is necessary for one to buffer their resistence to the
way someone fixes you code and just let it go.  Otherwise people
won't even bother trying to fix those bugs.

People who do the work make the rules, that is my supreme motto.  And
here that is Thomas, and therefore I will favor in the direction of
his judgement.

Re: [PATCH 0/3] Action API fixes

[David Miller]

From: jamal <hadi@cyberus.ca>
Date: Thu, 06 Jul 2006 08:03:48 -0400

> Dave, Did you actually read my comments on the patches? 
> One patch deserved to go to stable and one was the wrong fix.
> 
> Should i now send a patch against the one?

I read your comments.  Thomas's patches were correct, you comments
were refinements at best.