[PATCH] route: fix ICMP redirect validation

The commit f39925dbde7788cfb96419c0f092b086aa325c0f
(ipv4: Cache learned redirect information in inetpeer.)
removed some ICMP packet validations which are required by
RFC 1122, section 3.2.2.2:
...
  A Redirect message SHOULD be silently discarded if the new
  gateway address it specifies is not on the same connected
  (sub-) net through which the Redirect arrived [INTRO:2,
  Appendix A], or if the source of the Redirect is not the
  current first-hop gateway for the specified destination (see
  Section 3.3.1).

Signed-off-by: Flavio Leitner <redacted>
---
 net/ipv4/route.c |   47 ++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 26c77e1..1c11f28 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c

@@ -1308,7 +1308,12 @@ static void rt_del(unsigned hash, struct rtable *rt)
 void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw,
 		    __be32 saddr, struct net_device *dev)
 {
+	int s, i;
 	struct in_device *in_dev = __in_dev_get_rcu(dev);
+	struct rtable *rth;
+	struct rtable __rcu **rthp;
+	__be32 skeys[2] = { saddr, 0 };
+	int    ikeys[2] = { dev->ifindex, 0 };
 	struct inet_peer *peer;
 	struct net *net;
 

@@ -1321,6 +1326,9 @@ void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw,
 	    ipv4_is_zeronet(new_gw))
 		goto reject_redirect;
 
+	if (!rt_caching(net))
+		goto reject_redirect;;
+
 	if (!IN_DEV_SHARED_MEDIA(in_dev)) {
 		if (!inet_addr_onlink(in_dev, new_gw, old_gw))
 			goto reject_redirect;

@@ -1331,13 +1339,42 @@ void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw,
 			goto reject_redirect;
 	}
 
-	peer = inet_getpeer_v4(daddr, 1);
-	if (peer) {
-		peer->redirect_learned.a4 = new_gw;
+	for (s = 0; s < 2; s++) {
+		for (i = 0; i < 2; i++) {
+			unsigned int hash = rt_hash(daddr, skeys[s],
+						    ikeys[i], rt_genid(net));
 
-		inet_putpeer(peer);
+			rthp=&rt_hash_table[hash].chain;
 
-		atomic_inc(&__rt_peer_genid);
+			while ((rth = rcu_dereference(*rthp)) != NULL) {
+
+				if (rth->rt_key_dst != daddr ||
+				    rth->rt_key_src != skeys[s] ||
+				    rth->rt_oif != ikeys[i] ||
+				    rt_is_input_route(rth) ||
+				    rt_is_expired(rth) ||
+				    !net_eq(dev_net(rth->dst.dev), net)) {
+					rthp = &rth->dst.rt_next;
+					continue;
+				}
+
+				if (rth->rt_dst != daddr ||
+				    rth->rt_src != saddr ||
+				    rth->rt_gateway != old_gw ||
+				    rth->dst.dev != dev ||
+				    rth->dst.error)
+					break;
+
+				peer = inet_getpeer_v4(daddr, 1);
+				if (peer) {
+					peer->redirect_learned.a4 = new_gw;
+					inet_putpeer(peer);
+					atomic_inc(&__rt_peer_genid);
+				}
+
+				break;
+			}
+		}
 	}
 	return;
 

-- 
1.7.6

From: Flavio Leitner <redacted>
Date: Wed,  5 Oct 2011 11:20:04 -0300

The commit f39925dbde7788cfb96419c0f092b086aa325c0f
(ipv4: Cache learned redirect information in inetpeer.)
removed some ICMP packet validations which are required by
RFC 1122, section 3.2.2.2:
...
  A Redirect message SHOULD be silently discarded if the new
  gateway address it specifies is not on the same connected
  (sub-) net through which the Redirect arrived [INTRO:2,
  Appendix A], or if the source of the Redirect is not the
  current first-hop gateway for the specified destination (see
  Section 3.3.1).

Signed-off-by: Flavio Leitner <redacted>

The reason for putting this into the inetpeer cache was so that we
didn't need to consult the routing cache at all.  We're working to
remove it at some point, so every dependency matters.

Can you implement this such that only an inetpeer cache probe is
necessary?

Thanks.

On Mon, 17 Oct 2011 19:43:44 -0400 (EDT)
David Miller [off-list ref] wrote:

From: Flavio Leitner <redacted>
Date: Wed,  5 Oct 2011 11:20:04 -0300

quoted

The commit f39925dbde7788cfb96419c0f092b086aa325c0f
(ipv4: Cache learned redirect information in inetpeer.)
removed some ICMP packet validations which are required by
RFC 1122, section 3.2.2.2:
...
  A Redirect message SHOULD be silently discarded if the new
  gateway address it specifies is not on the same connected
  (sub-) net through which the Redirect arrived [INTRO:2,
  Appendix A], or if the source of the Redirect is not the
  current first-hop gateway for the specified destination (see
  Section 3.3.1).

Signed-off-by: Flavio Leitner <redacted>

The reason for putting this into the inetpeer cache was so that we
didn't need to consult the routing cache at all.  We're working to
remove it at some point, so every dependency matters.

Can you implement this such that only an inetpeer cache probe is
necessary?

Sure, I have reviewed your patch series to remove the routing
cache and I believe this version works with and without it, though
I have tested only with current net-next code.

Thanks for your time reviewing, I appreciate it.
fbl

Signed-off-by: Flavio Leitner <redacted>
---
 net/ipv4/route.c |   43 ++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 26c77e1..1a639b9 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c

@@ -1308,8 +1308,14 @@ static void rt_del(unsigned hash, struct rtable *rt)
 void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw,
 		    __be32 saddr, struct net_device *dev)
 {
+	int s, i;
 	struct in_device *in_dev = __in_dev_get_rcu(dev);
+	struct rtable *rt;
+	__be32 skeys[2] = { saddr, 0 };
+	int    ikeys[2] = { dev->ifindex, 0 };
+	struct flowi4 fl4;
 	struct inet_peer *peer;
+	bool   putpeer = false;
 	struct net *net;
 
 	if (!in_dev)

@@ -1331,13 +1337,40 @@ void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw,
 			goto reject_redirect;
 	}
 
-	peer = inet_getpeer_v4(daddr, 1);
-	if (peer) {
-		peer->redirect_learned.a4 = new_gw;
+	memset(&fl4, 0, sizeof(fl4));
+	fl4.daddr = daddr;
+	for (s = 0; s < 2; s++) {
+		for (i = 0; i < 2; i++) {
+			fl4.flowi4_oif = ikeys[i];
+			fl4.saddr = skeys[s];
+			rt = __ip_route_output_key(net, &fl4);
+			if (IS_ERR(rt))
+				continue;
 
-		inet_putpeer(peer);
+			if (rt->dst.error || rt->dst.dev != dev ||
+			    rt->rt_gateway != old_gw) {
+				ip_rt_put(rt);
+				continue;
+			}
 
-		atomic_inc(&__rt_peer_genid);
+			peer = rt->peer;
+			if (!peer) {
+				peer = inet_getpeer_v4(daddr, 1);
+				putpeer = true;
+			}
+
+			if (peer) {
+				peer->redirect_learned.a4 = new_gw;
+
+				if (putpeer)
+					inet_putpeer(peer);
+
+				atomic_inc(&__rt_peer_genid);
+			}
+
+			ip_rt_put(rt);
+			return;
+		}
 	}
 	return;
 

-- 
1.7.6

On Wed, 19 Oct 2011 16:05:37 -0200
Flavio Leitner [off-list ref] wrote:

On Mon, 17 Oct 2011 19:43:44 -0400 (EDT)
David Miller [off-list ref] wrote:

quoted

From: Flavio Leitner <redacted>
Date: Wed,  5 Oct 2011 11:20:04 -0300

quoted

The commit f39925dbde7788cfb96419c0f092b086aa325c0f
(ipv4: Cache learned redirect information in inetpeer.)
removed some ICMP packet validations which are required by
RFC 1122, section 3.2.2.2:

The reason for putting this into the inetpeer cache was so that we
didn't need to consult the routing cache at all.  We're working to
remove it at some point, so every dependency matters.

Can you implement this such that only an inetpeer cache probe is
necessary?

Sure, I have reviewed your patch series to remove the routing
cache and I believe this version works with and without it, though
I have tested only with current net-next code.

Thanks for your time reviewing, I appreciate it.

...

I was reviewing this again and instead of doing the above, it would
be better to use rt_bind_peer() to update rt->peer as well.

                        if (!rt->peer)
                                rt_bind_peer(rt, rt->rt_dst, 1);

                        peer = rt->peer;
                        if (peer) {
                                peer->redirect_learned.a4 = new_gw;
                                atomic_inc(&__rt_peer_genid);
                        }


but I am not sure if I understood you completely when you say
to do such that only an inetpeer cache probe is necessary.

thanks again,
fbl

From: Flavio Leitner <redacted>
Date: Thu, 20 Oct 2011 15:47:02 -0200

I was reviewing this again and instead of doing the above, it would
be better to use rt_bind_peer() to update rt->peer as well.

                        if (!rt->peer)
                                rt_bind_peer(rt, rt->rt_dst, 1);

                        peer = rt->peer;
                        if (peer) {
                                peer->redirect_learned.a4 = new_gw;
                                atomic_inc(&__rt_peer_genid);
                        }


but I am not sure if I understood you completely when you say
to do such that only an inetpeer cache probe is necessary.

If you have the route entry available already and you're doing the
inetpeer lookup anyways, you might as well use rt_bind_peer() since
all of the expensive work has to be done anyways.

So yes, using rt_bind_peer() would be the best thing to do here.

On Thu, 20 Oct 2011 16:19:29 -0400 (EDT)
David Miller [off-list ref] wrote:

From: Flavio Leitner <redacted>
Date: Thu, 20 Oct 2011 15:47:02 -0200

quoted

I was reviewing this again and instead of doing the above, it would
be better to use rt_bind_peer() to update rt->peer as well.

                        if (!rt->peer)
                                rt_bind_peer(rt, rt->rt_dst, 1);

                        peer = rt->peer;
                        if (peer) {
                                peer->redirect_learned.a4 = new_gw;
                                atomic_inc(&__rt_peer_genid);
                        }


but I am not sure if I understood you completely when you say
to do such that only an inetpeer cache probe is necessary.

If you have the route entry available already and you're doing the
inetpeer lookup anyways, you might as well use rt_bind_peer() since
all of the expensive work has to be done anyways.

So yes, using rt_bind_peer() would be the best thing to do here.

just posted patch v3. iirc, you prefer to receive patches as new
posts rather than replies to old threads.
"Subject: [PATCH net-next v3] route: fix ICMP redirect validation"

thanks again,
fbl