* ipip and ip_gre encapsulation bugs @ 2006-09-12 16:50 Al Viro 2006-09-14 0:23 ` Herbert Xu 0 siblings, 1 reply; 4+ messages in thread From: Al Viro @ 2006-09-12 16:50 UTC (permalink / raw) To: davem; +Cc: torvalds, netdev Handling of ipip and ip_gre ICMP error relaying is b0rken; it accesses 32bit net-endian field as host-endian, does comparison, subtraction and stuffs the result to 32bit net-endian. Without any conversions. Fixed, made endian-clean. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> ---- diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c index 0f9b3a3..b9dd2f0 100644 --- a/net/ipv4/ip_gre.c +++ b/net/ipv4/ip_gre.c @@ -393,7 +393,8 @@ #else int code = skb->h.icmph->code; int rel_type = 0; int rel_code = 0; - int rel_info = 0; + __be32 rel_info = 0; + __u32 n = 0; u16 flags; int grehlen = (iph->ihl<<2) + 4; struct sk_buff *skb2; @@ -422,14 +423,16 @@ #else default: return; case ICMP_PARAMETERPROB: - if (skb->h.icmph->un.gateway < (iph->ihl<<2)) + n = ntohl(skb->h.icmph->un.gateway); + if (n < (iph->ihl<<2)) return; /* So... This guy found something strange INSIDE encapsulated packet. Well, he is fool, but what can we do ? */ rel_type = ICMP_PARAMETERPROB; - rel_info = skb->h.icmph->un.gateway - grehlen; + n -= grehlen; + rel_info = htonl(n); break; case ICMP_DEST_UNREACH: @@ -440,13 +443,14 @@ #else return; case ICMP_FRAG_NEEDED: /* And it is the only really necessary thing :-) */ - rel_info = ntohs(skb->h.icmph->un.frag.mtu); - if (rel_info < grehlen+68) + n = ntohs(skb->h.icmph->un.frag.mtu); + if (n < grehlen+68) return; - rel_info -= grehlen; + n -= grehlen; /* BSD 4.2 MORE DOES NOT EXIST IN NATURE. */ - if (rel_info > ntohs(eiph->tot_len)) + if (n > ntohs(eiph->tot_len)) return; + rel_info = htonl(n); break; default: /* All others are translated to HOST_UNREACH. @@ -508,12 +512,11 @@ #else /* change mtu on this route */ if (type == ICMP_DEST_UNREACH && code == ICMP_FRAG_NEEDED) { - if (rel_info > dst_mtu(skb2->dst)) { + if (n > dst_mtu(skb2->dst)) { kfree_skb(skb2); return; } - skb2->dst->ops->update_pmtu(skb2->dst, rel_info); - rel_info = htonl(rel_info); + skb2->dst->ops->update_pmtu(skb2->dst, n); } else if (type == ICMP_TIME_EXCEEDED) { struct ip_tunnel *t = netdev_priv(skb2->dev); if (t->parms.iph.ttl) { diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c index 76ab50b..c27b071 100644 --- a/net/ipv4/ipip.c +++ b/net/ipv4/ipip.c @@ -341,7 +341,8 @@ #else int code = skb->h.icmph->code; int rel_type = 0; int rel_code = 0; - int rel_info = 0; + __be32 rel_info = 0; + __u32 n = 0; struct sk_buff *skb2; struct flowi fl; struct rtable *rt; @@ -354,14 +355,15 @@ #else default: return 0; case ICMP_PARAMETERPROB: - if (skb->h.icmph->un.gateway < hlen) + n = htonl(skb->h.icmph->un.gateway); + if (n < hlen) return 0; /* So... This guy found something strange INSIDE encapsulated packet. Well, he is fool, but what can we do ? */ rel_type = ICMP_PARAMETERPROB; - rel_info = skb->h.icmph->un.gateway - hlen; + rel_info = htonl(n - hlen); break; case ICMP_DEST_UNREACH: @@ -372,13 +374,14 @@ #else return 0; case ICMP_FRAG_NEEDED: /* And it is the only really necessary thing :-) */ - rel_info = ntohs(skb->h.icmph->un.frag.mtu); - if (rel_info < hlen+68) + n = ntohs(skb->h.icmph->un.frag.mtu); + if (n < hlen+68) return 0; - rel_info -= hlen; + n -= hlen; /* BSD 4.2 MORE DOES NOT EXIST IN NATURE. */ - if (rel_info > ntohs(eiph->tot_len)) + if (n > ntohs(eiph->tot_len)) return 0; + rel_info = htonl(n); break; default: /* All others are translated to HOST_UNREACH. @@ -440,12 +443,11 @@ #else /* change mtu on this route */ if (type == ICMP_DEST_UNREACH && code == ICMP_FRAG_NEEDED) { - if (rel_info > dst_mtu(skb2->dst)) { + if (n > dst_mtu(skb2->dst)) { kfree_skb(skb2); return 0; } - skb2->dst->ops->update_pmtu(skb2->dst, rel_info); - rel_info = htonl(rel_info); + skb2->dst->ops->update_pmtu(skb2->dst, n); } else if (type == ICMP_TIME_EXCEEDED) { struct ip_tunnel *t = netdev_priv(skb2->dev); if (t->parms.iph.ttl) { ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: ipip and ip_gre encapsulation bugs 2006-09-12 16:50 ipip and ip_gre encapsulation bugs Al Viro @ 2006-09-14 0:23 ` Herbert Xu 2006-09-14 1:16 ` Al Viro 0 siblings, 1 reply; 4+ messages in thread From: Herbert Xu @ 2006-09-14 0:23 UTC (permalink / raw) To: Al Viro; +Cc: davem, torvalds, netdev Al Viro <viro@ftp.linux.org.uk> wrote: > Handling of ipip and ip_gre ICMP error relaying is b0rken; it accesses > 32bit net-endian field as host-endian, does comparison, subtraction and > stuffs the result to 32bit net-endian. Without any conversions. Thanks for spotting this. > @@ -422,14 +423,16 @@ #else > default: > return; > case ICMP_PARAMETERPROB: > - if (skb->h.icmph->un.gateway < (iph->ihl<<2)) > + n = ntohl(skb->h.icmph->un.gateway); > + if (n < (iph->ihl<<2)) > return; I don't think this is right. The original code works correctly on little-endian. The patch introduces a swab on little-endian so it isn't right anymore. I suggest that you add a member to the icmph union which is just a u8 since that is what RFC792 specifies for PARAMETERPROB. That way we can just use that u8 value without any swapping at all. > @@ -440,13 +443,14 @@ #else > return; > case ICMP_FRAG_NEEDED: > /* And it is the only really necessary thing :-) */ > - rel_info = ntohs(skb->h.icmph->un.frag.mtu); > - if (rel_info < grehlen+68) > + n = ntohs(skb->h.icmph->un.frag.mtu); > + if (n < grehlen+68) > return; That's a good clean-up. > diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c > index 76ab50b..c27b071 100644 > --- a/net/ipv4/ipip.c > +++ b/net/ipv4/ipip.c > @@ -354,14 +355,15 @@ #else > default: > return 0; > case ICMP_PARAMETERPROB: > - if (skb->h.icmph->un.gateway < hlen) > + n = htonl(skb->h.icmph->un.gateway); > + if (n < hlen) > return 0; Same problem as above. Cheers, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: ipip and ip_gre encapsulation bugs 2006-09-14 0:23 ` Herbert Xu @ 2006-09-14 1:16 ` Al Viro 2006-09-14 4:03 ` David Miller 0 siblings, 1 reply; 4+ messages in thread From: Al Viro @ 2006-09-14 1:16 UTC (permalink / raw) To: Herbert Xu; +Cc: davem, torvalds, netdev On Thu, Sep 14, 2006 at 10:23:42AM +1000, Herbert Xu wrote: > > - if (skb->h.icmph->un.gateway < (iph->ihl<<2)) > > + n = ntohl(skb->h.icmph->un.gateway); > > + if (n < (iph->ihl<<2)) > > return; > > I don't think this is right. The original code works correctly on > little-endian. The patch introduces a swab on little-endian so it > isn't right anymore. > I suggest that you add a member to the icmph union which is just a > u8 since that is what RFC792 specifies for PARAMETERPROB. That way > we can just use that u8 value without any swapping at all. OK, after rereading the RFC... Pointer field is one octet, indeed. Avoiding all swapping will be tough - icmp_send() takes 32bit argument, net-endian. FWIW, I'd do the same thing as we do in the other places that deal with ICMP_PARAMETERPROB. E.g. ./net/ipv4/ip_options.c:456: icmp_send(skb, ICMP_PARAMETERPROB, 0, htonl((pp_ptr-iph)<<24)); and ./net/ipv4/icmp.c:651: info = ntohl(icmph->un.gateway) >> 24; IOW, >From cef2804df14cfc340f0fa70ecd09551c8e22447b Mon Sep 17 00:00:00 2001 From: Al Viro <viro@ftp.linux.org.uk> Date: Tue, 12 Sep 2006 17:50:58 +0100 Subject: [PATCH] ipip and ip_gre encapsulation bugs Handling of ipip and ip_gre ICMP error relaying is b0rken; it accesses 8bit field + 3 reserved octets as host-endian 32bit, does comparison, subtraction and stuffs the result back. That breaks on big-endian. Fixed, made endian-clean. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- net/ipv4/ip_gre.c | 23 +++++++++++++---------- net/ipv4/ipip.c | 22 ++++++++++++---------- 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c index 0f9b3a3..233f009 100644 --- a/net/ipv4/ip_gre.c +++ b/net/ipv4/ip_gre.c @@ -393,7 +393,8 @@ #else int code = skb->h.icmph->code; int rel_type = 0; int rel_code = 0; - int rel_info = 0; + __be32 rel_info = 0; + __u32 n = 0; u16 flags; int grehlen = (iph->ihl<<2) + 4; struct sk_buff *skb2; @@ -422,14 +423,16 @@ #else default: return; case ICMP_PARAMETERPROB: - if (skb->h.icmph->un.gateway < (iph->ihl<<2)) + n = ntohl(skb->h.icmph->un.gateway) >> 24; + if (n < (iph->ihl<<2)) return; /* So... This guy found something strange INSIDE encapsulated packet. Well, he is fool, but what can we do ? */ rel_type = ICMP_PARAMETERPROB; - rel_info = skb->h.icmph->un.gateway - grehlen; + n -= grehlen; + rel_info = htonl(n << 24); break; case ICMP_DEST_UNREACH: @@ -440,13 +443,14 @@ #else return; case ICMP_FRAG_NEEDED: /* And it is the only really necessary thing :-) */ - rel_info = ntohs(skb->h.icmph->un.frag.mtu); - if (rel_info < grehlen+68) + n = ntohs(skb->h.icmph->un.frag.mtu); + if (n < grehlen+68) return; - rel_info -= grehlen; + n -= grehlen; /* BSD 4.2 MORE DOES NOT EXIST IN NATURE. */ - if (rel_info > ntohs(eiph->tot_len)) + if (n > ntohs(eiph->tot_len)) return; + rel_info = htonl(n); break; default: /* All others are translated to HOST_UNREACH. @@ -508,12 +512,11 @@ #else /* change mtu on this route */ if (type == ICMP_DEST_UNREACH && code == ICMP_FRAG_NEEDED) { - if (rel_info > dst_mtu(skb2->dst)) { + if (n > dst_mtu(skb2->dst)) { kfree_skb(skb2); return; } - skb2->dst->ops->update_pmtu(skb2->dst, rel_info); - rel_info = htonl(rel_info); + skb2->dst->ops->update_pmtu(skb2->dst, n); } else if (type == ICMP_TIME_EXCEEDED) { struct ip_tunnel *t = netdev_priv(skb2->dev); if (t->parms.iph.ttl) { diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c index 76ab50b..0c45565 100644 --- a/net/ipv4/ipip.c +++ b/net/ipv4/ipip.c @@ -341,7 +341,8 @@ #else int code = skb->h.icmph->code; int rel_type = 0; int rel_code = 0; - int rel_info = 0; + __be32 rel_info = 0; + __u32 n = 0; struct sk_buff *skb2; struct flowi fl; struct rtable *rt; @@ -354,14 +355,15 @@ #else default: return 0; case ICMP_PARAMETERPROB: - if (skb->h.icmph->un.gateway < hlen) + n = ntohl(skb->h.icmph->un.gateway) >> 24; + if (n < hlen) return 0; /* So... This guy found something strange INSIDE encapsulated packet. Well, he is fool, but what can we do ? */ rel_type = ICMP_PARAMETERPROB; - rel_info = skb->h.icmph->un.gateway - hlen; + rel_info = htonl((n - hlen) << 24); break; case ICMP_DEST_UNREACH: @@ -372,13 +374,14 @@ #else return 0; case ICMP_FRAG_NEEDED: /* And it is the only really necessary thing :-) */ - rel_info = ntohs(skb->h.icmph->un.frag.mtu); - if (rel_info < hlen+68) + n = ntohs(skb->h.icmph->un.frag.mtu); + if (n < hlen+68) return 0; - rel_info -= hlen; + n -= hlen; /* BSD 4.2 MORE DOES NOT EXIST IN NATURE. */ - if (rel_info > ntohs(eiph->tot_len)) + if (n > ntohs(eiph->tot_len)) return 0; + rel_info = htonl(n); break; default: /* All others are translated to HOST_UNREACH. @@ -440,12 +443,11 @@ #else /* change mtu on this route */ if (type == ICMP_DEST_UNREACH && code == ICMP_FRAG_NEEDED) { - if (rel_info > dst_mtu(skb2->dst)) { + if (n > dst_mtu(skb2->dst)) { kfree_skb(skb2); return 0; } - skb2->dst->ops->update_pmtu(skb2->dst, rel_info); - rel_info = htonl(rel_info); + skb2->dst->ops->update_pmtu(skb2->dst, n); } else if (type == ICMP_TIME_EXCEEDED) { struct ip_tunnel *t = netdev_priv(skb2->dev); if (t->parms.iph.ttl) { -- 1.4.0.rc2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: ipip and ip_gre encapsulation bugs 2006-09-14 1:16 ` Al Viro @ 2006-09-14 4:03 ` David Miller 0 siblings, 0 replies; 4+ messages in thread From: David Miller @ 2006-09-14 4:03 UTC (permalink / raw) To: viro; +Cc: herbert, torvalds, netdev From: Al Viro <viro@ftp.linux.org.uk> Date: Thu, 14 Sep 2006 02:16:37 +0100 > OK, after rereading the RFC... Pointer field is one octet, indeed. > Avoiding all swapping will be tough - icmp_send() takes 32bit argument, > net-endian. Thanks for the fix Al, but did you notice that this code is totally unused? It's a code block commented out by a large ifdef, such that the code block is never enabled and serves as example code we could use if some limitations in the current internet did not exist. :-) But thanks anyways, I'll queue this up for 2.6.19 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2006-09-14 4:02 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2006-09-12 16:50 ipip and ip_gre encapsulation bugs Al Viro 2006-09-14 0:23 ` Herbert Xu 2006-09-14 1:16 ` Al Viro 2006-09-14 4:03 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).