* [PATCH] route: fix ICMP redirect validation
@ 2011-10-05 14:20 Flavio Leitner
2011-10-17 23:43 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Flavio Leitner @ 2011-10-05 14:20 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Flavio Leitner
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 <fbl@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] route: fix ICMP redirect validation
2011-10-05 14:20 [PATCH] route: fix ICMP redirect validation Flavio Leitner
@ 2011-10-17 23:43 ` David Miller
2011-10-19 18:05 ` Flavio Leitner
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2011-10-17 23:43 UTC (permalink / raw)
To: fbl; +Cc: netdev
From: Flavio Leitner <fbl@redhat.com>
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 <fbl@redhat.com>
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] route: fix ICMP redirect validation
2011-10-17 23:43 ` David Miller
@ 2011-10-19 18:05 ` Flavio Leitner
2011-10-20 17:47 ` Flavio Leitner
0 siblings, 1 reply; 6+ messages in thread
From: Flavio Leitner @ 2011-10-19 18:05 UTC (permalink / raw)
To: David Miller; +Cc: netdev
On Mon, 17 Oct 2011 19:43:44 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:
> From: Flavio Leitner <fbl@redhat.com>
> 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 <fbl@redhat.com>
>
> 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 <fbl@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] route: fix ICMP redirect validation
2011-10-19 18:05 ` Flavio Leitner
@ 2011-10-20 17:47 ` Flavio Leitner
2011-10-20 20:19 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Flavio Leitner @ 2011-10-20 17:47 UTC (permalink / raw)
To: Flavio Leitner; +Cc: David Miller, netdev
On Wed, 19 Oct 2011 16:05:37 -0200
Flavio Leitner <fbl@redhat.com> wrote:
> On Mon, 17 Oct 2011 19:43:44 -0400 (EDT)
> David Miller <davem@davemloft.net> wrote:
>
> > From: Flavio Leitner <fbl@redhat.com>
> > 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:
> >
> > 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.
...
> @@ -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;
> + }
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] route: fix ICMP redirect validation
2011-10-20 17:47 ` Flavio Leitner
@ 2011-10-20 20:19 ` David Miller
2011-10-21 18:13 ` Flavio Leitner
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2011-10-20 20:19 UTC (permalink / raw)
To: fbl; +Cc: netdev
From: Flavio Leitner <fbl@redhat.com>
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] route: fix ICMP redirect validation
2011-10-20 20:19 ` David Miller
@ 2011-10-21 18:13 ` Flavio Leitner
0 siblings, 0 replies; 6+ messages in thread
From: Flavio Leitner @ 2011-10-21 18:13 UTC (permalink / raw)
To: David Miller; +Cc: netdev
On Thu, 20 Oct 2011 16:19:29 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:
> From: Flavio Leitner <fbl@redhat.com>
> 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.
>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-10-21 18:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-05 14:20 [PATCH] route: fix ICMP redirect validation Flavio Leitner
2011-10-17 23:43 ` David Miller
2011-10-19 18:05 ` Flavio Leitner
2011-10-20 17:47 ` Flavio Leitner
2011-10-20 20:19 ` David Miller
2011-10-21 18:13 ` Flavio Leitner
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).