minimum ICMPv6 message size vs. RPL's DIS

PATCH Werner Almesberger 3 participants 12 messages Jul 25, 2013 – Aug 2, 2013

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Werner Almesberger

author

2013-07-25 00:17 UTC #0 lore raw

The kernel assumes that any valid ICMPv6 message is at least eight bytes long, for example in net/ipv6/raw.c:icmpv6_filter

The DIS message of RPL (RFC 6550 section 6.2, from the 6LoWPAN world), has a minimum length of six bytes, and is thus blocked by icmpv6_filter.

Contiki OS sends this sort of messages and is thus ignored by an user-space RPL application on Linux (in my case simpleRPL). Nodes will eventually join the network regardless, but this can take hours instead of seconds.

RFC 4443 seems to allow even a zero-sized body, making the minimum allowable size four bytes.

A similar restriction exists in net/ipv6/icmp.c:icmpv6_rcv but causes no harm there since all potential recipients at that point expect a larger payload.

Now, while this is easy enough to fix on the Linux side (see below), I wonder if it is really a Linux bug or if I (and the authors of RPL in Contiki) may have missed some minimum size requirement for ICMPv6 messages stated elsewhere.

Opinions ?

- Werner

net/ipv6/raw.c +1 -1

index c45f7a5..539534c 100644
@@ -112,7 +112,7 @@ static int icmpv6_filter(const struct sock *sk, const struct sk_buff *skb)
 	const struct icmp6hdr *hdr;
 
 	hdr = skb_header_pointer(skb, skb_transport_offset(skb),
-				 sizeof(_hdr), &_hdr);
+				 4, &_hdr);
 	if (hdr) {
 		const __u32 *data = &raw6_sk(sk)->filter.data[0];
 		unsigned int type = hdr->icmp6_type;

Hannes Frederic Sowa

2013-07-25 06:17 UTC #1 lore raw

Quoted 19 lines · Werner Almesberger

The kernel assumes that any valid ICMPv6 message is at least eight bytes long, for example in net/ipv6/raw.c:icmpv6_filter
The DIS message of RPL (RFC 6550 section 6.2, from the 6LoWPAN world), has a minimum length of six bytes, and is thus blocked by icmpv6_filter.
Contiki OS sends this sort of messages and is thus ignored by an user-space RPL application on Linux (in my case simpleRPL). Nodes will eventually join the network regardless, but this can take hours instead of seconds.
RFC 4443 seems to allow even a zero-sized body, making the minimum allowable size four bytes.
A similar restriction exists in net/ipv6/icmp.c:icmpv6_rcv but causes no harm there since all potential recipients at that point expect a larger payload.

Quoted 6 lines

Now, while this is easy enough to fix on the Linux side (see below), I wonder if it is really a Linux bug or if I (and the authors of RPL in Contiki) may have missed some minimum size requirement for ICMPv6 messages stated elsewhere.
Opinions ?

Browsing the RFCs, I do think you are correct. Did you check what IPv4 does? (what a strange errata to RFC 4443...)

net/ipv6/raw.c static int icmpv6_filter(const struct sock *sk, const struct sk_buff *skb) · line 118

Quoted 13 lines

net/ipv6/raw.c +1 -1

index c45f7a5..539534c 100644
@@ -112,7 +112,7 @@ static int icmpv6_filter(const struct sock *sk, const struct sk_buff *skb)
 	const struct icmp6hdr *hdr;
 
 	hdr = skb_header_pointer(skb, skb_transport_offset(skb),
-				 sizeof(_hdr), &_hdr);
+				 4, &_hdr);
 	if (hdr) {
 		const __u32 *data = &raw6_sk(sk)->filter.data[0];
 		unsigned int type = hdr->icmp6_type;

Hmm, maybe we should update the icmp header to something like

struct icmp6hdr {
	struct icmp6hdr_head {
		__u8 icmp6_type;
		...;
	} icmpv6_head;

#define icmp6_type icmpv6_head.icmp6_type

and just change the type in icmpv6_filter to icmp6hdr_head.

Hmm, there is a bug in this function, _hdr must not be a pointer.

Greetings,

Hannes

Werner Almesberger

author

2013-07-25 10:32 UTC #2 lore raw

Quoted 1 line · Hannes Frederic Sowa

Hmm, maybe we should update the icmp header to something like

That would be quite clean. Is it okay to introduce new names like that in a uapi/ header (uapi/linux/icmpv6.h) ?

Quoted 1 line

Hmm, there is a bug in this function, _hdr must not be a pointer.

Oh, I didn't even notice that. Very good catch !

So on 32 bit system, it would actually work even with "short" ICMPv6 messages. Two wrongs sometimes do make a right :-)

I've attached a revised patch that, according to quick testing, still works and doesn't break anything else.

Thanks,
- Werner

---------------------------------- cut here -----------------------------------

include/uapi/linux/icmpv6.h +9 -5

index e0133c7..11eb5ff 100644
@@ -5,11 +5,15 @@
 #include <asm/byteorder.h>
 
 struct icmp6hdr {
-
-	__u8		icmp6_type;
-	__u8		icmp6_code;
-	__sum16		icmp6_cksum;
-
+	struct icmp6hdr_head {
+		__u8		type;
+		__u8		code;
+		__sum16		cksum;
+	} icmpv6_head;
+
+#define	icmp6_type	icmpv6_head.type
+#define	icmp6_code	icmpv6_head.code
+#define	icmp6_cksum	icmpv6_head.cksum
 
 	union {
 		__be32			un_data32[1];

net/ipv6/raw.c +6 -6

index c45f7a5..99ab06f 100644
@@ -108,14 +108,14 @@ found:
  */
 static int icmpv6_filter(const struct sock *sk, const struct sk_buff *skb)
 {
-	struct icmp6hdr *_hdr;
-	const struct icmp6hdr *hdr;
+	struct icmp6hdr_head _head;
+	const struct icmp6hdr_head *head;
 
-	hdr = skb_header_pointer(skb, skb_transport_offset(skb),
-				 sizeof(_hdr), &_hdr);
-	if (hdr) {
+	head = skb_header_pointer(skb, skb_transport_offset(skb),
+				  sizeof(_head), &_head);
+	if (head) {
 		const __u32 *data = &raw6_sk(sk)->filter.data[0];
-		unsigned int type = hdr->icmp6_type;
+		unsigned int type = head->type;
 
 		return (data[type >> 5] & (1U << (type & 31))) != 0;
 	}

Hannes Frederic Sowa

2013-07-25 13:03 UTC #3 lore raw

Quoted 26 lines · Werner Almesberger

Quoted 1 line · Hannes Frederic Sowa
Hmm, maybe we should update the icmp header to something like
That would be quite clean. Is it okay to introduce new names like that in a uapi/ header (uapi/linux/icmpv6.h) ?
Quoted 1 line
Hmm, there is a bug in this function, _hdr must not be a pointer.
Oh, I didn't even notice that. Very good catch !
So on 32 bit system, it would actually work even with "short" ICMPv6 messages. Two wrongs sometimes do make a right :-)
I've attached a revised patch that, according to quick testing, still works and doesn't break anything else.
---------------------------------- cut here -----------------------------------
include/uapi/linux/icmpv6.h +0 -0
index e0133c7..11eb5ff 100644
@@ -5,11 +5,15 @@
 #include <asm/byteorder.h>
 
 struct icmp6hdr {

include/uapi/linux/icmpv6.h line 12

Quoted 10 lines

include/uapi/linux/icmpv6.h +5 -5

-
-	__u8		icmp6_type;
-	__u8		icmp6_code;
-	__sum16		icmp6_cksum;
-
+	struct icmp6hdr_head {
+		__u8		type;
+		__u8		code;
+		__sum16		cksum;
+	} icmpv6_head;

Hm, could you drop the 'v' (we want to stay in the naming convention; I know I introduced it).

Quoted 22 lines

include/uapi/linux/icmpv6.h +4 -0

+
+#define	icmp6_type	icmpv6_head.type
+#define	icmp6_code	icmpv6_head.code
+#define	icmp6_cksum	icmpv6_head.cksum
 
 	union {
 		__be32			un_data32[1];

net/ipv6/raw.c +2 -4

index c45f7a5..99ab06f 100644
@@ -108,14 +108,14 @@ found:
  */
 static int icmpv6_filter(const struct sock *sk, const struct sk_buff *skb)
 {
-	struct icmp6hdr *_hdr;
-	const struct icmp6hdr *hdr;
+	struct icmp6hdr_head _head;
+	const struct icmp6hdr_head *head;
 
-	hdr = skb_header_pointer(skb, skb_transport_offset(skb),
-				 sizeof(_hdr), &_hdr);

net/ipv6/raw.c found: · line 121

Quoted 10 lines

net/ipv6/raw.c +4 -2

-	if (hdr) {
+	head = skb_header_pointer(skb, skb_transport_offset(skb),
+				  sizeof(_head), &_head);
+	if (head) {
 		const __u32 *data = &raw6_sk(sk)->filter.data[0];
-		unsigned int type = hdr->icmp6_type;
+		unsigned int type = head->type;
 
 		return (data[type >> 5] & (1U << (type & 31))) != 0;
 	}

Looks fine, could you do a proper patch submission? (Most simple way, do a git commit, describe your changes, git format-patch HEAD^ and check it with checkpatch --strict). Details are in the Documentation/ directory, especially Submit*. I will do a proper review later, then.

Actually, it would be best to split the pointer error in a seperate patch (has to be the first one). It may be a candidate for stable.

Thanks,

Hannes

Hannes Frederic Sowa

2013-07-25 13:58 UTC #4 lore raw

In reply to Werner Almesberger

Quoted 5 lines · Werner Almesberger

Quoted 1 line · Hannes Frederic Sowa
Hmm, maybe we should update the icmp header to something like
That would be quite clean. Is it okay to introduce new names like that in a uapi/ header (uapi/linux/icmpv6.h) ?

I would say no problem. But as I just realized that it could be a bit problematic because the new defines have actually pretty common names, let's cc David Miller. Perhaps he has an advice?

Greetings,

Hannes

Werner Almesberger

author

2013-07-25 14:33 UTC #5 lore raw

Quoted 3 lines · Hannes Frederic Sowa

I would say no problem. But as I just realized that it could be a bit problematic because the new defines have actually pretty common names, let's cc David Miller. Perhaps he has an advice?

Yeah, I'll let the issue sit here for a while so that more people can comment. The change has numerous implications, including

- there may actually be a minimum size requirement *somewhere* and I just didn't find it, in which case Linux would be right,

- name pollution visible to future user space,

- subtly changes kernel API semantics for ICMPv6 receivers, to the detriment of those that rely on the kernel to filter messages < 8 bytes and misbehave if exposed to them.

So this is definitely not the kind of change I want to rush.

Even the pointer fix changes the API in a way that could break applications that currently work (if only on 32 bit platforms, but it's not their fault that things would go wrong on 64 bit), so we can't apply that one before having a decision on the other issue as well.

Thanks,
- Werner

Hannes Frederic Sowa

2013-07-25 18:40 UTC #6 lore raw

Quoted 10 lines · Werner Almesberger

Quoted 3 lines · Hannes Frederic Sowa
I would say no problem. But as I just realized that it could be a bit problematic because the new defines have actually pretty common names, let's cc David Miller. Perhaps he has an advice?
Yeah, I'll let the issue sit here for a while so that more people can comment. The change has numerous implications, including
- there may actually be a minimum size requirement *somewhere* and I just didn't find it, in which case Linux would be right,

I don't know how they could do this if they want to let other RFCs extend icmp types. It definitely makes sense that an icmpv6 packet could not have any payload (only for informational icmpv6 packets, error icmp msgs must have 32 bits of payload, see Apendix A - RFC4443).

Quoted 1 line

- name pollution visible to future user space,

I did a search on codesearch.debian.org and the change seems safe from a first glimpse.

Quoted 3 lines

- subtly changes kernel API semantics for ICMPv6 receivers, to the detriment of those that rely on the kernel to filter messages < 8 bytes and misbehave if exposed to them.

Yes, that could be an issue. I would be willing to accept this fallout. :)

Quoted 7 lines

So this is definitely not the kind of change I want to rush.
Even the pointer fix changes the API in a way that could break applications that currently work (if only on 32 bit platforms, but it's not their fault that things would go wrong on 64 bit), so we can't apply that one before having a decision on the other issue as well.

Yes, you are right!

Thanks,

Hannes

Werner Almesberger

author

2013-07-25 21:49 UTC #7 lore raw

Quoted 2 lines · Hannes Frederic Sowa

I don't know how they could do this if they want to let other RFCs extend icmp types.

Oh, ICMPs can have padding. That's used to enforce "nice" alignment. Even RFC 6550 (RPL) has that. For example, you could simply pad the troublesome DIS, message which is

Offset Value Description
------ ----- ------------------------------------------------
0 0x9b ICMPv6 Type = RPL (155, section 6)
1 0x00 ICMPv6 Code = DODAG Information Solicitation (0)
2 0x?? Checksum
3 0x?? (continued)

4 0x00 Flags = 0 (section 6.2.1)
5 0x00 Reserved

to eight bytes (i.e., four bytes of body) by adding

6 0x01 Option Type = PadN (section 6.7.3)
7 0x00 Option Length = 0

But if nothing obliges the sender to do so, there's no excuse for Linux to expect such padding.

Quoted 1 line

Yes, that could be an issue. I would be willing to accept this fallout. :)

I'm kinda curious what sort of policy we have on that. The worst case would be that there's a bunch of 64 bit Linux machines out there, doing critical infrastructure things in the Internet (not an unlikely role, given the API in question), and their user space has some vulnerability if the kernel lets a "short" ICMPv6 packet through.

Of course, "The Almesberger-Sowa Internet Meltdown of 2013" does have a nice ring to it, in an apocalyptic kind of way ...

- Werner

Hannes Frederic Sowa

2013-07-25 23:31 UTC #8 lore raw

Quoted 18 lines · Werner Almesberger

Quoted 2 lines · Hannes Frederic Sowa
I don't know how they could do this if they want to let other RFCs extend icmp types.
Oh, ICMPs can have padding. That's used to enforce "nice" alignment. Even RFC 6550 (RPL) has that. For example, you could simply pad the troublesome DIS, message which is
Offset Value Description
------ ----- ------------------------------------------------
0 0x9b ICMPv6 Type = RPL (155, section 6)
1 0x00 ICMPv6 Code = DODAG Information Solicitation (0)
2 0x?? Checksum
3 0x?? (continued)
4 0x00 Flags = 0 (section 6.2.1)
5 0x00 Reserved

Quoted 7 lines

to eight bytes (i.e., four bytes of body) by adding
6 0x01 Option Type = PadN (section 6.7.3)
7 0x00 Option Length = 0
But if nothing obliges the sender to do so, there's no excuse for Linux to expect such padding.

Yes, of course, that's possible but not specified at all in the general ICMPv6 RFC. If packets are too short for some medium, I guess, one would stretch it with extension header paddings before the icmpv6 header.

Quoted 8 lines

Quoted 1 line
Yes, that could be an issue. I would be willing to accept this fallout. :)
I'm kinda curious what sort of policy we have on that. The worst case would be that there's a bunch of 64 bit Linux machines out there, doing critical infrastructure things in the Internet (not an unlikely role, given the API in question), and their user space has some vulnerability if the kernel lets a "short" ICMPv6 packet through.

You forgot one critical aspect: Important infrastructure is *never* going to be updated and definitely never runs IPv6. ;)

I don't think there is a policy, just intuition.

Quoted 2 lines

Of course, "The Almesberger-Sowa Internet Meltdown of 2013" does have a nice ring to it, in an apocalyptic kind of way ...

I would like to avoid such a scenario, but have seen enough patches that I kind of cooled down a bit. ;)

In summary, I agree, we should get both changes at once into the tree or none (of course I would still change the pointer to something reasonable and describe the circumstances in a comment if we don't change the current behaviour).

Greetings,

Hannes

Hannes Frederic Sowa

2013-08-01 05:48 UTC #9 lore raw

In reply to Werner Almesberger

Hi Werner!

Quoted 61 lines · Werner Almesberger

Quoted 1 line · Hannes Frederic Sowa
Hmm, maybe we should update the icmp header to something like
That would be quite clean. Is it okay to introduce new names like that in a uapi/ header (uapi/linux/icmpv6.h) ?
Quoted 1 line
Hmm, there is a bug in this function, _hdr must not be a pointer.
Oh, I didn't even notice that. Very good catch !
So on 32 bit system, it would actually work even with "short" ICMPv6 messages. Two wrongs sometimes do make a right :-)
I've attached a revised patch that, according to quick testing, still works and doesn't break anything else.
Thanks,
- Werner
---------------------------------- cut here -----------------------------------
include/uapi/linux/icmpv6.h +9 -5
index e0133c7..11eb5ff 100644
@@ -5,11 +5,15 @@
 #include <asm/byteorder.h>
 
 struct icmp6hdr {
-
-	__u8		icmp6_type;
-	__u8		icmp6_code;
-	__sum16		icmp6_cksum;
-
+	struct icmp6hdr_head {
+		__u8		type;
+		__u8		code;
+		__sum16		cksum;
+	} icmpv6_head;
+
+#define	icmp6_type	icmpv6_head.type
+#define	icmp6_code	icmpv6_head.code
+#define	icmp6_cksum	icmpv6_head.cksum
 
 	union {
 		__be32			un_data32[1];
net/ipv6/raw.c +2 -4
index c45f7a5..99ab06f 100644
@@ -108,14 +108,14 @@ found:
  */
 static int icmpv6_filter(const struct sock *sk, const struct sk_buff *skb)
 {
-	struct icmp6hdr *_hdr;
-	const struct icmp6hdr *hdr;
+	struct icmp6hdr_head _head;
+	const struct icmp6hdr_head *head;
 
-	hdr = skb_header_pointer(skb, skb_transport_offset(skb),
-				 sizeof(_hdr), &_hdr);

net/ipv6/raw.c found: · line 121

Quoted 10 lines

net/ipv6/raw.c +4 -2

-	if (hdr) {
+	head = skb_header_pointer(skb, skb_transport_offset(skb),
+				  sizeof(_head), &_head);
+	if (head) {
 		const __u32 *data = &raw6_sk(sk)->filter.data[0];
-		unsigned int type = hdr->icmp6_type;
+		unsigned int type = head->type;
 
 		return (data[type >> 5] & (1U << (type & 31))) != 0;
 	}

I would go ahead and post this as an offical patch submission now and let David Miller have a look.

Thanks,

Hannes

David Miller

maintainer

2013-08-02 01:10 UTC #10 lore raw

From: Hannes Frederic Sowa <[email protected]>
Date: Thu, 1 Aug 2013 07:48:44 +0200

Quoted 2 lines

I would go ahead and post this as an offical patch submission now and let David Miller have a look.

FWIW I think this is a safe change.

Werner Almesberger

author

2013-08-02 04:53 UTC #11 lore raw

Quoted 1 line · David Miller

FWIW I think this is a safe change.

Thanks !

Regarding the change to icmpv6.h, I found that the introduction of the otherwise very nice and tidy "struct icmp6hdr_head" creates a conflict with net/bridge/br_multicast.c:br_multicast_ipv6_rcv which has a variable called "icmp6_type".

While it would be easy enough to rename that variable, this makes me wonder if the change isn't too intrusive after all, I think I'd rather go with my original idea and just change the size to 4, with a comment why and what this is.

I'll post the patches in a bit.

- Werner