Re: [IPv6] Fix ICMPv6 redirect handling with target multicast address, try 3
From: Brian Haley [EMAIL PROTECTED]
Date: Wed, 03 Oct 2007 10:44:17 -0400
When the ICMPv6 Target address is multicast, Linux processes the
redirect instead of dropping it. The problem is in this code in
ndisc_redirect_rcv():
if (ipv6_addr_equal(dest, target)) {
on_link = 1;
} else if (!(ipv6_addr_type(target) IPV6_ADDR_LINKLOCAL)) {
ND_PRINTK2(KERN_WARNING
ICMPv6 Redirect: target address is not
link-local.\n);
return;
}
This second check will succeed if the Target address is, for example,
FF02::1 because it has link-local scope. Instead, it should be checking
if it's a unicast link-local address, as stated in RFC 2461/4861 Section
8.1:
- The ICMP Target Address is either a link-local address (when
redirected to a router) or the same as the ICMP Destination
Address (when redirected to the on-link destination).
I know this doesn't explicitly say unicast link-local address, but it's
implied.
This bug is preventing Linux kernels from achieving IPv6 Logo Phase II
certification because of a recent error that was found in the TAHI test
suite - Neighbor Disovery suite test 206 (v6LC.2.3.6_G) had the
multicast address in the Destination field instead of Target field, so
we were passing the test. This won't be the case anymore.
The patch below fixes this problem, and also fixes ndisc_send_redirect()
to not send an invalid redirect with a multicast address in the Target
field. I re-ran the TAHI Neighbor Discovery section to make sure Linux
passes all 245 tests now.
Signed-off-by: Brian Haley [EMAIL PROTECTED]
Acked-by: David L Stevens [EMAIL PROTECTED]
I believe everyone's concerns have been addressed in this
version of the patch, so I have applied it to net-2.6
Thanks everyone!
-
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
[IPv6] Fix ICMPv6 redirect handling with target multicast address, try 3
When the ICMPv6 Target address is multicast, Linux processes the
redirect instead of dropping it. The problem is in this code in
ndisc_redirect_rcv():
if (ipv6_addr_equal(dest, target)) {
on_link = 1;
} else if (!(ipv6_addr_type(target) IPV6_ADDR_LINKLOCAL)) {
ND_PRINTK2(KERN_WARNING
ICMPv6 Redirect: target address is not
link-local.\n);
return;
}
This second check will succeed if the Target address is, for example,
FF02::1 because it has link-local scope. Instead, it should be checking
if it's a unicast link-local address, as stated in RFC 2461/4861 Section
8.1:
- The ICMP Target Address is either a link-local address (when
redirected to a router) or the same as the ICMP Destination
Address (when redirected to the on-link destination).
I know this doesn't explicitly say unicast link-local address, but it's
implied.
This bug is preventing Linux kernels from achieving IPv6 Logo Phase II
certification because of a recent error that was found in the TAHI test
suite - Neighbor Disovery suite test 206 (v6LC.2.3.6_G) had the
multicast address in the Destination field instead of Target field, so
we were passing the test. This won't be the case anymore.
The patch below fixes this problem, and also fixes ndisc_send_redirect()
to not send an invalid redirect with a multicast address in the Target
field. I re-ran the TAHI Neighbor Discovery section to make sure Linux
passes all 245 tests now.
-Brian
Signed-off-by: Brian Haley [EMAIL PROTECTED]
Acked-by: David L Stevens [EMAIL PROTECTED]
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 74c4d8d..b761dbe 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1267,9 +1267,10 @@ static void ndisc_redirect_rcv(struct sk_buff *skb)
if (ipv6_addr_equal(dest, target)) {
on_link = 1;
- } else if (!(ipv6_addr_type(target) IPV6_ADDR_LINKLOCAL)) {
+ } else if (ipv6_addr_type(target) !=
+ (IPV6_ADDR_UNICAST|IPV6_ADDR_LINKLOCAL)) {
ND_PRINTK2(KERN_WARNING
- ICMPv6 Redirect: target address is not link-local.\n);
+ ICMPv6 Redirect: target address is not link-local unicast.\n);
return;
}
@@ -1343,9 +1344,9 @@ void ndisc_send_redirect(struct sk_buff *skb, struct neighbour *neigh,
}
if (!ipv6_addr_equal(ipv6_hdr(skb)-daddr, target)
- !(ipv6_addr_type(target) IPV6_ADDR_LINKLOCAL)) {
+ ipv6_addr_type(target) != (IPV6_ADDR_UNICAST|IPV6_ADDR_LINKLOCAL)) {
ND_PRINTK2(KERN_WARNING
- ICMPv6 Redirect: target address is not link-local.\n);
+ ICMPv6 Redirect: target address is not link-local unicast.\n);
return;
}
[IPv6] Fix ICMPv6 redirect handling with target multicast address
When the ICMPv6 Target address is multicast, Linux processes the
redirect instead of dropping it. The problem is in this code in
ndisc_redirect_rcv():
if (ipv6_addr_equal(dest, target)) {
on_link = 1;
} else if (!(ipv6_addr_type(target) IPV6_ADDR_LINKLOCAL)) {
ND_PRINTK2(KERN_WARNING
ICMPv6 Redirect: target address is not
link-local.\n);
return;
}
This second check will succeed if the Target address is, for example,
FF02::1 because it has link-local scope. Instead, it should be checking
if it's a unicast link-local address, as stated in RFC 2461/4861 Section
8.1:
- The ICMP Target Address is either a link-local address (when
redirected to a router) or the same as the ICMP Destination
Address (when redirected to the on-link destination).
I know this doesn't explicitly say unicast link-local address, but it's
implied.
This bug is preventing Linux kernels from achieving IPv6 Logo Phase II
certification because of a recent error that was found in the TAHI test
suite - Neighbor Disovery suite test 206 (v6LC.2.3.6_G) had the
multicast address in the Destination field instead of Target field, so
we were passing the test. This won't be the case anymore.
The patch below fixes this problem, and also fixes ndisc_send_redirect()
to not send an invalid redirect with a multicast address in the Target
field. I re-ran the TAHI Neighbor Discovery section to make sure Linux
passes all 245 tests now.
-Brian
Signed-off-by: Brian Haley [EMAIL PROTECTED]
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 74c4d8d..a0a6406 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1267,7 +1267,8 @@ static void ndisc_redirect_rcv(struct sk_buff *skb)
if (ipv6_addr_equal(dest, target)) {
on_link = 1;
- } else if (!(ipv6_addr_type(target) IPV6_ADDR_LINKLOCAL)) {
+ } else if (ipv6_addr_type(target) !=
+ (IPV6_ADDR_UNICAST|IPV6_ADDR_LINKLOCAL)) {
ND_PRINTK2(KERN_WARNING
ICMPv6 Redirect: target address is not link-local.\n);
return;
@@ -1343,7 +1344,7 @@ void ndisc_send_redirect(struct sk_buff *skb, struct neighbour *neigh,
}
if (!ipv6_addr_equal(ipv6_hdr(skb)-daddr, target)
- !(ipv6_addr_type(target) IPV6_ADDR_LINKLOCAL)) {
+ ipv6_addr_type(target) != (IPV6_ADDR_UNICAST|IPV6_ADDR_LINKLOCAL)) {
ND_PRINTK2(KERN_WARNING
ICMPv6 Redirect: target address is not link-local.\n);
return;
Re: [IPv6] Fix ICMPv6 redirect handling with target multicast address
Brian, ipv6_addr_type() returns a mask, so checking for equality will fail to match if any other (irrelevant) attributes are set. How about using bitwise operators for that? Also, the error message is no longer descriptive of the failure if it's a link-local multicast, but you could make it target address is not link-local unicast.\n (in both places). +-DLS - 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
Re: [IPv6] Fix ICMPv6 redirect handling with target multicast address
Hi David, David Stevens wrote: ipv6_addr_type() returns a mask, so checking for equality will fail to match if any other (irrelevant) attributes are set. How about using bitwise operators for that? ipv6_addr_type() does return a mask, but there's a lot of code that just checks for equality since some things are mutually-exclusive - this code is actually identical to what ip6_route_add() does. I don't particularly like this duality, but it's there - I'd gladly volunteer to clean this up everywhere if I didn't think there might be some performance reason it was done like that. Also, the error message is no longer descriptive of the failure if it's a link-local multicast, but you could make it target address is not link-local unicast.\n (in both places). I can do that, thanks. -Brian - 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
Re: [IPv6] Fix ICMPv6 redirect handling with target multicast address
Brian, I don't think a few instructions is a performance issue in the redirect paths (it'd be pretty broken if you're getting or generating lots of them), but I know there are lots of other checks similar to that that will break with new attributes, so doing that as a general clean-up separately is ok with me, too. With the error message changes, you can add: Acked-by: David L Stevens [EMAIL PROTECTED] FWIW. :-) +-DLS - 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
Re: [IPV6] Fix ICMPv6 redirect handling with target multicast address
Hello. In article [EMAIL PROTECTED] (at Sat, 29 Sep 2007 10:04:48 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 [EMAIL PROTECTED] says: In article [EMAIL PROTECTED] (at Fri, 28 Sep 2007 17:50:38 -0700), David Stevens [EMAIL PROTECTED] says: Brian, A multicast address should never be the target of a neighbor discovery request; the sender should use the mapping function for all multicasts. So, I'm not sure that your example can ever happen, and it certainly is ok to send ICMPv6 errors to multicast addresses in general. But I don't see that it hurts anything. either (since it should never happen :-)), so I don't particularly object, either. I think it'd also be better if you add the check to be: if (ipv6_addr_type(target) (IPV6_ADDR_LINKLOCAL|IPV6_ADDR_UNICAST)) or something along those lines, rather than reproducing ipv6_addr_type() code separately in a new ipv6_addr_linklocal() function. I'm fine with the idea of the fix itself. Please use ipv6_addr_type() so far and convert other users as well to ipv6_addr_linklocal() in another patch. Regards, --yoshfuji - 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
Re: [IPV6] Fix ICMPv6 redirect handling with target multicast address
Hi, YOSHIFUJI Hideaki / 吉藤英明 wrote: I think it'd also be better if you add the check to be: if (ipv6_addr_type(target) (IPV6_ADDR_LINKLOCAL|IPV6_ADDR_UNICAST)) or something along those lines, rather than reproducing ipv6_addr_type() code separately in a new ipv6_addr_linklocal() function. I'm fine with the idea of the fix itself. Ok, in both the receive and send code? Please use ipv6_addr_type() so far and convert other users as well to ipv6_addr_linklocal() in another patch. I'll re-do the patch. -Brian - 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
[IPV6] Fix ICMPv6 redirect handling with target multicast address
When the ICMPv6 Target address is multicast, Linux processes the
redirect instead of dropping it. The problem is in this code in
ndisc_redirect_rcv():
if (ipv6_addr_equal(dest, target)) {
on_link = 1;
} else if (!(ipv6_addr_type(target) IPV6_ADDR_LINKLOCAL)) {
ND_PRINTK2(KERN_WARNING
ICMPv6 Redirect: target address is not
link-local.\n);
return;
}
This second check will succeed if the Target address is, for example,
FF02::1 because it has link-local scope. Instead, it should be checking
if it's a unicast link-local address, as stated in RFC 2461/4861 Section
8.1:
- The ICMP Target Address is either a link-local address (when
redirected to a router) or the same as the ICMP Destination
Address (when redirected to the on-link destination).
I know this doesn't explicitly say unicast link-local address, but it's
implied.
This bug is preventing Linux kernels from achieving IPv6 Logo Phase II
certification because of a recent error that was found in the TAHI test
suite - Neighbor Disovery suite test 206 (v6LC.2.3.6_G) had the
multicast address in the Destination field instead of Target field, so
we were passing the test. This won't be the case anymore.
The patch below fixes this problem, and also fixes ndisc_send_redirect()
to not send an invalid redirect with a multicast address in the Target
field. I re-ran the TAHI Neighbor Discovery section to make sure Linux
passes all 245 tests now.
-Brian
Signed-off-by: Brian Haley [EMAIL PROTECTED]
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 31b3f1b..4f47d29 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -368,6 +368,11 @@ static inline int ipv6_prefix_equal(const struct in6_addr *a1,
prefixlen);
}
+static inline int ipv6_addr_linklocal(const struct in6_addr *a)
+{
+ return ((a-s6_addr32[0] htonl(0xFFC0)) == htonl(0xFE80));
+}
+
static inline int ipv6_addr_any(const struct in6_addr *a)
{
return ((a-s6_addr32[0] | a-s6_addr32[1] |
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 74c4d8d..8f953a7 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1267,7 +1267,7 @@ static void ndisc_redirect_rcv(struct sk_buff *skb)
if (ipv6_addr_equal(dest, target)) {
on_link = 1;
- } else if (!(ipv6_addr_type(target) IPV6_ADDR_LINKLOCAL)) {
+ } else if (!ipv6_addr_linklocal(target)) {
ND_PRINTK2(KERN_WARNING
ICMPv6 Redirect: target address is not link-local.\n);
return;
@@ -1343,7 +1343,7 @@ void ndisc_send_redirect(struct sk_buff *skb, struct neighbour *neigh,
}
if (!ipv6_addr_equal(ipv6_hdr(skb)-daddr, target)
- !(ipv6_addr_type(target) IPV6_ADDR_LINKLOCAL)) {
+ !ipv6_addr_linklocal(target)) {
ND_PRINTK2(KERN_WARNING
ICMPv6 Redirect: target address is not link-local.\n);
return;
Re: [IPV6] Fix ICMPv6 redirect handling with target multicast address
Brian, A multicast address should never be the target of a neighbor discovery request; the sender should use the mapping function for all multicasts. So, I'm not sure that your example can ever happen, and it certainly is ok to send ICMPv6 errors to multicast addresses in general. But I don't see that it hurts anything. either (since it should never happen :-)), so I don't particularly object, either. I think it'd also be better if you add the check to be: if (ipv6_addr_type(target) (IPV6_ADDR_LINKLOCAL|IPV6_ADDR_UNICAST)) or something along those lines, rather than reproducing ipv6_addr_type() code separately in a new ipv6_addr_linklocal() function. +-DLS - 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
Re: [IPV6] Fix ICMPv6 redirect handling with target multicast address
Dave, Brian, Let me double check this patch. Regards, --yoshfuji In article [EMAIL PROTECTED] (at Fri, 28 Sep 2007 17:50:38 -0700), David Stevens [EMAIL PROTECTED] says: Brian, A multicast address should never be the target of a neighbor discovery request; the sender should use the mapping function for all multicasts. So, I'm not sure that your example can ever happen, and it certainly is ok to send ICMPv6 errors to multicast addresses in general. But I don't see that it hurts anything. either (since it should never happen :-)), so I don't particularly object, either. I think it'd also be better if you add the check to be: if (ipv6_addr_type(target) (IPV6_ADDR_LINKLOCAL|IPV6_ADDR_UNICAST)) or something along those lines, rather than reproducing ipv6_addr_type() code separately in a new ipv6_addr_linklocal() function. +-DLS - 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
