[IPV6] Fix ICMPv6 redirect handling with target multicast address

[IPV6] Fix ICMPv6 redirect handling with target multicast address

[Brian Haley]

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

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 <brian.haley@hp.com>


[-- Attachment #2: ipv6.redirect.patch --]
[-- Type: text/x-patch, Size: 1313 bytes --]

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(0xFFC00000)) == htonl(0xFE800000));
+}
+
 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

[David Stevens]

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

Re: [IPV6] Fix ICMPv6 redirect handling with target multicast address

[YOSHIFUJI Hideaki / 吉藤英明]

Dave, Brian,

Let me double check this patch.

Regards,

--yoshfuji

In article <OF5FC97D70.5FD0A80A-ON88257365.00025E58-88257365.00048D1E@us.ibm.com> (at Fri, 28 Sep 2007 17:50:38 -0700), David Stevens <dlstevens@us.ibm.com> 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
> 
> 

Re: [IPV6] Fix ICMPv6 redirect handling with target multicast address

[Brian Haley]

Hi Yoshifuji,

YOSHIFUJI Hideaki / ???? wrote:
> Dave, Brian,
> 
> Let me double check this patch.
> 
> Regards,
> 
> --yoshfuji
> 
> In article <OF5FC97D70.5FD0A80A-ON88257365.00025E58-88257365.00048D1E@us.ibm.com> (at Fri, 28 Sep 2007 17:50:38 -0700), David Stevens <dlstevens@us.ibm.com> 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 :-)),

TAHI generates a lot of packets that shouldn't happen :) see below for 
the problem.  The patch in ndisc_send_redirect() is probably 
unnecessary, but since the code was identical I figured it wouldn't hurt.

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

I did have it:

if (ipv6_addr_type(target) &
     (IPV6_ADDR_LINKLOCAL|IPV6_ADDR_UNICAST) ==
     (IPV6_ADDR_LINKLOCAL|IPV6_ADDR_UNICAST))

but changed it since I had proposed ipv6_addr_linklocal() in a previous 
patch because there are other possible users of that.

This is the actual packet trace that was sent to me, edited to remove 
non-relevant data, but I've actually been unable to reproduce Packet 12 
below (note the Destination MAC).  My knee-jerk reaction was the 
proposed patch.  It could just be timing-related, I'll do more testing 
Monday.  Strangely, the TAHI test passes anyway, but it shouldn't.

Packet 10 : Icmpv6 : Redirect
Ethernet
Destination MAC.........................00:1A:4B:EB:FC:E6
Source MAC..............................00:00:10:10:10:60
Ipv6
Version.................................6
Next Header.............................58
Hop Limit...............................255
Source IP.....................FE80:0000:0000:0000:0200:10FF:FE10:1060
Destination IP................3000:0000:0000:0000:021A:4BFF:FEEB:FCE6
Icmpv6
Type....................................0x89
Code....................................0x00
Reserved................................0x00000000
Target Address................FF02:0000:0000:0000:0000:0000:0000:0001
Destination Address...........3001:0000:0000:0000:0200:10FF:FE10:1180
Options
LLAddressOption
   Option Type.............................2
   Option Length...........................1
   Link-Layer Address......................0x000010101061

Packet 11 : Icmpv6 : Echo Request
Ethernet
Destination MAC.........................00:1A:4B:EB:FC:E6
Source MAC..............................00:00:10:10:10:60
Ipv6
Version.................................6
Next Header.............................58
Hop Limit...............................255
Source IP.....................3001:0000:0000:0000:0200:10FF:FE10:1180
Destination IP................3000:0000:0000:0000:021A:4BFF:FEEB:FCE6
Icmpv6
Type....................................0x80
Code....................................0x00
Identifier..............................0x0000
Sequence Number.........................0x0000
Data....................................0x00

Packet 12 : Icmpv6 : Echo Reply
Ethernet
Destination MAC.........................33:33:00:00:00:01
Source MAC..............................00:1A:4B:EB:FC:E6
Ipv6
Version.................................6
Next Header.............................58
Hop Limit...............................64
Source IP.....................3000:0000:0000:0000:021A:4BFF:FEEB:FCE6
Destination IP................3001:0000:0000:0000:0200:10FF:FE10:1180
Icmpv6
Type....................................0x81
Code....................................0x00
Identifier..............................0x0000
Sequence Number.........................0x0000
Data....................................0x00

This should have been sent to 00:00:10:10:10:60

-Brian

Re: [IPV6] Fix ICMPv6 redirect handling with target multicast address

[YOSHIFUJI Hideaki / 吉藤英明]

Hello.

In article <20070929.100448.41933886.yoshfuji@linux-ipv6.org> (at Sat, 29 Sep 2007 10:04:48 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@linux-ipv6.org> says:

> In article <OF5FC97D70.5FD0A80A-ON88257365.00025E58-88257365.00048D1E@us.ibm.com> (at Fri, 28 Sep 2007 17:50:38 -0700), David Stevens <dlstevens@us.ibm.com> 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

Re: [IPV6] Fix ICMPv6 redirect handling with target multicast address

[Brian Haley]

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

[IPv6] Fix ICMPv6 redirect handling with target multicast address

[Brian Haley]

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

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 <brian.haley@hp.com>


[-- Attachment #2: ipv6.redirect.patch --]
[-- Type: text/x-patch, Size: 897 bytes --]

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

[David Stevens]

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

Re: [IPv6] Fix ICMPv6 redirect handling with target multicast address

[Brian Haley]

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

Re: [IPv6] Fix ICMPv6 redirect handling with target multicast address

[David Stevens]

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 <dlstevens@us.ibm.com>

FWIW. :-)

                                                +-DLS

[IPv6] Fix ICMPv6 redirect handling with target multicast address, try 3

[Brian Haley]

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

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 <brian.haley@hp.com>
Acked-by: David L Stevens <dlstevens@us.ibm.com>

[-- Attachment #2: ipv6.redirect.patch --]
[-- Type: text/x-patch, Size: 1051 bytes --]

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

Re: [IPv6] Fix ICMPv6 redirect handling with target multicast address, try 3

[David Miller]

From: Brian Haley <brian.haley@hp.com>
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 <brian.haley@hp.com>
> Acked-by: David L Stevens <dlstevens@us.ibm.com>

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!