PKT_SCHED: Fix cls indev validation

Dave,

This patch is actually part of a patchset for 2.6.11 inclusion but the memory corruption possibility might make it worth for 2.6.10. It's not really dangerous since it requires admin capabilities though.

Puts the indev validation into its own function to allow parameter validation before any changes are made. Changes the sanity check from >= IFNAMSIZ to > IFNAMSIZ to correctly handle 0 terminated strings and replaces the dangerous sprintf with a memcpy bound to the TLV size. Providing a indev TLV for kernels without CONFIG_NET_CLS_IND support will now lead to EOPPNOTSUPP.

The sprintf could have been abused to overwrite memory after the indev attribute if the TLV data was not 0 terminated. (Bound to a few limitations, e.g. it would have to contain a rta header at RTA_ALIGN(IFNAMSIZ)+1 etc.)

@@ -160,19 +160,25 @@
 
 #ifdef CONFIG_NET_CLS_IND
 static inline int
-tcf_change_indev(struct tcf_proto *tp, char *indev, struct rtattr *indev_tlv)
+tcf_validate_indev(struct rtattr *indev_tlv)
 {
-	if (RTA_PAYLOAD(indev_tlv) >= IFNAMSIZ) {
-		printk("cls: bad indev name %s\n", (char *) RTA_DATA(indev_tlv));
+	if (indev_tlv && RTA_PAYLOAD(indev_tlv) > IFNAMSIZ) {
+		if (net_ratelimit())
+			printk("cls: bad indev name %s\n", (char *) RTA_DATA(indev_tlv));
 		return -EINVAL;
 	}
 
-	memset(indev, 0, IFNAMSIZ);
-	sprintf(indev, "%s", (char *) RTA_DATA(indev_tlv));
-
 	return 0;
 }
 
+static inline void
+tcf_change_indev(struct tcf_proto *tp, char *indev, struct rtattr *id_tlv)
+{
+	memset(indev, 0, IFNAMSIZ);
+	memcpy(indev, RTA_DATA(id_tlv), RTA_PAYLOAD(id_tlv));
+	indev[IFNAMSIZ - 1] = '\0';
+}
+
 static inline int
 tcf_match_indev(struct sk_buff *skb, char *indev)
 {

@@ -185,6 +191,12 @@
 
 	return 1;
 }
+#else /* CONFIG_NET_CLS_IND */
+static inline int
+tcf_validate_indev(struct rtattr *indev_tlv)
+{
+	return indev_tlv ? -EOPNOTSUPP : 0;
+}
 #endif /* CONFIG_NET_CLS_IND */
 
 #ifdef CONFIG_NET_CLS_POLICE

@@ -488,6 +488,12 @@
 			 struct tc_u_knode *n, struct rtattr **tb,
 			 struct rtattr *est)
 {
+	int err;
+
+	err = tcf_validate_indev(tb[TCA_U32_INDEV-1]);
+	if (err < 0)
+		return err;
+
 	if (tb[TCA_U32_LINK-1]) {
 		u32 handle = *(u32*)RTA_DATA(tb[TCA_U32_LINK-1]);
 		struct tc_u_hnode *ht_down = NULL;

@@ -535,12 +541,10 @@
 	}
 #endif
 #endif
+
 #ifdef CONFIG_NET_CLS_IND
-	if (tb[TCA_U32_INDEV-1]) {
-		int err = tcf_change_indev(tp, n->indev, tb[TCA_U32_INDEV-1]);
-		if (err < 0)
-			return err;
-	}
+	if (tb[TCA_U32_INDEV-1])
+		tcf_change_indev(tp, n->indev, tb[TCA_U32_INDEV-1]);
 #endif
 
 	return 0;

@@ -208,21 +208,24 @@
 fw_change_attrs(struct tcf_proto *tp, struct fw_filter *f,
 	struct rtattr **tb, struct rtattr **tca, unsigned long base)
 {
-	int err = -EINVAL;
+	int err;
+
+	err = tcf_validate_indev(tb[TCA_FW_INDEV-1]);
+	if (err < 0)
+		goto errout;
 
 	if (tb[TCA_FW_CLASSID-1]) {
-		if (RTA_PAYLOAD(tb[TCA_FW_CLASSID-1]) != sizeof(u32))
+		if (RTA_PAYLOAD(tb[TCA_FW_CLASSID-1]) != sizeof(u32)) {
+			err = -EINVAL;
 			goto errout;
+		}
 		f->res.classid = *(u32*)RTA_DATA(tb[TCA_FW_CLASSID-1]);
 		tcf_bind_filter(tp, &f->res, base);
 	}
 
 #ifdef CONFIG_NET_CLS_IND
-	if (tb[TCA_FW_INDEV-1]) {
-		err = tcf_change_indev(tp, f->indev, tb[TCA_FW_INDEV-1]);
-		if (err < 0)
-			goto errout;
-	}
+	if (tb[TCA_FW_INDEV-1])
+		tcf_change_indev(tp, f->indev, tb[TCA_FW_INDEV-1]);
 #endif /* CONFIG_NET_CLS_IND */
 
 #ifdef CONFIG_NET_CLS_ACT

There are lots of places where this is possible, just look at all the silly checks in the action code:

   sprintf(act_name, "%s", (char*)RTA_DATA(kind));
   if (RTA_PAYLOAD(kind) >= IFNAMSIZ) {

Why special-case indev ? Neither u32 nor fw make any attempt to clean up after errors in their init functions, so instead of fixing a single attribute they need to do proper cleanup, than we can just continue to validate indev when changing it. Returning EOPNOTSUPP makes sense, of course.

I'm also against keeping all those printks when touching the code. Its ok when writing new code, but I don't see why this code, unlike everything else, needs to report errors in the ringbuffer instead of returning meaningful error codes.

* Patrick McHardy <[email protected]> 2004-12-20 09:27

Agreed, also the recursive algorithms are vulnerable but this doesn't mean we should just ignore it. I'm fine with droping this for 2.6.10 and wait for my complete patchset.

As I said, the patch is part of a patchset which cleans up all the init paths. I will introduce a new abstraction layer called tcf_attrs which hides action/police configuration, removes all the ifdefs without polluting the cls config data structures and enforces the policy "change everything or nothing" to make things consistent again. (They were once)

It basically works by having a struct tcf_attrs which ifdefs tc_action, tcf_police and is empty if none of them are configured. The classifiers include it in their private data and call tcf_attrs_validate() and tcf_attrs_change() to validate respectively commit the configuration requests or tcf_attrs_match() to match them. The TLV mapping is done via tcf_attr_map which must be provided by the classifiers:

static struct tcf_attr_map u32_map = {
	.action = TCA_U32_ACT,
	.police = TCA_U32_POLICE,
};

Speaking of it, would everyone agree to put indev matching into the abstraction layer as well so it is available for all classifiers? It doesn't cost us anything except a few iproute2 additions.

I didn't want to change behaviour because there might be someone checking for it, it's not that I'd like it.

No, RTA_DATA(id_tlv) is not guaranteed to be NUL terminated and internal calls to strlen might crash. Even if it would be safe, I don't like using str functions if NUL termination is not guaranteed.

Hehe. I am sure thats a cutnpaste(LinuxWay) from some code in the kernel probably sch_api.c (or maybe the code it was cutnpasted has been fixed in the last 3 years ;->).
That needs fixing. Who is sending the patch?

Indev is going to die - no need in investing a lot of effort in it.

Its not that expensive since done on config path. But agree when proper codes exist its not needed.

cheers,
jamal

* jamal <[email protected]> 2004-12-20 09:16

I'll put it into my patchset so it gets into the test cycles.

It might cause random data to be printed onto the console ;) I'll remove it in my patchset.

Could you make your patchset available somehow ?
I'm not sure which areas of the code I can still
touch without conflicting with you :)

Regards
Patrick

* Patrick McHardy <[email protected]> 2004-12-21 11:17

http://people.suug.ch/~tgr/patches/queue/

Unfinished and untested.

 include/linux/pkt_cls.h   |    6 
 include/linux/rtnetlink.h |    3 
 include/net/pkt_cls.h     |   45 +++++
 net/sched/cls_api.c       |  205 +++++++++++++++++++++++++
 net/sched/cls_fw.c        |  145 +++---------------
 net/sched/cls_route.c     |  355 ++++++++++++++++++++++++--------------------
 net/sched/cls_rsvp.h      |   88 +++++------
 net/sched/cls_tcindex.c   |  364 +++++++++++++++++++++++++---------------------
 net/sched/cls_u32.c       |  131 ++++------------
 9 files changed, 766 insertions(+), 576 deletions(-)

Thanks. I'm going to take on the action code then.

Regards
Patrick

I just took a quick glimpse.

1)Recall: Policer will have to die at some point - only reason for its existence is for backward compat.
New iproute2 code sooner than later stop using that inteface so we can kill it. I suspect we can kill it in a year or two and definetely the day 2.7 comes out.

2) The name tcf_attrs doesnt sound right - attributes are normally data pieces not methods. Cant think of a good name.

3) What can i say? dang - this indev thing is getting out of control ;-> If you are going to go this far for beautification sake then kill the .indev thing please before it becomes a beast. Do what we discussed a while back:
- have a generic very basic extended generic match API which indev uses that gets invoked from the classifier. It should take no more than one page to write the indev extension - if it exceeds that you are doing something wrong. There should be capability to mix and match these extenders so i can say in u32 something like:
match ip src X
match extend indev src eth0
match protocol tcp
match extended metadata fwmark 0x10

etc.

I think its time we did this right than defering.

Of course all backward compatibility rules apply ;->

cheers,
jamal

* jamal <[email protected]> 2004-12-22 08:32

I fully agree. The patchset looks like a beautification but it isn't. Its main purpose is to make changing consistent again. I tried achieving this with the existing API and the ifdef hell got horrible and ended up having over 60 lines of redundant code per classifier. I would rather be implementing new fancy stuff but fixing the existing issues comes first.

I feel the same, I was thinking of extensions but wasn't pleased either. Suggestions appreciated.

Too late, it's there, we must maintain it ;->

Again, it's not for beautification, validating the indev tlv must be done before changing native classifier attributes which lead to more ifdefs. Putting it into tcf_attrs saves code and makes it available to the other classifiers at the same time. It's a dodgy situation, the current status is unsatisfying and all changes made now will be removed again.

My actual plan was to introduce a nested TCA_TYPE_ATTRS TLV containing all the generic matches and attributes so a classifier no longer has to be changed but the backward compatibility will be a PITA.

Indeed, what about this: I'll try and do a proposal on a new generic matching layer holding the action bits and providing backward compatibility to police/indev. We can then build the metadata match on top of it. I'm going to post the classifier I'm working on for a long time even if it's still buggy and unfinished so we can at least take over some ideas and concepts and then come up with something final that doesn't need to be changed again in 2 months.

Except there is a continous stream of patches to cleanup cleanups ;-> Thats where the cosmetics definition comes in.
Lets just do it right and get it over with.

I think needs to be fixed. Theres a clear bold warning that it would die. We shouldnt keep building more walls and adding gardens around it.

Ok, waiting to see this. Post it on the list.

cheers,
jamal

Dave, drop this one. I will move indev into the abstraction layer i'm going to introduce .11 which will fix the current issues, remove the ifdefs and will make removal/replacement much simpler once we have metadata match.

OK.