[PATCH] ip_route_input panic fix

[PATCH] ip_route_input panic fix

[Stephen Hemminger]

This fixes http://bugzilla.kernel.org/show_bug.cgi?id=6388
The bug is caused by ip_route_input dereferencing skb->nh.protocol of
the dummy skb passed dow from inet_rtm_getroute (Thanks Thomas for seeing
it). It only happens if the route requested is for a multicast IP
address.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

--- linux-2.6.16.6.orig/net/ipv4/route.c
+++ linux-2.6.16.6/net/ipv4/route.c
@@ -2750,7 +2750,10 @@ int inet_rtm_getroute(struct sk_buff *in
 	/* Reserve room for dummy headers, this skb can pass
 	   through good chunk of routing engine.
 	 */
-	skb->mac.raw = skb->data;
+	skb->mac.raw = skb->nh.raw = skb->data;
+
+	/* Bugfix: need to give ip_route_input enough of an IP header to not gag. */
+	skb->nh.iph->protocol = IPPROTO_ICMP;
 	skb_reserve(skb, MAX_HEADER + sizeof(struct iphdr));
 
 	if (rta[RTA_SRC - 1])

Re: [PATCH] ip_route_input panic fix

[Herbert Xu]

Hi Stephen:

Stephen Hemminger <shemminger@osdl.org> wrote:
> This fixes http://bugzilla.kernel.org/show_bug.cgi?id=6388
> The bug is caused by ip_route_input dereferencing skb->nh.protocol of
> the dummy skb passed dow from inet_rtm_getroute (Thanks Thomas for seeing
> it). It only happens if the route requested is for a multicast IP
> address.

Good catch.

> -       skb->mac.raw = skb->data;
> +       skb->mac.raw = skb->nh.raw = skb->data;

This should fix it.

> +       /* Bugfix: need to give ip_route_input enough of an IP header to not gag. */
> +       skb->nh.iph->protocol = IPPROTO_ICMP;

Do we really need this? After all we can get completely bogus values
coming in through the network too.

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

Re: [PATCH] ip_route_input panic fix

[Stephen Hemminger]

On Tue, 18 Apr 2006 12:28:48 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Hi Stephen:
> 
> Stephen Hemminger <shemminger@osdl.org> wrote:
> > This fixes http://bugzilla.kernel.org/show_bug.cgi?id=6388
> > The bug is caused by ip_route_input dereferencing skb->nh.protocol of
> > the dummy skb passed dow from inet_rtm_getroute (Thanks Thomas for seeing
> > it). It only happens if the route requested is for a multicast IP
> > address.
> 
> Good catch.
> 
> > -       skb->mac.raw = skb->data;
> > +       skb->mac.raw = skb->nh.raw = skb->data;
> 
> This should fix it.
> 
> > +       /* Bugfix: need to give ip_route_input enough of an IP header to not gag. */
> > +       skb->nh.iph->protocol = IPPROTO_ICMP;
> 
> Do we really need this? After all we can get completely bogus values
> coming in through the network too.

Not really, just that ip_check_mc looks at the proto for !IGMP. And maybe
some tool like coverity or sparse would be smart enough to look for
uninitialized data usage.

Re: [PATCH] ip_route_input panic fix

[Herbert Xu]

On Mon, Apr 17, 2006 at 07:49:31PM -0700, Stephen Hemminger wrote:
>
> Not really, just that ip_check_mc looks at the proto for !IGMP. And maybe
> some tool like coverity or sparse would be smart enough to look for
> uninitialized data usage.

That's a good point.

Thanks Stephen,
-- 
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

Re: [PATCH] ip_route_input panic fix

[David S. Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 18 Apr 2006 12:28:48 +1000

> Stephen Hemminger <shemminger@osdl.org> wrote:
> > +       /* Bugfix: need to give ip_route_input enough of an IP header to not gag. */
> > +       skb->nh.iph->protocol = IPPROTO_ICMP;
> 
> Do we really need this? After all we can get completely bogus values
> coming in through the network too.

ip_mc_check() will return 1 if it happens to be IGMP, that's not
a good default for this netlink lookup case.

Re: [PATCH] ip_route_input panic fix

[Herbert Xu]

Stephen Hemminger <shemminger@osdl.org> wrote:
> 
> --- linux-2.6.16.6.orig/net/ipv4/route.c
> +++ linux-2.6.16.6/net/ipv4/route.c
> @@ -2750,7 +2750,10 @@ int inet_rtm_getroute(struct sk_buff *in
>        /* Reserve room for dummy headers, this skb can pass
>           through good chunk of routing engine.
>         */
> -       skb->mac.raw = skb->data;
> +       skb->mac.raw = skb->nh.raw = skb->data;
> +
> +       /* Bugfix: need to give ip_route_input enough of an IP header to not gag. */
> +       skb->nh.iph->protocol = IPPROTO_ICMP;

Looking at this again, the root of this problem is the IGMPv3
patch which started using the skb->nh.iph->protocol as a key.

So what we really should do is make the protocol an explicit parameter
to the ip_route_input function.  This will make it clear to all the
users which include some pretty weird cases that the protocol is needed.

In fact I'm unsure as to whether all the other users of ip_route_input
is safe as it is regarding the protocol.

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

Re: [PATCH] ip_route_input panic fix

[David S. Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 18 Apr 2006 16:54:48 +1000

> Looking at this again, the root of this problem is the IGMPv3
> patch which started using the skb->nh.iph->protocol as a key.
> 
> So what we really should do is make the protocol an explicit parameter
> to the ip_route_input function.  This will make it clear to all the
> users which include some pretty weird cases that the protocol is needed.
> 
> In fact I'm unsure as to whether all the other users of ip_route_input
> is safe as it is regarding the protocol.

There are other areas of the packet which are interpreted in various
ways.  For example, the martian source handling will dump the MAC
directly from skb->mac.raw into the kernel logs.

The output path is so much cleaner, because things like the protocol
are filled out in the struct flowi so there is no need to be parsing
the SKB in any way.

Re: [PATCH] ip_route_input panic fix

[Herbert Xu]

On Tue, Apr 18, 2006 at 02:54:16PM -0700, David S. Miller wrote:
> 
> There are other areas of the packet which are interpreted in various
> ways.  For example, the martian source handling will dump the MAC
> directly from skb->mac.raw into the kernel logs.

That's scary.  I think this stuff needs an audit.

> The output path is so much cleaner, because things like the protocol
> are filled out in the struct flowi so there is no need to be parsing
> the SKB in any way.

Absolutely.  I think we should put Stephen's patch in ASAP.  Once the
immediate problem is gone we can take our time and make the input path
more like the output.

At the end of this I'd like to see the skb argument from ip_route_input
replaced by a dst_entry.

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

Re: [PATCH] ip_route_input panic fix

[Alexey Kuznetsov]

Hello!

> Looking at this again, the root of this problem is the IGMPv3
> patch which started using the skb->nh.iph->protocol as a key.

No, root is that this fake skb was not properly initialized.
It should, it should be a good real IP skb.


> In fact I'm unsure as to whether all the other users of ip_route_input
> is safe as it is regarding the protocol.

ip_route_input takes skb as an argument exactly because it needs nothing
but skb and there is always an skb, when we "input".
ip_route_output would be happy to take an skb as well,
but unfortuntely it happens before we have an skb.

I do not see anything scary here: agree, when skb->nh happens to be undefined,
such skb would crash almost any place in IP stack. :-)


Actually, this weird case in inet_get_route() is the only place, where
a dummy skb is used and it is needed mostly to resolve multicast routes.
In this case this fake skb really passes through all the engine, even
delivered to user space in some sense, and when the route is resolved,
the same skb is submitted to netlink socket. I remember, Dave found
something very bad about this and this even deserved a place in TODO list,
but franky speaking I did not understand what is so wrong with this trick.

Alexey

Re: [PATCH] ip_route_input panic fix

[Herbert Xu]

On Wed, Apr 19, 2006 at 03:52:22AM +0400, Alexey Kuznetsov wrote:
> 
> Actually, this weird case in inet_get_route() is the only place, where

There is also the ARP code which passes an ARP packet through that
would get dereferenced as an IP packet.  Granted this shouldn't crash
because nh is set properly.

But we really should make up our mind as to whether the routing key
comes from the arguments to ip_route_input (src/dst/...) or the skb.

Using both is just asking for trouble.

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

Re: [PATCH] ip_route_input panic fix

[David S. Miller]

From: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Date: Wed, 19 Apr 2006 03:52:22 +0400

> Actually, this weird case in inet_get_route() is the only place, where
> a dummy skb is used and it is needed mostly to resolve multicast routes.
> In this case this fake skb really passes through all the engine, even
> delivered to user space in some sense, and when the route is resolved,
> the same skb is submitted to netlink socket. I remember, Dave found
> something very bad about this and this even deserved a place in TODO list,
> but franky speaking I did not understand what is so wrong with this trick.

Problem there is via rt_fill_info().  When multicast route cannot be
found by ipmr, it tries to use this netlink SKB to send out a probe.

ipmr_get_route() is the trouble maker.  If ipmr_cache_find() cannot
find an entry, it tries to use the netlink SKB to send out an ipv4
packet, completely mangling it, via ipmr_cache_unresolved().

Even worse it may even free the skb on us, or queue it to
mroute_socket.

It is pure disaster, this entire code path.

Re: [PATCH] ip_route_input panic fix

[Alexey Kuznetsov]

Hello!

> ipmr_get_route() is the trouble maker.  If ipmr_cache_find() cannot
> find an entry, it tries to use the netlink SKB to send out an ipv4
> packet, completely mangling it, via ipmr_cache_unresolved().

It just adds dummy IP header to tail of this skb. Nothing illegal.
The skb is not sent out, it is enqueued in an ARP-like queue and
the IP header is submitted to mroute_socket.

When user level mrouter resolves the route, ipmr_cache_resolve()
removes this, completes filling netlink attributes and sends it to netlink.


> Even worse it may even free the skb on us,

Oops, seems, this is a real bug... Need to think how to fix.


> or queue it to mroute_socket.

It is OK, it is the purpose.


> It is pure disaster, this entire code path.

Well, it may be buggy, but esentially there is nothing to simplify.
At least, I do not see.

Alexey

Re: [PATCH] ip_route_input panic fix

[Alexey Kuznetsov]

Hello!

> There is also the ARP code which passes an ARP packet through that
> would get dereferenced as an IP packet.  Granted this shouldn't crash
> because nh is set properly.

And point to something which is not an IP header. So, iph->protocol
is something funny. :-)

It is plain luck that this never happens, ARP packets
with multicast addresses are filtered out.

Mess, I agree.


> But we really should make up our mind as to whether the routing key
> comes from the arguments to ip_route_input (src/dst/...) or the skb.
>
> Using both is just asking for trouble.

Well, both sets are present only for use the same function in ARP.

So, arguments. skb can be even preserved, but it should not be used
for anything but debugging or for hints, when we should not create
cache entry.


BTW, I cannot figure out what ip_check_mc tries to do with protocol
(which is __u16 by some reason). If it creates cache entry, protocol
is not checked. Funny.

Alexey

Re: [PATCH] ip_route_input panic fix

[Alexey Kuznetsov]

Hello!

> There is also the ARP code which passes an ARP packet through that
> would get dereferenced as an IP packet.  Granted this shouldn't crash
> because nh is set properly.

And points to something which is not an IP header. So, iph->protocol
is something funny. :-)

It is plain luck that this never happens, ARP packets
with multicast addresses are filtered out.

Mess, I agree.


> But we really should make up our mind as to whether the routing key
> comes from the arguments to ip_route_input (src/dst/...) or the skb.
>
> Using both is just asking for trouble.

Well, both sets are present only for use the same function in ARP.

So, arguments. Actually, skb can be preserved, but it should not be used
for anything but debugging or for hints, when we should not create
cache entry.


BTW, I cannot figure out what ip_check_mc() tries to do with protocol
(which is __u16 by some reason). If it creates cache entry, protocol
is not checked. Funny.

Alexey