* Re: minimum ICMPv6 message size vs. RPL's DIS
2013-07-25 10:30 ` Werner Almesberger
@ 2013-07-25 13:03 ` Hannes Frederic Sowa
2013-07-25 13:58 ` Hannes Frederic Sowa
2013-08-01 5:48 ` Hannes Frederic Sowa
2 siblings, 0 replies; 12+ messages in thread
From: Hannes Frederic Sowa @ 2013-07-25 13:03 UTC (permalink / raw)
To: Werner Almesberger; +Cc: netdev
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: minimum ICMPv6 message size vs. RPL's DIS
2013-07-25 10:30 ` Werner Almesberger
2013-07-25 13:03 ` Hannes Frederic Sowa
@ 2013-07-25 13:58 ` Hannes Frederic Sowa
2013-07-25 14:32 ` Werner Almesberger
2013-08-01 5:48 ` Hannes Frederic Sowa
2 siblings, 1 reply; 12+ messages in thread
From: Hannes Frederic Sowa @ 2013-07-25 13:58 UTC (permalink / raw)
To: Werner Almesberger; +Cc: netdev, davem
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: minimum ICMPv6 message size vs. RPL's DIS
2013-07-25 13:58 ` Hannes Frederic Sowa
@ 2013-07-25 14:32 ` Werner Almesberger
2013-07-25 18:40 ` Hannes Frederic Sowa
0 siblings, 1 reply; 12+ messages in thread
From: Werner Almesberger @ 2013-07-25 14:32 UTC (permalink / raw)
To: netdev, davem
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: minimum ICMPv6 message size vs. RPL's DIS
2013-07-25 14:32 ` Werner Almesberger
@ 2013-07-25 18:40 ` Hannes Frederic Sowa
2013-07-25 21:47 ` Werner Almesberger
0 siblings, 1 reply; 12+ messages in thread
From: Hannes Frederic Sowa @ 2013-07-25 18:40 UTC (permalink / raw)
To: Werner Almesberger; +Cc: netdev, davem
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: minimum ICMPv6 message size vs. RPL's DIS
2013-07-25 18:40 ` Hannes Frederic Sowa
@ 2013-07-25 21:47 ` Werner Almesberger
2013-07-25 23:31 ` Hannes Frederic Sowa
0 siblings, 1 reply; 12+ messages in thread
From: Werner Almesberger @ 2013-07-25 21:47 UTC (permalink / raw)
To: netdev, davem
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: minimum ICMPv6 message size vs. RPL's DIS
2013-07-25 21:47 ` Werner Almesberger
@ 2013-07-25 23:31 ` Hannes Frederic Sowa
0 siblings, 0 replies; 12+ messages in thread
From: Hannes Frederic Sowa @ 2013-07-25 23:31 UTC (permalink / raw)
To: Werner Almesberger; +Cc: netdev, davem
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: minimum ICMPv6 message size vs. RPL's DIS
2013-07-25 10:30 ` Werner Almesberger
2013-07-25 13:03 ` Hannes Frederic Sowa
2013-07-25 13:58 ` Hannes Frederic Sowa
@ 2013-08-01 5:48 ` Hannes Frederic Sowa
2013-08-02 1:10 ` David Miller
2 siblings, 1 reply; 12+ messages in thread
From: Hannes Frederic Sowa @ 2013-08-01 5:48 UTC (permalink / raw)
To: Werner Almesberger, davem; +Cc: netdev
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: minimum ICMPv6 message size vs. RPL's DIS
2013-08-01 5:48 ` Hannes Frederic Sowa
@ 2013-08-02 1:10 ` David Miller
2013-08-02 4:51 ` Werner Almesberger
0 siblings, 1 reply; 12+ messages in thread
From: David Miller @ 2013-08-02 1:10 UTC (permalink / raw)
To: hannes; +Cc: werner, netdev
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: minimum ICMPv6 message size vs. RPL's DIS
2013-08-02 1:10 ` David Miller
@ 2013-08-02 4:51 ` Werner Almesberger
0 siblings, 0 replies; 12+ messages in thread
From: Werner Almesberger @ 2013-08-02 4:51 UTC (permalink / raw)
To: David Miller; +Cc: hannes, netdev
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
^ permalink raw reply [flat|nested] 12+ messages in thread