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