Action API fixes

PATCH In review Thomas Graf 4 participants 20 messages Jul 4, 2006 – Jul 7, 2006

Subsystems (3)

Networking Network drivers Traffic control (tc)

lore

Share & tools

lore

Thomas Graf

author

2006-07-04 22:06 UTC #0 lore raw

cover Action API fixes

Dave,

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

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

Thomas Graf

author

2006-07-04 22:06 UTC #1 lore raw

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

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 <[email protected]>

net/sched/act_api.c +5 -6

Index: 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;
 	}

Patrick McHardy

2006-07-04 23:44 UTC #2 lore raw

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

Quoted 55 lines · Thomas Graf

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 <[email protected]>
Index: net-2.6.git/net/sched/act_api.c
===================================================================
net/sched/act_api.c +4 -5
@@ -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;
 	}
 

net/sched/act_api.c tc_dump_action(struct sk_buff *skb, stru · line 838

Quoted 7 lines

net/sched/act_api.c +1 -1

@@ -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.

jamal

2006-07-05 01:49 UTC #3 lore raw

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

Quoted 10 lines · Patrick McHardy

net/sched/act_api.c tc_dump_action(struct sk_buff *skb, stru · line 838
Quoted 5 lines · Thomas Graf
net/sched/act_api.c +1 -1
 	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

Thomas Graf

author

2006-07-05 09:09 UTC #4 lore raw

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

In reply to Patrick McHardy

* Patrick McHardy <[email protected]> 2006-07-05 01:42

Quoted 4 lines

Quoted 3 lines · Thomas Graf
net/sched/act_api.c +0 -0
@@ -834,7 +833,7 @@ tc_dump_action(struct sk_buff *skb, stru
 	a.ops = a_o;
 

Quoted 8 lines

net/sched/act_api.c tc_dump_action(struct sk_buff *skb, stru · line 838
Quoted 5 lines
net/sched/act_api.c +1 -1
 	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.

jamal

Acked-by on v1 1/3

2006-07-05 01:34 UTC #5 lore raw

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

In reply to Thomas Graf

Quoted 5 lines · Thomas Graf

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.

Quoted 9 lines

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 <[email protected]>

Good catch.

Acked-by Jamal Hadi Salim <[email protected]>

cheers,
jamal

Thomas Graf

author

2006-07-04 22:06 UTC #6 lore raw

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

In reply to Thomas Graf

Signed-off-by Thomas Graf <[email protected]>

net/sched/act_api.c +1 -0

Index: 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;
 	}

jamal

Acked-by on v1 2/3

2006-07-05 01:40 UTC #7 lore raw

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

Quoted 8 lines · Thomas Graf

plain text document attachment (act_fix_init_ret_val)
Signed-off-by Thomas Graf <[email protected]>
Index: net-2.6.git/net/sched/act_api.c
===================================================================

net/sched/act_api.c struct tc_action *tcf_action_init_1(stru · line 311

Quoted 7 lines

net/sched/act_api.c +1 -0

@@ -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 <[email protected]>

Thomas Graf

author

2006-07-04 22:06 UTC #8 lore raw

PATCH 3/3 : Fix error handling while dumping actions

In reply to Thomas Graf

"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 <[email protected]>

net/sched/act_api.c +4 -2

Index: 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,

jamal

2006-07-05 01:47 UTC #9 lore raw

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

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

Quoted 42 lines · Thomas Graf

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 <[email protected]>
Index: net-2.6.git/net/sched/act_api.c
===================================================================
net/sched/act_api.c +4 -4
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

jamal

2006-07-05 13:35 UTC #10 lore raw

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

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

Quoted 46 lines · 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
Quoted 35 lines · Thomas Graf
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 <[email protected]>
Index: net-2.6.git/net/sched/act_api.c
===================================================================
net/sched/act_api.c +4 -2
@@ -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,

Thomas Graf

author

2006-07-05 13:54 UTC #11 lore raw

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

* jamal <[email protected]> 2006-07-05 09:35

Quoted 3 lines

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.

Quoted 2 lines

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

jamal

2006-07-05 14:00 UTC #12 lore raw

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

Quoted 7 lines · Thomas Graf

* jamal <[email protected]> 2006-07-05 09:35
Quoted 3 lines
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.

Quoted 6 lines

Quoted 2 lines
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

David Miller

maintainer

2006-07-06 03:47 UTC #13 lore raw

In reply to Thomas Graf

From: Thomas Graf <[email protected]>
Date: Wed, 05 Jul 2006 00:05:04 +0200

Quoted 1 line

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

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

jamal

2006-07-06 12:03 UTC #14 lore raw

Quoted 6 lines · David Miller

From: Thomas Graf <[email protected]>
Date: Wed, 05 Jul 2006 00:05:04 +0200
Quoted 1 line
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

jamal

2006-07-06 12:27 UTC #15 lore raw

Quoted 4 lines · jamal

Quoted 3 lines · David Miller
From: Thomas Graf <[email protected]>
Date: Wed, 05 Jul 2006 00:05:04 +0200

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
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

Fix dumping possibly returning a positive code

Signed-off-by Jamal Hadi Salim <[email protected]>

---

net/sched/act_api.c +1 -1

index 2ffa11c..0477c7a 100644
@@ -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,

Thomas Graf

author

2006-07-06 12:45 UTC #16 lore raw

* jamal <[email protected]> 2006-07-06 08:27

Quoted 5 lines

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.

jamal

2006-07-06 12:53 UTC #17 lore raw

Quoted 10 lines · Thomas Graf

* jamal <[email protected]> 2006-07-06 08:27
Quoted 5 lines
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

David Miller

maintainer

2006-07-07 06:56 UTC #18 lore raw

In reply to Thomas Graf

From: Thomas Graf <[email protected]>
Date: Thu, 6 Jul 2006 14:45:39 +0200

Quoted 10 lines

* jamal <[email protected]> 2006-07-06 08:27
Quoted 5 lines
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.

David Miller

maintainer

2006-07-07 06:54 UTC #19 lore raw

In reply to jamal

From: jamal <[email protected]>
Date: Thu, 06 Jul 2006 08:03:48 -0400

Quoted 4 lines

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.