[PATCH] PKT_SCHED: Fix cls indev validation

[PATCH] PKT_SCHED: Fix cls indev validation

[Thomas Graf]

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

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

--- linux-2.6.10-rc3-bk7.orig/include/net/pkt_cls.h	2004-12-14 14:24:04.000000000 +0100
+++ linux-2.6.10-rc3-bk7/include/net/pkt_cls.h	2004-12-14 14:54:41.000000000 +0100
@@ -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
--- linux-2.6.10-rc3-bk7.orig/net/sched/cls_u32.c	2004-12-14 14:24:34.000000000 +0100
+++ linux-2.6.10-rc3-bk7/net/sched/cls_u32.c	2004-12-14 14:55:18.000000000 +0100
@@ -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;
--- linux-2.6.10-rc3-bk7.orig/net/sched/cls_fw.c	2004-12-14 14:24:34.000000000 +0100
+++ linux-2.6.10-rc3-bk7/net/sched/cls_fw.c	2004-12-14 14:33:50.000000000 +0100
@@ -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

Re: [PATCH] PKT_SCHED: Fix cls indev validation

[Patrick McHardy]

Thomas Graf wrote:
> 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.

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) {

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

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.

> +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';
> +}

And this should just use strlcpy.

Regards
Patrick

Re: [PATCH] PKT_SCHED: Fix cls indev validation

[Thomas Graf]

* Patrick McHardy <41C68CEF.3030803@trash.net> 2004-12-20 09:27
> Thomas Graf wrote:
> >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.
> 
> 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) {

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.

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

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

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

> >+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';
> >+}
> 
> And this should just use strlcpy.

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.

Re: [PATCH] PKT_SCHED: Fix cls indev validation

[jamal]

On Mon, 2004-12-20 at 03:27, Patrick McHardy wrote:

> 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) {
> 

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?

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

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

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

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

cheers,
jamal

Re: [PATCH] PKT_SCHED: Fix cls indev validation

[Thomas Graf]

* jamal <1103552215.1048.333.camel@jzny.localdomain> 2004-12-20 09:16
> 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?

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

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

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

Re: [PATCH] PKT_SCHED: Fix cls indev validation

[Thomas Graf]

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.

Re: [PATCH] PKT_SCHED: Fix cls indev validation

[David S. Miller]

On Tue, 21 Dec 2004 01:22:30 +0100
Thomas Graf <tgraf@suug.ch> wrote:

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

Re: [PATCH] PKT_SCHED: Fix cls indev validation

[Patrick McHardy]

Thomas Graf wrote:
> * jamal <1103552215.1048.333.camel@jzny.localdomain> 2004-12-20 09:16
> 
>>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?
> 
> I'll put it into my patchset so it gets into the test cycles.

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

Re: [PATCH] PKT_SCHED: Fix cls indev validation

[Thomas Graf]

* Patrick McHardy <41C7F833.4000909@trash.net> 2004-12-21 11:17
> Could you make your patchset available somehow ?

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

Unfinished and untested.

> I'm not sure which areas of the code I can still
> touch without conflicting with you :)

 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(-)

Re: [PATCH] PKT_SCHED: Fix cls indev validation

[Patrick McHardy]

Thomas Graf wrote:
> * Patrick McHardy <41C7F833.4000909@trash.net> 2004-12-21 11:17
> 
>>I'm not sure which areas of the code I can still
>>touch without conflicting with you :)
> 
> 
>  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

Re: [PATCH] PKT_SCHED: Fix cls indev validation

[jamal]

On Tue, 2004-12-21 at 19:31, Thomas Graf wrote:
> * Patrick McHardy <41C7F833.4000909@trash.net> 2004-12-21 11:17
> > Could you make your patchset available somehow ?
> 
> http://people.suug.ch/~tgr/patches/queue/
> 
> Unfinished and untested.

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

Re: [PATCH] PKT_SCHED: Fix cls indev validation

[Thomas Graf]

* jamal <1103722366.1089.75.camel@jzny.localdomain> 2004-12-22 08:32
> On Tue, 2004-12-21 at 19:31, Thomas Graf wrote:
> > * Patrick McHardy <41C7F833.4000909@trash.net> 2004-12-21 11:17
> > > Could you make your patchset available somehow ?
> > 
> > http://people.suug.ch/~tgr/patches/queue/
> > 
> > Unfinished and untested.
> 
> 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.

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.

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

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

> 3) What can i say? dang - this indev thing is getting out of control ;->

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

> If you are going to go this far for beautification sake then
> kill the .indev thing please before it becomes a beast.

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.

> 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

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.

> I think its time we did this right than defering.

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.

Re: [PATCH] PKT_SCHED: Fix cls indev validation

[jamal]

On Wed, 2004-12-22 at 09:26, Thomas Graf wrote:
> * jamal <1103722366.1089.75.camel@jzny.localdomain> 2004-12-22 08:32
> > On Tue, 2004-12-21 at 19:31, Thomas Graf wrote:
> > > * Patrick McHardy <41C7F833.4000909@trash.net> 2004-12-21 11:17
> > > > Could you make your patchset available somehow ?
> > > 
> > > http://people.suug.ch/~tgr/patches/queue/
> > > 
> > > Unfinished and untested.
> > 
> > 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.
> 
> I fully agree. The patchset looks like a beautification but it isn't.
> Its main purpose is to make changing consistent again. 

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 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.
> 
> > 2) The name tcf_attrs doesnt sound right - attributes are normally
> > data pieces not methods. Cant think of a good name.
> 
> I feel the same, I was thinking of extensions but wasn't pleased either.
> Suggestions appreciated.
> 
> > 3) What can i say? dang - this indev thing is getting out of control ;->
> 
> Too late, it's there, we must maintain it ;->
> 

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.


> 
> > I think its time we did this right than defering.
> 
> 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.

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

cheers,
jamal