Re: [LARTC] [BUG?] ip ru flush && RTNETLINK answers: Numerical result out of range

Re: [LARTC] [BUG?] ip ru flush && RTNETLINK answers: Numerical result out of range

[Patrick McHardy]

Luciano Ruete wrote:
> After an: 
> # ip ru flush
> I loose all my ip rules but the priority 0 one. 
> root@sarasvati:~# ip ru
> 0:      from all lookup 255
> root@sarasvati:~#  
> 
> Ok with that, but now i'm not able to insert any new rule.
> This leads to a total loose of conectivity.
> 
> root@sarasvati:~# ip ru add from all table default
> RTNETLINK answers: Numerical result out of range
> root@sarasvati:~# ip ru add from all lookup main
> RTNETLINK answers: Numerical result out of range
> 
> Even seting the priority value by hand, i got the same error:
> 
> root@sarasvati:~# ip ru add from all lookup main priority 32766
> RTNETLINK answers: Numerical result out of range
> 
> To be able to send this e-mail without rebooting i had to insert my gw ip 
> routes in table 255.
> 
> Is this a bug in iproute?
> 
> Some adiotional data:
> ip utility, iproute2-ss060323
> Linux sarasvati 2.6.20-5-386 #2 Sat Jan 6 14:44:57 UTC 2007 i686 GNU/Linux


The problem seems to be the nla policy added in 2.6.19 or 2.6.20.
When specifying a prefix as "all", iproute adds a zero byte long
attribute (FRA_SRC in this case). The IPv4 fib_rules policy states
that it has to be exactly 4 bytes long, which makes validation fail.
This also affects IPv6 and DECnet.

I would argue that iproute is broken and shouldn't add a zero
byte long attribute, but we still need to make sure the kernel
accepts these attributes as valid.

Thomas, I can't see a clean way to fix this right now that
doesn't either bloat struct nla_policy or removes FRA_SRC/FRA_DST
from the policy, could you please look into this? Thanks.

Re: [LARTC] [BUG?] ip ru flush && RTNETLINK answers: Numerical result out of range

[Thomas Graf]

* Patrick McHardy <kaber@trash.net> 2007-03-19 06:54
> Thomas, I can't see a clean way to fix this right now that
> doesn't either bloat struct nla_policy or removes FRA_SRC/FRA_DST
> from the policy, could you please look into this? Thanks.

I guess the only way is to remove FRA_SRC/FRA_DST from the policy
and validate it in configure() based on src_len/dst_len.

Re: [BUG?] ip ru flush && RTNETLINK answers: Numerical result out of range

[Patrick McHardy]

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

Thomas Graf wrote:
> * Patrick McHardy <kaber@trash.net> 2007-03-19 06:54
> 
>>Thomas, I can't see a clean way to fix this right now that
>>doesn't either bloat struct nla_policy or removes FRA_SRC/FRA_DST
>>from the policy, could you please look into this? Thanks.
> 
> 
> I guess the only way is to remove FRA_SRC/FRA_DST from the policy
> and validate it in configure() based on src_len/dst_len.


Its not too pretty, but I agree. This patch fixes the problem.
I'll also push it to -stable.

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 3942 bytes --]

[NET]: Fix fib_rules compatibility breakage

The fib_rules netlink attribute policy introduced in 2.6.19 broke
userspace compatibilty. When specifying a rule with "from all"
or "to all", iproute adds a zero byte long netlink attribute,
but the policy requires all addresses to have a size equal to
sizeof(struct in_addr)/sizeof(struct in6_addr), resulting in a
validation error.

Fix by only looking at the FRA_SRC/FRA_DST attributes if src_len
or dst_len is larger than zero.

DECnet is unaffected since iproute doesn't support specifying
addresses as "all".

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 39f42dd26f1f9c93b9700e1bace540ed9bb94e46
tree ecc71ef742d9d636bf129b34ae7a18173377ccc0
parent db98e0b434a6265c451ffe94ec0a29b8d0aaf587
author Patrick McHardy <kaber@trash.net> Tue, 20 Mar 2007 07:08:38 +0100
committer Patrick McHardy <kaber@trash.net> Tue, 20 Mar 2007 07:08:38 +0100

 net/ipv4/fib_rules.c  |   18 ++++++++++++------
 net/ipv6/fib6_rules.c |   16 ++++++++++++----
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/net/ipv4/fib_rules.c b/net/ipv4/fib_rules.c
index b837c33..9524b2e 100644
--- a/net/ipv4/fib_rules.c
+++ b/net/ipv4/fib_rules.c
@@ -171,8 +171,6 @@ static struct fib_table *fib_empty_table
 
 static struct nla_policy fib4_rule_policy[FRA_MAX+1] __read_mostly = {
 	FRA_GENERIC_POLICY,
-	[FRA_SRC]	= { .type = NLA_U32 },
-	[FRA_DST]	= { .type = NLA_U32 },
 	[FRA_FLOW]	= { .type = NLA_U32 },
 };
 
@@ -187,6 +185,12 @@ static int fib4_rule_configure(struct fi
 	    (frh->tos & ~IPTOS_TOS_MASK))
 		goto errout;
 
+	if (frh->src_len && tb[FRA_SRC] && nla_len(tb[FRA_SRC]) != sizeof(u32))
+		goto errout;
+
+	if (frh->dst_len && tb[FRA_DST] && nla_len(tb[FRA_DST]) != sizeof(u32))
+		goto errout;
+
 	if (rule->table == RT_TABLE_UNSPEC) {
 		if (rule->action == FR_ACT_TO_TBL) {
 			struct fib_table *table;
@@ -201,10 +205,10 @@ static int fib4_rule_configure(struct fi
 		}
 	}
 
-	if (tb[FRA_SRC])
+	if (frh->src_len && tb[FRA_SRC])
 		rule4->src = nla_get_be32(tb[FRA_SRC]);
 
-	if (tb[FRA_DST])
+	if (frh->dst_len && tb[FRA_DST])
 		rule4->dst = nla_get_be32(tb[FRA_DST]);
 
 #ifdef CONFIG_NET_CLS_ROUTE
@@ -242,10 +246,12 @@ #ifdef CONFIG_NET_CLS_ROUTE
 		return 0;
 #endif
 
-	if (tb[FRA_SRC] && (rule4->src != nla_get_be32(tb[FRA_SRC])))
+	if (frh->src_len && tb[FRA_SRC] &&
+	    (rule4->src != nla_get_be32(tb[FRA_SRC])))
 		return 0;
 
-	if (tb[FRA_DST] && (rule4->dst != nla_get_be32(tb[FRA_DST])))
+	if (frh->dst_len && tb[FRA_DST] &&
+	    (rule4->dst != nla_get_be32(tb[FRA_DST])))
 		return 0;
 
 	return 1;
diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index 0862809..a15244e 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -145,6 +145,14 @@ static int fib6_rule_configure(struct fi
 	if (frh->src_len > 128 || frh->dst_len > 128)
 		goto errout;
 
+	if (frh->src_len && tb[FRA_SRC] &&
+	    nla_len(tb[FRA_SRC]) != sizeof(struct in6_addr))
+		goto errout;
+
+	if (frh->dst_len && tb[FRA_DST] &&
+	    nla_len(tb[FRA_DST]) != sizeof(struct in6_addr))
+		goto errout;
+
 	if (rule->action == FR_ACT_TO_TBL) {
 		if (rule->table == RT6_TABLE_UNSPEC)
 			goto errout;
@@ -155,11 +163,11 @@ static int fib6_rule_configure(struct fi
 		}
 	}
 
-	if (tb[FRA_SRC])
+	if (frh->src_len && tb[FRA_SRC])
 		nla_memcpy(&rule6->src.addr, tb[FRA_SRC],
 			   sizeof(struct in6_addr));
 
-	if (tb[FRA_DST])
+	if (frh->dst_len && tb[FRA_DST])
 		nla_memcpy(&rule6->dst.addr, tb[FRA_DST],
 			   sizeof(struct in6_addr));
 
@@ -186,11 +194,11 @@ static int fib6_rule_compare(struct fib_
 	if (frh->tos && (rule6->tclass != frh->tos))
 		return 0;
 
-	if (tb[FRA_SRC] &&
+	if (frh->src_len && tb[FRA_SRC] &&
 	    nla_memcmp(tb[FRA_SRC], &rule6->src.addr, sizeof(struct in6_addr)))
 		return 0;
 
-	if (tb[FRA_DST] &&
+	if (frh->dst_len && tb[FRA_DST] &&
 	    nla_memcmp(tb[FRA_DST], &rule6->dst.addr, sizeof(struct in6_addr)))
 		return 0;
 


[-- Attachment #3: Type: text/plain, Size: 143 bytes --]

_______________________________________________
LARTC mailing list
LARTC@mailman.ds9a.nl
http://mailman.ds9a.nl/cgi-bin/mailman/listinfo/lartc

Re: [LARTC] [BUG?] ip ru flush && RTNETLINK answers: Numerical result out of range

[Patrick McHardy]

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

Patrick McHardy wrote:
> [NET]: Fix fib_rules compatibility breakage

I forgot to remove FRA_SRC/FRA_DST from fib6_rule_policy.
Updated patch attached.


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 4282 bytes --]

[NET]: Fix fib_rules compatibility breakage

The fib_rules netlink attribute policy introduced in 2.6.19 broke
userspace compatibilty. When specifying a rule with "from all"
or "to all", iproute adds a zero byte long netlink attribute,
but the policy requires all addresses to have a size equal to
sizeof(struct in_addr)/sizeof(struct in6_addr), resulting in a
validation error.

Fix by only looking at the FRA_SRC/FRA_DST attributes if src_len
or dst_len is larger than zero.

DECnet is unaffected since iproute doesn't support specifying
addresses as "all".

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 676307508e675dcf434f4f18e619b195f2f503ad
tree aa87342144b3b19ebddb5011380c3ac96c38dbbc
parent db98e0b434a6265c451ffe94ec0a29b8d0aaf587
author Patrick McHardy <kaber@trash.net> Tue, 20 Mar 2007 07:08:38 +0100
committer Patrick McHardy <kaber@trash.net> Tue, 20 Mar 2007 07:42:19 +0100

 net/ipv4/fib_rules.c  |   18 ++++++++++++------
 net/ipv6/fib6_rules.c |   18 ++++++++++++------
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/net/ipv4/fib_rules.c b/net/ipv4/fib_rules.c
index b837c33..9524b2e 100644
--- a/net/ipv4/fib_rules.c
+++ b/net/ipv4/fib_rules.c
@@ -171,8 +171,6 @@ static struct fib_table *fib_empty_table
 
 static struct nla_policy fib4_rule_policy[FRA_MAX+1] __read_mostly = {
 	FRA_GENERIC_POLICY,
-	[FRA_SRC]	= { .type = NLA_U32 },
-	[FRA_DST]	= { .type = NLA_U32 },
 	[FRA_FLOW]	= { .type = NLA_U32 },
 };
 
@@ -187,6 +185,12 @@ static int fib4_rule_configure(struct fi
 	    (frh->tos & ~IPTOS_TOS_MASK))
 		goto errout;
 
+	if (frh->src_len && tb[FRA_SRC] && nla_len(tb[FRA_SRC]) != sizeof(u32))
+		goto errout;
+
+	if (frh->dst_len && tb[FRA_DST] && nla_len(tb[FRA_DST]) != sizeof(u32))
+		goto errout;
+
 	if (rule->table == RT_TABLE_UNSPEC) {
 		if (rule->action == FR_ACT_TO_TBL) {
 			struct fib_table *table;
@@ -201,10 +205,10 @@ static int fib4_rule_configure(struct fi
 		}
 	}
 
-	if (tb[FRA_SRC])
+	if (frh->src_len && tb[FRA_SRC])
 		rule4->src = nla_get_be32(tb[FRA_SRC]);
 
-	if (tb[FRA_DST])
+	if (frh->dst_len && tb[FRA_DST])
 		rule4->dst = nla_get_be32(tb[FRA_DST]);
 
 #ifdef CONFIG_NET_CLS_ROUTE
@@ -242,10 +246,12 @@ #ifdef CONFIG_NET_CLS_ROUTE
 		return 0;
 #endif
 
-	if (tb[FRA_SRC] && (rule4->src != nla_get_be32(tb[FRA_SRC])))
+	if (frh->src_len && tb[FRA_SRC] &&
+	    (rule4->src != nla_get_be32(tb[FRA_SRC])))
 		return 0;
 
-	if (tb[FRA_DST] && (rule4->dst != nla_get_be32(tb[FRA_DST])))
+	if (frh->dst_len && tb[FRA_DST] &&
+	    (rule4->dst != nla_get_be32(tb[FRA_DST])))
 		return 0;
 
 	return 1;
diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index 0862809..6331306 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -131,8 +131,6 @@ static int fib6_rule_match(struct fib_ru
 
 static struct nla_policy fib6_rule_policy[FRA_MAX+1] __read_mostly = {
 	FRA_GENERIC_POLICY,
-	[FRA_SRC]	= { .len = sizeof(struct in6_addr) },
-	[FRA_DST]	= { .len = sizeof(struct in6_addr) },
 };
 
 static int fib6_rule_configure(struct fib_rule *rule, struct sk_buff *skb,
@@ -145,6 +143,14 @@ static int fib6_rule_configure(struct fi
 	if (frh->src_len > 128 || frh->dst_len > 128)
 		goto errout;
 
+	if (frh->src_len && tb[FRA_SRC] &&
+	    nla_len(tb[FRA_SRC]) != sizeof(struct in6_addr))
+		goto errout;
+
+	if (frh->dst_len && tb[FRA_DST] &&
+	    nla_len(tb[FRA_DST]) != sizeof(struct in6_addr))
+		goto errout;
+
 	if (rule->action == FR_ACT_TO_TBL) {
 		if (rule->table == RT6_TABLE_UNSPEC)
 			goto errout;
@@ -155,11 +161,11 @@ static int fib6_rule_configure(struct fi
 		}
 	}
 
-	if (tb[FRA_SRC])
+	if (frh->src_len && tb[FRA_SRC])
 		nla_memcpy(&rule6->src.addr, tb[FRA_SRC],
 			   sizeof(struct in6_addr));
 
-	if (tb[FRA_DST])
+	if (frh->dst_len && tb[FRA_DST])
 		nla_memcpy(&rule6->dst.addr, tb[FRA_DST],
 			   sizeof(struct in6_addr));
 
@@ -186,11 +192,11 @@ static int fib6_rule_compare(struct fib_
 	if (frh->tos && (rule6->tclass != frh->tos))
 		return 0;
 
-	if (tb[FRA_SRC] &&
+	if (frh->src_len && tb[FRA_SRC] &&
 	    nla_memcmp(tb[FRA_SRC], &rule6->src.addr, sizeof(struct in6_addr)))
 		return 0;
 
-	if (tb[FRA_DST] &&
+	if (frh->dst_len && tb[FRA_DST] &&
 	    nla_memcmp(tb[FRA_DST], &rule6->dst.addr, sizeof(struct in6_addr)))
 		return 0;
 

[NET]: Fix fib_rules compatibility breakage

[Thomas Graf]

Based on Patrick's patch:
The fib_rules netlink attribute policy introduced in 2.6.19 broke
userspace compatibilty. When specifying a rule with "from all"
or "to all", iproute adds a zero byte long netlink attribute,
but the policy requires all addresses to have a size equal to
sizeof(struct in_addr)/sizeof(struct in6_addr), resulting in a
validation error.

Check attribute length of FRA_SRC/FRA_DST in the generic framework
by letting the family specific rules implementation provide the
length of an address. Report an error if address length is non
zero but no address attribute is provided. Fix actual bug by
checking address length for non-zero instead of relying on
availability of attribute.

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

Index: net-2.6/include/net/fib_rules.h
===================================================================
--- net-2.6.orig/include/net/fib_rules.h	2007-03-20 15:38:19.000000000 +0100
+++ net-2.6/include/net/fib_rules.h	2007-03-20 16:01:31.000000000 +0100
@@ -34,6 +34,7 @@ struct fib_rules_ops
 	int			family;
 	struct list_head	list;
 	int			rule_size;
+	int			addr_size;
 
 	int			(*action)(struct fib_rule *,
 					  struct flowi *, int,
Index: net-2.6/net/core/fib_rules.c
===================================================================
--- net-2.6.orig/net/core/fib_rules.c	2007-03-20 15:37:39.000000000 +0100
+++ net-2.6/net/core/fib_rules.c	2007-03-20 15:56:59.000000000 +0100
@@ -173,6 +173,19 @@ int fib_nl_newrule(struct sk_buff *skb, 
 	if (err < 0)
 		goto errout;
 
+	err = -EINVAL;
+	if (frh->src_len)
+		if (tb[FRA_SRC] == NULL ||
+		    frh->src_len > (ops->addr_size * 8) ||
+		    nla_len(tb[FRA_SRC]) != ops->addr_size)
+			goto errout;
+
+	if (frh->dst_len)
+		if (tb[FRA_DST] == NULL ||
+		    frh->dst_len > (ops->addr_size * 8) ||
+		    nla_len(tb[FRA_DST]) != ops->addr_size)
+			goto errout;
+
 	rule = kzalloc(ops->rule_size, GFP_KERNEL);
 	if (rule == NULL) {
 		err = -ENOMEM;
Index: net-2.6/net/decnet/dn_rules.c
===================================================================
--- net-2.6.orig/net/decnet/dn_rules.c	2007-03-20 15:35:26.000000000 +0100
+++ net-2.6/net/decnet/dn_rules.c	2007-03-20 15:58:29.000000000 +0100
@@ -109,8 +109,6 @@ errout:
 
 static struct nla_policy dn_fib_rule_policy[FRA_MAX+1] __read_mostly = {
 	FRA_GENERIC_POLICY,
-	[FRA_SRC]	= { .type = NLA_U16 },
-	[FRA_DST]	= { .type = NLA_U16 },
 };
 
 static int dn_fib_rule_match(struct fib_rule *rule, struct flowi *fl, int flags)
@@ -133,7 +131,7 @@ static int dn_fib_rule_configure(struct 
 	int err = -EINVAL;
 	struct dn_fib_rule *r = (struct dn_fib_rule *)rule;
 
-	if (frh->src_len > 16 || frh->dst_len > 16 || frh->tos)
+	if (frh->tos)
 		goto  errout;
 
 	if (rule->table == RT_TABLE_UNSPEC) {
@@ -150,10 +148,10 @@ static int dn_fib_rule_configure(struct 
 		}
 	}
 
-	if (tb[FRA_SRC])
+	if (frh->src_len)
 		r->src = nla_get_le16(tb[FRA_SRC]);
 
-	if (tb[FRA_DST])
+	if (frh->dst_len)
 		r->dst = nla_get_le16(tb[FRA_DST]);
 
 	r->src_len = frh->src_len;
@@ -176,10 +174,10 @@ static int dn_fib_rule_compare(struct fi
 	if (frh->dst_len && (r->dst_len != frh->dst_len))
 		return 0;
 
-	if (tb[FRA_SRC] && (r->src != nla_get_le16(tb[FRA_SRC])))
+	if (frh->src_len && (r->src != nla_get_le16(tb[FRA_SRC])))
 		return 0;
 
-	if (tb[FRA_DST] && (r->dst != nla_get_le16(tb[FRA_DST])))
+	if (frh->dst_len && (r->dst != nla_get_le16(tb[FRA_DST])))
 		return 0;
 
 	return 1;
@@ -249,6 +247,7 @@ int dn_fib_dump_rules(struct sk_buff *sk
 static struct fib_rules_ops dn_fib_rules_ops = {
 	.family		= AF_DECnet,
 	.rule_size	= sizeof(struct dn_fib_rule),
+	.addr_size	= sizeof(u16),
 	.action		= dn_fib_rule_action,
 	.match		= dn_fib_rule_match,
 	.configure	= dn_fib_rule_configure,
Index: net-2.6/net/ipv4/fib_rules.c
===================================================================
--- net-2.6.orig/net/ipv4/fib_rules.c	2007-03-20 15:46:16.000000000 +0100
+++ net-2.6/net/ipv4/fib_rules.c	2007-03-20 15:55:08.000000000 +0100
@@ -171,8 +171,6 @@ static struct fib_table *fib_empty_table
 
 static struct nla_policy fib4_rule_policy[FRA_MAX+1] __read_mostly = {
 	FRA_GENERIC_POLICY,
-	[FRA_SRC]	= { .type = NLA_U32 },
-	[FRA_DST]	= { .type = NLA_U32 },
 	[FRA_FLOW]	= { .type = NLA_U32 },
 };
 
@@ -183,8 +181,7 @@ static int fib4_rule_configure(struct fi
 	int err = -EINVAL;
 	struct fib4_rule *rule4 = (struct fib4_rule *) rule;
 
-	if (frh->src_len > 32 || frh->dst_len > 32 ||
-	    (frh->tos & ~IPTOS_TOS_MASK))
+	if (frh->tos & ~IPTOS_TOS_MASK)
 		goto errout;
 
 	if (rule->table == RT_TABLE_UNSPEC) {
@@ -201,10 +198,10 @@ static int fib4_rule_configure(struct fi
 		}
 	}
 
-	if (tb[FRA_SRC])
+	if (frh->src_len)
 		rule4->src = nla_get_be32(tb[FRA_SRC]);
 
-	if (tb[FRA_DST])
+	if (frh->dst_len)
 		rule4->dst = nla_get_be32(tb[FRA_DST]);
 
 #ifdef CONFIG_NET_CLS_ROUTE
@@ -242,10 +239,10 @@ static int fib4_rule_compare(struct fib_
 		return 0;
 #endif
 
-	if (tb[FRA_SRC] && (rule4->src != nla_get_be32(tb[FRA_SRC])))
+	if (frh->src_len && (rule4->src != nla_get_be32(tb[FRA_SRC])))
 		return 0;
 
-	if (tb[FRA_DST] && (rule4->dst != nla_get_be32(tb[FRA_DST])))
+	if (frh->dst_len && (rule4->dst != nla_get_be32(tb[FRA_DST])))
 		return 0;
 
 	return 1;
@@ -309,6 +306,7 @@ static size_t fib4_rule_nlmsg_payload(st
 static struct fib_rules_ops fib4_rules_ops = {
 	.family		= AF_INET,
 	.rule_size	= sizeof(struct fib4_rule),
+	.addr_size	= sizeof(u32),
 	.action		= fib4_rule_action,
 	.match		= fib4_rule_match,
 	.configure	= fib4_rule_configure,
Index: net-2.6/net/ipv6/fib6_rules.c
===================================================================
--- net-2.6.orig/net/ipv6/fib6_rules.c	2007-03-20 15:48:50.000000000 +0100
+++ net-2.6/net/ipv6/fib6_rules.c	2007-03-20 15:57:44.000000000 +0100
@@ -131,8 +131,6 @@ static int fib6_rule_match(struct fib_ru
 
 static struct nla_policy fib6_rule_policy[FRA_MAX+1] __read_mostly = {
 	FRA_GENERIC_POLICY,
-	[FRA_SRC]	= { .len = sizeof(struct in6_addr) },
-	[FRA_DST]	= { .len = sizeof(struct in6_addr) },
 };
 
 static int fib6_rule_configure(struct fib_rule *rule, struct sk_buff *skb,
@@ -142,9 +140,6 @@ static int fib6_rule_configure(struct fi
 	int err = -EINVAL;
 	struct fib6_rule *rule6 = (struct fib6_rule *) rule;
 
-	if (frh->src_len > 128 || frh->dst_len > 128)
-		goto errout;
-
 	if (rule->action == FR_ACT_TO_TBL) {
 		if (rule->table == RT6_TABLE_UNSPEC)
 			goto errout;
@@ -155,11 +150,11 @@ static int fib6_rule_configure(struct fi
 		}
 	}
 
-	if (tb[FRA_SRC])
+	if (frh->src_len)
 		nla_memcpy(&rule6->src.addr, tb[FRA_SRC],
 			   sizeof(struct in6_addr));
 
-	if (tb[FRA_DST])
+	if (frh->dst_len)
 		nla_memcpy(&rule6->dst.addr, tb[FRA_DST],
 			   sizeof(struct in6_addr));
 
@@ -186,11 +181,11 @@ static int fib6_rule_compare(struct fib_
 	if (frh->tos && (rule6->tclass != frh->tos))
 		return 0;
 
-	if (tb[FRA_SRC] &&
+	if (frh->src_len &&
 	    nla_memcmp(tb[FRA_SRC], &rule6->src.addr, sizeof(struct in6_addr)))
 		return 0;
 
-	if (tb[FRA_DST] &&
+	if (frh->dst_len &&
 	    nla_memcmp(tb[FRA_DST], &rule6->dst.addr, sizeof(struct in6_addr)))
 		return 0;
 
@@ -240,6 +235,7 @@ static size_t fib6_rule_nlmsg_payload(st
 static struct fib_rules_ops fib6_rules_ops = {
 	.family			= AF_INET6,
 	.rule_size		= sizeof(struct fib6_rule),
+	.addr_size		= sizeof(struct in6_addr),
 	.action			= fib6_rule_action,
 	.match			= fib6_rule_match,
 	.configure		= fib6_rule_configure,

Re: [NET]: Fix fib_rules compatibility breakage

[Patrick McHardy]

Thomas Graf wrote:
> @@ -242,10 +239,10 @@ static int fib4_rule_compare(struct fib_
>  		return 0;
>  #endif
>  
> -	if (tb[FRA_SRC] && (rule4->src != nla_get_be32(tb[FRA_SRC])))
> +	if (frh->src_len && (rule4->src != nla_get_be32(tb[FRA_SRC])))
>  		return 0;
>  
> -	if (tb[FRA_DST] && (rule4->dst != nla_get_be32(tb[FRA_DST])))
> +	if (frh->dst_len && (rule4->dst != nla_get_be32(tb[FRA_DST])))
>  		return 0;
>  


The presence of the attributes when src_len/dst_len is non-zero
is only verified in fib_newrule, so this looks like it might crash
when something broken sets src_len/dst_len to a non-zero value
without actually adding the attributes.

Other than that it looks fine.

[NET]: Fix fib_rules compatibility breakage

[Thomas Graf]

* Patrick McHardy <kaber@trash.net> 2007-03-20 17:59
> The presence of the attributes when src_len/dst_len is non-zero
> is only verified in fib_newrule, so this looks like it might crash
> when something broken sets src_len/dst_len to a non-zero value
> without actually adding the attributes.

You're right, we need to validate in fib_nl_delrule() as well.

Based on Patrick's patch:
The fib_rules netlink attribute policy introduced in 2.6.19 broke
userspace compatibilty. When specifying a rule with "from all"
or "to all", iproute adds a zero byte long netlink attribute,
but the policy requires all addresses to have a size equal to
sizeof(struct in_addr)/sizeof(struct in6_addr), resulting in a
validation error.

Check attribute length of FRA_SRC/FRA_DST in the generic framework
by letting the family specific rules implementation provide the
length of an address. Report an error if address length is non
zero but no address attribute is provided. Fix actual bug by
checking address length for non-zero instead of relying on
availability of attribute.

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

Index: net-2.6/include/net/fib_rules.h
===================================================================
--- net-2.6.orig/include/net/fib_rules.h	2007-03-20 16:49:06.000000000 +0100
+++ net-2.6/include/net/fib_rules.h	2007-03-20 17:22:35.000000000 +0100
@@ -34,6 +34,7 @@ struct fib_rules_ops
 	int			family;
 	struct list_head	list;
 	int			rule_size;
+	int			addr_size;
 
 	int			(*action)(struct fib_rule *,
 					  struct flowi *, int,
Index: net-2.6/net/core/fib_rules.c
===================================================================
--- net-2.6.orig/net/core/fib_rules.c	2007-03-20 16:49:06.000000000 +0100
+++ net-2.6/net/core/fib_rules.c	2007-03-20 19:09:52.000000000 +0100
@@ -152,6 +152,28 @@ out:
 
 EXPORT_SYMBOL_GPL(fib_rules_lookup);
 
+static int validate_rulemsg(struct fib_rule_hdr *frh, struct nlattr **tb,
+			    struct fib_rules_ops *ops)
+{
+	int err = -EINVAL;
+
+	if (frh->src_len)
+		if (tb[FRA_SRC] == NULL ||
+		    frh->src_len > (ops->addr_size * 8) ||
+		    nla_len(tb[FRA_SRC]) != ops->addr_size)
+			goto errout;
+
+	if (frh->dst_len)
+		if (tb[FRA_DST] == NULL ||
+		    frh->dst_len > (ops->addr_size * 8) ||
+		    nla_len(tb[FRA_DST]) != ops->addr_size)
+			goto errout;
+
+	err = 0;
+errout:
+	return err;
+}
+
 int fib_nl_newrule(struct sk_buff *skb, struct nlmsghdr* nlh, void *arg)
 {
 	struct fib_rule_hdr *frh = nlmsg_data(nlh);
@@ -173,6 +195,10 @@ int fib_nl_newrule(struct sk_buff *skb, 
 	if (err < 0)
 		goto errout;
 
+	err = validate_rulemsg(frh, tb, ops);
+	if (err < 0)
+		goto errout;
+
 	rule = kzalloc(ops->rule_size, GFP_KERNEL);
 	if (rule == NULL) {
 		err = -ENOMEM;
@@ -260,6 +286,10 @@ int fib_nl_delrule(struct sk_buff *skb, 
 	if (err < 0)
 		goto errout;
 
+	err = validate_rulemsg(frh, tb, ops);
+	if (err < 0)
+		goto errout;
+
 	list_for_each_entry(rule, ops->rules_list, list) {
 		if (frh->action && (frh->action != rule->action))
 			continue;
Index: net-2.6/net/decnet/dn_rules.c
===================================================================
--- net-2.6.orig/net/decnet/dn_rules.c	2007-03-20 16:49:06.000000000 +0100
+++ net-2.6/net/decnet/dn_rules.c	2007-03-20 17:22:35.000000000 +0100
@@ -109,8 +109,6 @@ errout:
 
 static struct nla_policy dn_fib_rule_policy[FRA_MAX+1] __read_mostly = {
 	FRA_GENERIC_POLICY,
-	[FRA_SRC]	= { .type = NLA_U16 },
-	[FRA_DST]	= { .type = NLA_U16 },
 };
 
 static int dn_fib_rule_match(struct fib_rule *rule, struct flowi *fl, int flags)
@@ -133,7 +131,7 @@ static int dn_fib_rule_configure(struct 
 	int err = -EINVAL;
 	struct dn_fib_rule *r = (struct dn_fib_rule *)rule;
 
-	if (frh->src_len > 16 || frh->dst_len > 16 || frh->tos)
+	if (frh->tos)
 		goto  errout;
 
 	if (rule->table == RT_TABLE_UNSPEC) {
@@ -150,10 +148,10 @@ static int dn_fib_rule_configure(struct 
 		}
 	}
 
-	if (tb[FRA_SRC])
+	if (frh->src_len)
 		r->src = nla_get_le16(tb[FRA_SRC]);
 
-	if (tb[FRA_DST])
+	if (frh->dst_len)
 		r->dst = nla_get_le16(tb[FRA_DST]);
 
 	r->src_len = frh->src_len;
@@ -176,10 +174,10 @@ static int dn_fib_rule_compare(struct fi
 	if (frh->dst_len && (r->dst_len != frh->dst_len))
 		return 0;
 
-	if (tb[FRA_SRC] && (r->src != nla_get_le16(tb[FRA_SRC])))
+	if (frh->src_len && (r->src != nla_get_le16(tb[FRA_SRC])))
 		return 0;
 
-	if (tb[FRA_DST] && (r->dst != nla_get_le16(tb[FRA_DST])))
+	if (frh->dst_len && (r->dst != nla_get_le16(tb[FRA_DST])))
 		return 0;
 
 	return 1;
@@ -249,6 +247,7 @@ int dn_fib_dump_rules(struct sk_buff *sk
 static struct fib_rules_ops dn_fib_rules_ops = {
 	.family		= AF_DECnet,
 	.rule_size	= sizeof(struct dn_fib_rule),
+	.addr_size	= sizeof(u16),
 	.action		= dn_fib_rule_action,
 	.match		= dn_fib_rule_match,
 	.configure	= dn_fib_rule_configure,
Index: net-2.6/net/ipv4/fib_rules.c
===================================================================
--- net-2.6.orig/net/ipv4/fib_rules.c	2007-03-20 16:49:06.000000000 +0100
+++ net-2.6/net/ipv4/fib_rules.c	2007-03-20 17:22:35.000000000 +0100
@@ -171,8 +171,6 @@ static struct fib_table *fib_empty_table
 
 static struct nla_policy fib4_rule_policy[FRA_MAX+1] __read_mostly = {
 	FRA_GENERIC_POLICY,
-	[FRA_SRC]	= { .type = NLA_U32 },
-	[FRA_DST]	= { .type = NLA_U32 },
 	[FRA_FLOW]	= { .type = NLA_U32 },
 };
 
@@ -183,8 +181,7 @@ static int fib4_rule_configure(struct fi
 	int err = -EINVAL;
 	struct fib4_rule *rule4 = (struct fib4_rule *) rule;
 
-	if (frh->src_len > 32 || frh->dst_len > 32 ||
-	    (frh->tos & ~IPTOS_TOS_MASK))
+	if (frh->tos & ~IPTOS_TOS_MASK)
 		goto errout;
 
 	if (rule->table == RT_TABLE_UNSPEC) {
@@ -201,10 +198,10 @@ static int fib4_rule_configure(struct fi
 		}
 	}
 
-	if (tb[FRA_SRC])
+	if (frh->src_len)
 		rule4->src = nla_get_be32(tb[FRA_SRC]);
 
-	if (tb[FRA_DST])
+	if (frh->dst_len)
 		rule4->dst = nla_get_be32(tb[FRA_DST]);
 
 #ifdef CONFIG_NET_CLS_ROUTE
@@ -242,10 +239,10 @@ static int fib4_rule_compare(struct fib_
 		return 0;
 #endif
 
-	if (tb[FRA_SRC] && (rule4->src != nla_get_be32(tb[FRA_SRC])))
+	if (frh->src_len && (rule4->src != nla_get_be32(tb[FRA_SRC])))
 		return 0;
 
-	if (tb[FRA_DST] && (rule4->dst != nla_get_be32(tb[FRA_DST])))
+	if (frh->dst_len && (rule4->dst != nla_get_be32(tb[FRA_DST])))
 		return 0;
 
 	return 1;
@@ -309,6 +306,7 @@ static size_t fib4_rule_nlmsg_payload(st
 static struct fib_rules_ops fib4_rules_ops = {
 	.family		= AF_INET,
 	.rule_size	= sizeof(struct fib4_rule),
+	.addr_size	= sizeof(u32),
 	.action		= fib4_rule_action,
 	.match		= fib4_rule_match,
 	.configure	= fib4_rule_configure,
Index: net-2.6/net/ipv6/fib6_rules.c
===================================================================
--- net-2.6.orig/net/ipv6/fib6_rules.c	2007-03-20 16:49:06.000000000 +0100
+++ net-2.6/net/ipv6/fib6_rules.c	2007-03-20 17:22:35.000000000 +0100
@@ -131,8 +131,6 @@ static int fib6_rule_match(struct fib_ru
 
 static struct nla_policy fib6_rule_policy[FRA_MAX+1] __read_mostly = {
 	FRA_GENERIC_POLICY,
-	[FRA_SRC]	= { .len = sizeof(struct in6_addr) },
-	[FRA_DST]	= { .len = sizeof(struct in6_addr) },
 };
 
 static int fib6_rule_configure(struct fib_rule *rule, struct sk_buff *skb,
@@ -142,9 +140,6 @@ static int fib6_rule_configure(struct fi
 	int err = -EINVAL;
 	struct fib6_rule *rule6 = (struct fib6_rule *) rule;
 
-	if (frh->src_len > 128 || frh->dst_len > 128)
-		goto errout;
-
 	if (rule->action == FR_ACT_TO_TBL) {
 		if (rule->table == RT6_TABLE_UNSPEC)
 			goto errout;
@@ -155,11 +150,11 @@ static int fib6_rule_configure(struct fi
 		}
 	}
 
-	if (tb[FRA_SRC])
+	if (frh->src_len)
 		nla_memcpy(&rule6->src.addr, tb[FRA_SRC],
 			   sizeof(struct in6_addr));
 
-	if (tb[FRA_DST])
+	if (frh->dst_len)
 		nla_memcpy(&rule6->dst.addr, tb[FRA_DST],
 			   sizeof(struct in6_addr));
 
@@ -186,11 +181,11 @@ static int fib6_rule_compare(struct fib_
 	if (frh->tos && (rule6->tclass != frh->tos))
 		return 0;
 
-	if (tb[FRA_SRC] &&
+	if (frh->src_len &&
 	    nla_memcmp(tb[FRA_SRC], &rule6->src.addr, sizeof(struct in6_addr)))
 		return 0;
 
-	if (tb[FRA_DST] &&
+	if (frh->dst_len &&
 	    nla_memcmp(tb[FRA_DST], &rule6->dst.addr, sizeof(struct in6_addr)))
 		return 0;
 
@@ -240,6 +235,7 @@ static size_t fib6_rule_nlmsg_payload(st
 static struct fib_rules_ops fib6_rules_ops = {
 	.family			= AF_INET6,
 	.rule_size		= sizeof(struct fib6_rule),
+	.addr_size		= sizeof(struct in6_addr),
 	.action			= fib6_rule_action,
 	.match			= fib6_rule_match,
 	.configure		= fib6_rule_configure,

Re: [NET]: Fix fib_rules compatibility breakage

[Patrick McHardy]

Thomas Graf wrote:
> * Patrick McHardy <kaber@trash.net> 2007-03-20 17:59
> 
>>The presence of the attributes when src_len/dst_len is non-zero
>>is only verified in fib_newrule, so this looks like it might crash
>>when something broken sets src_len/dst_len to a non-zero value
>>without actually adding the attributes.
> 
> 
> You're right, we need to validate in fib_nl_delrule() as well.
> 
> Based on Patrick's patch:
> The fib_rules netlink attribute policy introduced in 2.6.19 broke
> userspace compatibilty. When specifying a rule with "from all"
> or "to all", iproute adds a zero byte long netlink attribute,
> but the policy requires all addresses to have a size equal to
> sizeof(struct in_addr)/sizeof(struct in6_addr), resulting in a
> validation error.
> 
> Check attribute length of FRA_SRC/FRA_DST in the generic framework
> by letting the family specific rules implementation provide the
> length of an address. Report an error if address length is non
> zero but no address attribute is provided. Fix actual bug by
> checking address length for non-zero instead of relying on
> availability of attribute.
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>

This looks good, thanks.

Signed-off-by: Patrick McHardy <kaber@trash.net>

Re: [NET]: Fix fib_rules compatibility breakage

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Tue, 20 Mar 2007 20:58:55 +0100

> Thomas Graf wrote:
> > * Patrick McHardy <kaber@trash.net> 2007-03-20 17:59
> > 
> >>The presence of the attributes when src_len/dst_len is non-zero
> >>is only verified in fib_newrule, so this looks like it might crash
> >>when something broken sets src_len/dst_len to a non-zero value
> >>without actually adding the attributes.
> > 
> > 
> > You're right, we need to validate in fib_nl_delrule() as well.
> > 
> > Based on Patrick's patch:
> > The fib_rules netlink attribute policy introduced in 2.6.19 broke
> > userspace compatibilty. When specifying a rule with "from all"
> > or "to all", iproute adds a zero byte long netlink attribute,
> > but the policy requires all addresses to have a size equal to
> > sizeof(struct in_addr)/sizeof(struct in6_addr), resulting in a
> > validation error.
> > 
> > Check attribute length of FRA_SRC/FRA_DST in the generic framework
> > by letting the family specific rules implementation provide the
> > length of an address. Report an error if address length is non
> > zero but no address attribute is provided. Fix actual bug by
> > checking address length for non-zero instead of relying on
> > availability of attribute.
> > 
> > Signed-off-by: Thomas Graf <tgraf@suug.ch>
> 
> This looks good, thanks.
> 
> Signed-off-by: Patrick McHardy <kaber@trash.net>

Applied, thanks guys, I'll push this to 2.6.20-stable as well.