minimum ICMPv6 message size vs. RPL's DIS

Hannes Frederic Sowa wrote:
> 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) ?

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Jul 25, 2013 at 07:30:49AM -0300, Werner Almesberger wrote:
> Hannes Frederic Sowa wrote:
> > 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) ?
> 
> > 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 -----------------------------------
> 
> diff --git a/include/uapi/linux/icmpv6.h b/include/uapi/linux/icmpv6.h
> index e0133c7..11eb5ff 100644
> --- a/include/uapi/linux/icmpv6.h
> +++ b/include/uapi/linux/icmpv6.h
> @@ -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;

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

> +
> +#define	icmp6_type	icmpv6_head.type
> +#define	icmp6_code	icmpv6_head.code
> +#define	icmp6_cksum	icmpv6_head.cksum
>  
>  	union {
>  		__be32			un_data32[1];
> diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
> index c45f7a5..99ab06f 100644
> --- a/net/ipv6/raw.c
> +++ b/net/ipv6/raw.c
> @@ -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;
>  	}

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Jul 25, 2013 at 07:30:49AM -0300, Werner Almesberger wrote:
> Hannes Frederic Sowa wrote:
> > 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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hannes Frederic Sowa wrote:
> 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Jul 25, 2013 at 11:32:23AM -0300, Werner Almesberger wrote:
> Hannes Frederic Sowa wrote:
> > 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).

> - name pollution visible to future user space,

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

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

> 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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hannes Frederic Sowa wrote:
> 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.

> 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Jul 25, 2013 at 06:47:49PM -0300, Werner Almesberger wrote:
> Hannes Frederic Sowa wrote:
> > 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.

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.

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

> 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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Werner!

On Thu, Jul 25, 2013 at 07:30:49AM -0300, Werner Almesberger wrote:
> Hannes Frederic Sowa wrote:
> > 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) ?
> 
> > 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 -----------------------------------
> 
> diff --git a/include/uapi/linux/icmpv6.h b/include/uapi/linux/icmpv6.h
> index e0133c7..11eb5ff 100644
> --- a/include/uapi/linux/icmpv6.h
> +++ b/include/uapi/linux/icmpv6.h
> @@ -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];
> diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
> index c45f7a5..99ab06f 100644
> --- a/net/ipv6/raw.c
> +++ b/net/ipv6/raw.c
> @@ -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;
>  	}

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

Thanks,

  Hannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Thu, 1 Aug 2013 07:48:44 +0200

> 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Miller wrote:
> 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html