Unencrypted packets after SNAT, allthough IPSEC-Policies are present

BUG REPORT Konstantinos Kolelis 2 participants 8 messages Sep 10, 2014 – Sep 16, 2014

Subsystems (4)

Networking Network drivers IPv4/IPv6 Networking IPsec

lore

Share & tools

lore

Konstantinos Kolelis

author

2014-09-10 17:46 UTC #0 lore raw

Hi all,

i' ve observed a problem with xfrm lookups, SNAT, blackhole route and missing SAs.
The problem occures with all Kernels above 3.6.x and might has to do with the changes in
ip4_blackhole_route() function in net/route.c.

Let say you have two network interfaces:
eth0 with ip 172.16.0.10/24
and
eth1 with ip 192.168.0.1/24

and you have done the following configuration:

iptables -t nat -A POSTROUTING -s 192.168.0.0/24 -j SNAT --to-source 172.16.0.10

and

ip xfrm policy add dir out src 172.16.0.10 dst 0.0.0.0/0 tmpl proto esp src 172.16.0.10 dst 172.31.0.10 mode tunnel

with the following routes:
default via 172.16.0.1 dev eth0 proto static 172.16.0.0/24 dev eth0 proto kernel scope link src 172.16.0.10 192.168.0.0/24 dev eth1 proto kernel scope link src 192.168.0.1

If for what ever reason IPSEC-SAs can not be established, maybe because 172.31.0.10 is down,
the traffic comming from 192.168.0.0/24 will leave unencrypted the external (eth0) interface.

I can see that the traffic is source-Nated correctly and xfrm_me_harder() is called.
Also i can see that a xfrm_bundle can not be created so make_blackhole_route()
and ip4_blackhole_route() is called. But the callback for dst_output() is never called afterwards.

The following Patch is workaround which restores the expected behavior. It should work for Kernel 3.6.x and higher.

net/ipv4/ip_output.c 2014-09-10 16:27:12.287893706 +0200 renamed from net/ipv4/ip_output.c 2014-09-06 01:37:11.000000000 +0200 +3 -0

diff -rupN a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
@@ -260,6 +260,9 @@ static int ip_finish_output(struct sk_bu
     if (skb_dst(skb)->xfrm != NULL) {
         IPCB(skb)->flags |= IPSKB_REROUTED;
         return dst_output(skb);
+    } else if (skb_dst(skb)->error == -EINVAL) {
+        IPCB(skb)->flags |= IPSKB_REROUTED;
+        return dst_output(skb);
     }
 #endif
     if (skb_is_gso(skb))

net/ipv4/route.c 2014-09-10 16:13:07.179847637 +0200 renamed from net/ipv4/route.c 2014-09-06 01:37:11.000000000 +0200 +1 -0

diff -rupN a/net/ipv4/route.c b/net/ipv4/route.c
@@ -2231,6 +2231,7 @@ struct dst_entry *ipv4_blackhole_route(s
         struct dst_entry *new = &rt->dst;
 
         new->__use = 1;
+        new->error = -EINVAL;
         new->input = dst_discard;
         new->output = dst_discard_sk;

Please let me know if you need more details.

Best regards,

Kosta

Steffen Klassert

maintainer

2014-09-11 11:54 UTC #1 lore raw

Quoted 7 lines · Konstantinos Kolelis

Hi all,
i' ve observed a problem with xfrm lookups, SNAT, blackhole route and missing SAs.
The problem occures with all Kernels above 3.6.x and might has to do with the changes in
ip4_blackhole_route() function in net/route.c.

Thanks for the report!

Is kernel v3.6 the first kernel with this issue? It seems that we have this problem already longer, at least if my analysis is correct.

Quoted 21 lines

Let say you have two network interfaces:
eth0 with ip 172.16.0.10/24
and
eth1 with ip 192.168.0.1/24
and you have done the following configuration:
iptables -t nat -A POSTROUTING -s 192.168.0.0/24 -j SNAT --to-source 172.16.0.10
and
ip xfrm policy add dir out src 172.16.0.10 dst 0.0.0.0/0 tmpl proto esp src 172.16.0.10 dst 172.31.0.10 mode tunnel
with the following routes:
default via 172.16.0.1 dev eth0 proto static 172.16.0.0/24 dev eth0 proto kernel scope link src 172.16.0.10 192.168.0.0/24 dev eth1 proto kernel scope link src 192.168.0.1

Quoted 4 lines

If for what ever reason IPSEC-SAs can not be established, maybe because 172.31.0.10 is down,
the traffic comming from 192.168.0.0/24 will leave unencrypted the external (eth0) interface.

I can reproduce it with SNAT and MASQUERADE. Looks like this was introduced back in 2011 with git commit 2774c131 ("xfrm: Handle blackhole route creation via afinfo.").

Before that commit, xfrm_lookup() and __xfrm_lookup() returned an error if we have a matching policy but no states. The route lookup functions used __xfrm_lookup() and generated a blackhole route if __xfrm_lookup() returned -EREMOTE. All other functions used xfrm_lookup() which returned -EAGAIN. This was treated as as an error and the packet was dropped immediately.

After this commit all callers to xfrm_lookup() rely that dst_output() is called afterwards. This seems to be not the case, at least when postrouting nat is used.

Maybe we should go back to let only the route lookup functions genarate a blackhole route. Everyone else should better drop the packets immediately.

I'll try to do a patch.

Konstantinos Kolelis

author

2014-09-11 13:13 UTC #2 lore raw

Am 11.09.2014 13:54, schrieb Steffen Klassert:

Quoted 9 lines · Konstantinos Kolelis

Quoted 7 lines
Hi all,
i' ve observed a problem with xfrm lookups, SNAT, blackhole route and missing SAs.
The problem occures with all Kernels above 3.6.x and might has to do with the changes in
ip4_blackhole_route() function in net/route.c.

Quoted 6 lines

Thanks for the report!
Is kernel v3.6 the first kernel with this issue? It seems that we have this problem already longer, at least if my analysis is correct.

It worked until Kernel 3.4.103, i did not check with v3.5 though.

Quoted 41 lines

Quoted 21 lines
Let say you have two network interfaces:
eth0 with ip 172.16.0.10/24
and
eth1 with ip 192.168.0.1/24
and you have done the following configuration:
iptables -t nat -A POSTROUTING -s 192.168.0.0/24 -j SNAT --to-source 172.16.0.10
and
ip xfrm policy add dir out src 172.16.0.10 dst 0.0.0.0/0 tmpl proto esp src 172.16.0.10 dst 172.31.0.10 mode tunnel
with the following routes:
default via 172.16.0.1 dev eth0 proto static 172.16.0.0/24 dev eth0 proto kernel scope link src 172.16.0.10 192.168.0.0/24 dev eth1 proto kernel scope link src 192.168.0.1
Quoted 4 lines
If for what ever reason IPSEC-SAs can not be established, maybe because 172.31.0.10 is down,
the traffic comming from 192.168.0.0/24 will leave unencrypted the external (eth0) interface.
I can reproduce it with SNAT and MASQUERADE. Looks like this was introduced back in 2011 with git commit 2774c131 ("xfrm: Handle blackhole route creation via afinfo.").
Before that commit, xfrm_lookup() and __xfrm_lookup() returned an error if we have a matching policy but no states. The route lookup functions used __xfrm_lookup() and generated a blackhole route if __xfrm_lookup() returned -EREMOTE. All other functions used xfrm_lookup() which returned -EAGAIN. This was treated as as an error and the packet was dropped immediately.
After this commit all callers to xfrm_lookup() rely that dst_output() is called afterwards. This seems to be not the case, at least when postrouting nat is used.

Quoted 6 lines

Maybe we should go back to let only the route lookup functions genarate a blackhole route. Everyone else should better drop the packets immediately.
I'll try to do a patch.

You should also check what happens if xfrm_larval_drop is false. Allthough blackhole route was not used, you could still run into the same problem. It just took a while longer.

Steffen Klassert

maintainer

2014-09-12 09:31 UTC #3 lore raw

Quoted 10 lines · Konstantinos Kolelis

Am 11.09.2014 13:54, schrieb Steffen Klassert:
Quoted 9 lines · Konstantinos Kolelis
Quoted 7 lines
Hi all,
i' ve observed a problem with xfrm lookups, SNAT, blackhole route and missing SAs.
The problem occures with all Kernels above 3.6.x and might has to do with the changes in
ip4_blackhole_route() function in net/route.c.

Quoted 8 lines

Quoted 6 lines
Thanks for the report!
Is kernel v3.6 the first kernel with this issue? It seems that we have this problem already longer, at least if my analysis is correct.
It worked until Kernel 3.4.103, i did not check with v3.5 though.

Hm. I thought the problem exists already for longer, so I did a bisect. To my surprise, the following commit introduced the bug:

commit a263b3093641fb1ec377582c90986a7fd0625184

Author: David S. Miller <[email protected]>
Date:   Mon Jul 2 02:02:15 2012 -0700

ipv4: Make neigh lookups directly in output packet path.

Do not use the dst cached neigh, we'll be getting rid of that.

Signed-off-by David S. Miller <[email protected]>

But this is not really the offending commit. As I said, the problem was introduced by git commit 2774c131 ("xfrm: Handle blackhole route creation via afinfo.") because the assumption that dst_output() is called always after a xfrm_lookup() is wrong. On postrouting nat, dst_output() is not called for blackholed packets. But we were in luck because ip_finish_output2() tried to use a dst cached neigh entry. On blackhole routes we did not cache neigh entries, so the packets were dropped in the last moment. The above commit finally opened the door by replacing the usage of a dst cached neigh entry by a direct lookup.

So we need to ensure that a blackhole route is generated only by the route lookup functions.

Quoted 46 lines

Quoted 46 lines
Quoted 22 lines
Let say you have two network interfaces:
eth0 with ip 172.16.0.10/24
and
eth1 with ip 192.168.0.1/24
and you have done the following configuration:
iptables -t nat -A POSTROUTING -s 192.168.0.0/24 -j SNAT --to-source 172.16.0.10
and
ip xfrm policy add dir out src 172.16.0.10 dst 0.0.0.0/0 tmpl proto esp src 172.16.0.10 dst 172.31.0.10 mode tunnel
with the following routes:
default via 172.16.0.1 dev eth0 proto static 172.16.0.0/24 dev eth0 proto kernel scope link src 172.16.0.10 192.168.0.0/24 dev eth1 proto kernel scope link src 192.168.0.1
Quoted 4 lines
If for what ever reason IPSEC-SAs can not be established, maybe because 172.31.0.10 is down,
the traffic comming from 192.168.0.0/24 will leave unencrypted the external (eth0) interface.
I can reproduce it with SNAT and MASQUERADE. Looks like this was introduced back in 2011 with git commit 2774c131 ("xfrm: Handle blackhole route creation via afinfo.").
Before that commit, xfrm_lookup() and __xfrm_lookup() returned an error if we have a matching policy but no states. The route lookup functions used __xfrm_lookup() and generated a blackhole route if __xfrm_lookup() returned -EREMOTE. All other functions used xfrm_lookup() which returned -EAGAIN. This was treated as as an error and the packet was dropped immediately.
After this commit all callers to xfrm_lookup() rely that dst_output() is called afterwards. This seems to be not the case, at least when postrouting nat is used.
Maybe we should go back to let only the route lookup functions genarate a blackhole route. Everyone else should better drop the packets immediately.

Quoted 7 lines

Quoted 2 lines
I'll try to do a patch.
You should also check what happens if xfrm_larval_drop is false. Allthough blackhole route was not used, you could still run into the same problem. It just took a while longer.

Yes, I'm aware of that. Thanks for the hint!

Steffen Klassert

maintainer

2014-09-15 08:09 UTC #4 lore raw

Quoted 44 lines · Steffen Klassert

Quoted 10 lines · Konstantinos Kolelis
Am 11.09.2014 13:54, schrieb Steffen Klassert:
Quoted 9 lines · Konstantinos Kolelis
Quoted 7 lines
Hi all,
i' ve observed a problem with xfrm lookups, SNAT, blackhole route and missing SAs.
The problem occures with all Kernels above 3.6.x and might has to do with the changes in
ip4_blackhole_route() function in net/route.c.
Quoted 8 lines
Quoted 6 lines
Thanks for the report!
Is kernel v3.6 the first kernel with this issue? It seems that we have this problem already longer, at least if my analysis is correct.
It worked until Kernel 3.4.103, i did not check with v3.5 though.
Hm. I thought the problem exists already for longer, so I did a bisect. To my surprise, the following commit introduced the bug:
commit a263b3093641fb1ec377582c90986a7fd0625184
Author: David S. Miller <[email protected]>
Date:   Mon Jul 2 02:02:15 2012 -0700
ipv4: Make neigh lookups directly in output packet path.
Do not use the dst cached neigh, we'll be getting rid of that.
Signed-off-by David S. Miller <[email protected]>
But this is not really the offending commit. As I said, the problem was introduced by git commit 2774c131 ("xfrm: Handle blackhole route creation via afinfo.") because the assumption that dst_output() is called always after a xfrm_lookup() is wrong. On postrouting nat, dst_output() is not called for blackholed packets. But we were in luck because ip_finish_output2() tried to use a dst cached neigh entry. On blackhole routes we did not cache neigh entries, so the packets were dropped in the last moment. The above commit finally opened the door by replacing the usage of a dst cached neigh entry by a direct lookup.

Quoted 2 lines

So we need to ensure that a blackhole route is generated only by the route lookup functions.

Can you please try the patch below? This should fix the default case where xfrm_larval_drop is true. The xfrm_larval_drop false case needs a separate fix.

Subject: [PATCH] xfrm: Generate blackhole routes only from route lookup
functions

Currently we genarate a blackhole route route whenever we have matching policies but can not resolve the states. Here we assume that dst_output() is called to kill the balckholed packets. Unfortunately this assumption is not true in all cases, so it is possible that these packets leave the system unwanted.

We fix this by generating blackhole routes only from the route lookup functions, here we can guarantee a call to dst_output() afterwards.

Fixes 2774c131b1d ("xfrm: Handle blackhole route creation via afinfo.")

Reported-by Konstantinos Kolelis <[email protected]>

Signed-off-by Steffen Klassert <[email protected]>

---
 include/net/dst.h      | 15 ++++++++++++++-
 net/ipv4/route.c       |  6 +++---
 net/ipv6/ip6_output.c  |  4 ++--
 net/xfrm/xfrm_policy.c | 16 +++++++++++++++-
 4 files changed, 34 insertions(+), 7 deletions(-)

include/net/dst.h +14 -1

index 71c60f4..fa11c90 100644
@@ -490,7 +490,16 @@ static inline struct dst_entry *xfrm_lookup(struct net *net,
 					    int flags)
 {
 	return dst_orig;
-} 
+}
+
+static inline struct dst_entry *xfrm_lookup_route(struct net *net,
+						  struct dst_entry *dst_orig,
+						  const struct flowi *fl,
+						  struct sock *sk,
+						  int flags)
+{
+	return dst_orig;
+}
 
 static inline struct xfrm_state *dst_xfrm(const struct dst_entry *dst)
 {

@@ -502,6 +511,10 @@ struct dst_entry *xfrm_lookup(struct net *net, struct dst_entry *dst_orig,
 			      const struct flowi *fl, struct sock *sk,
 			      int flags);
 
+struct dst_entry *xfrm_lookup_route(struct net *net, struct dst_entry *dst_orig,
+				    const struct flowi *fl, struct sock *sk,
+				    int flags);
+
 /* skb attached with this dst needs transformation if dst->xfrm is valid */
 static inline struct xfrm_state *dst_xfrm(const struct dst_entry *dst)
 {

net/ipv4/route.c +3 -3

index eaa4b00..173e7ea 100644
@@ -2265,9 +2265,9 @@ struct rtable *ip_route_output_flow(struct net *net, struct flowi4 *flp4,
 		return rt;
 
 	if (flp4->flowi4_proto)
-		rt = (struct rtable *) xfrm_lookup(net, &rt->dst,
-						   flowi4_to_flowi(flp4),
-						   sk, 0);
+		rt = (struct rtable *)xfrm_lookup_route(net, &rt->dst,
+							flowi4_to_flowi(flp4),
+							sk, 0);
 
 	return rt;
 }

net/ipv6/ip6_output.c +2 -2

index 315a55d..0a3448b 100644
@@ -1009,7 +1009,7 @@ struct dst_entry *ip6_dst_lookup_flow(struct sock *sk, struct flowi6 *fl6,
 	if (final_dst)
 		fl6->daddr = *final_dst;
 
-	return xfrm_lookup(sock_net(sk), dst, flowi6_to_flowi(fl6), sk, 0);
+	return xfrm_lookup_route(sock_net(sk), dst, flowi6_to_flowi(fl6), sk, 0);
 }
 EXPORT_SYMBOL_GPL(ip6_dst_lookup_flow);

@@ -1041,7 +1041,7 @@ struct dst_entry *ip6_sk_dst_lookup_flow(struct sock *sk, struct flowi6 *fl6,
 	if (final_dst)
 		fl6->daddr = *final_dst;
 
-	return xfrm_lookup(sock_net(sk), dst, flowi6_to_flowi(fl6), sk, 0);
+	return xfrm_lookup_route(sock_net(sk), dst, flowi6_to_flowi(fl6), sk, 0);
 }
 EXPORT_SYMBOL_GPL(ip6_sk_dst_lookup_flow);

net/xfrm/xfrm_policy.c +15 -1

index beeed60..e041822 100644
@@ -2138,7 +2138,8 @@ struct dst_entry *xfrm_lookup(struct net *net, struct dst_entry *dst_orig,
 			xfrm_pols_put(pols, drop_pols);
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTNOSTATES);
 
-			return make_blackhole(net, family, dst_orig);
+			err = -EREMOTE;
+			goto error;
 		}
 
 		err = -EAGAIN;

@@ -2195,6 +2196,19 @@ dropdst:
 }
 EXPORT_SYMBOL(xfrm_lookup);
 
+struct dst_entry *xfrm_lookup_route(struct net *net, struct dst_entry *dst_orig,
+				    const struct flowi *fl,
+				    struct sock *sk, int flags)
+{
+	struct dst_entry *dst = xfrm_lookup(net, dst_orig, fl, sk, flags);
+
+	if (IS_ERR(dst) && PTR_ERR(dst) == -EREMOTE)
+		return make_blackhole(net, dst_orig->ops->family, dst_orig);
+
+	return dst;
+}
+EXPORT_SYMBOL(xfrm_lookup_route);
+
 static inline int
 xfrm_secpath_reject(int idx, struct sk_buff *skb, const struct flowi *fl)
 {

Signature · 1 line

1.9.1

Steffen Klassert

maintainer

2014-09-15 12:04 UTC #5 lore raw

net/xfrm/xfrm_policy.c struct dst_entry *xfrm_lookup(struct net *net, struct dst_entry *dst_orig, · line 2142

Quoted 12 lines · Steffen Klassert

net/xfrm/xfrm_policy.c +2 -1

index beeed60..e041822 100644
@@ -2138,7 +2138,8 @@ struct dst_entry *xfrm_lookup(struct net *net, struct dst_entry *dst_orig,
 			xfrm_pols_put(pols, drop_pols);
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTNOSTATES);
 
-			return make_blackhole(net, family, dst_orig);
+			err = -EREMOTE;
+			goto error;

We must return here, otherwise we drop some refcounts too much. I'll send an updated patch tomorrow.

Steffen Klassert

maintainer

2014-09-16 07:30 UTC #6 lore raw

Quoted 10 lines · Steffen Klassert

Quoted 9 lines · Steffen Klassert

net/xfrm/xfrm_policy.c +0 -0

index beeed60..e041822 100644
@@ -2138,7 +2138,8 @@ struct dst_entry *xfrm_lookup(struct net *net, struct dst_entry *dst_orig,
 			xfrm_pols_put(pols, drop_pols);
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTNOSTATES);

Quoted 7 lines

net/xfrm/xfrm_policy.c struct dst_entry *xfrm_lookup(struct net *net, struct dst_entry *dst_orig, · line 2142
Quoted 3 lines
net/xfrm/xfrm_policy.c +2 -1
-			return make_blackhole(net, family, dst_orig);
+			err = -EREMOTE;
+			goto error;
We must return here, otherwise we drop some refcounts too much. I'll send an updated patch tomorrow.

Below is the updated version of the patch.

Subject: [PATCH] xfrm: Generate blackhole routes only from route lookup
functions

We fix this by generating blackhole routes only from the route lookup functions, here we can guarantee a call to dst_output() afterwards.

Fixes 2774c131b1d ("xfrm: Handle blackhole route creation via afinfo.")

Reported-by Konstantinos Kolelis <[email protected]>

Signed-off-by Steffen Klassert <[email protected]>

---
 include/net/dst.h      | 15 ++++++++++++++-
 net/ipv4/route.c       |  6 +++---
 net/ipv6/ip6_output.c  |  4 ++--
 net/xfrm/xfrm_policy.c | 18 +++++++++++++++++-
 4 files changed, 36 insertions(+), 7 deletions(-)

include/net/dst.h +14 -1

index 71c60f4..fa11c90 100644
@@ -490,7 +490,16 @@ static inline struct dst_entry *xfrm_lookup(struct net *net,
 					    int flags)
 {
 	return dst_orig;
-} 
+}
+
+static inline struct dst_entry *xfrm_lookup_route(struct net *net,
+						  struct dst_entry *dst_orig,
+						  const struct flowi *fl,
+						  struct sock *sk,
+						  int flags)
+{
+	return dst_orig;
+}
 
 static inline struct xfrm_state *dst_xfrm(const struct dst_entry *dst)
 {

@@ -502,6 +511,10 @@ struct dst_entry *xfrm_lookup(struct net *net, struct dst_entry *dst_orig,
 			      const struct flowi *fl, struct sock *sk,
 			      int flags);
 
+struct dst_entry *xfrm_lookup_route(struct net *net, struct dst_entry *dst_orig,
+				    const struct flowi *fl, struct sock *sk,
+				    int flags);
+
 /* skb attached with this dst needs transformation if dst->xfrm is valid */
 static inline struct xfrm_state *dst_xfrm(const struct dst_entry *dst)
 {

net/ipv4/route.c +3 -3

index eaa4b00..173e7ea 100644
@@ -2265,9 +2265,9 @@ struct rtable *ip_route_output_flow(struct net *net, struct flowi4 *flp4,
 		return rt;
 
 	if (flp4->flowi4_proto)
-		rt = (struct rtable *) xfrm_lookup(net, &rt->dst,
-						   flowi4_to_flowi(flp4),
-						   sk, 0);
+		rt = (struct rtable *)xfrm_lookup_route(net, &rt->dst,
+							flowi4_to_flowi(flp4),
+							sk, 0);
 
 	return rt;
 }

net/ipv6/ip6_output.c +2 -2

index 315a55d..0a3448b 100644
@@ -1009,7 +1009,7 @@ struct dst_entry *ip6_dst_lookup_flow(struct sock *sk, struct flowi6 *fl6,
 	if (final_dst)
 		fl6->daddr = *final_dst;
 
-	return xfrm_lookup(sock_net(sk), dst, flowi6_to_flowi(fl6), sk, 0);
+	return xfrm_lookup_route(sock_net(sk), dst, flowi6_to_flowi(fl6), sk, 0);
 }
 EXPORT_SYMBOL_GPL(ip6_dst_lookup_flow);

@@ -1041,7 +1041,7 @@ struct dst_entry *ip6_sk_dst_lookup_flow(struct sock *sk, struct flowi6 *fl6,
 	if (final_dst)
 		fl6->daddr = *final_dst;
 
-	return xfrm_lookup(sock_net(sk), dst, flowi6_to_flowi(fl6), sk, 0);
+	return xfrm_lookup_route(sock_net(sk), dst, flowi6_to_flowi(fl6), sk, 0);
 }
 EXPORT_SYMBOL_GPL(ip6_sk_dst_lookup_flow);

net/xfrm/xfrm_policy.c +17 -1

index beeed60..7505674 100644
@@ -2138,7 +2138,7 @@ struct dst_entry *xfrm_lookup(struct net *net, struct dst_entry *dst_orig,
 			xfrm_pols_put(pols, drop_pols);
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTNOSTATES);
 
-			return make_blackhole(net, family, dst_orig);
+			return ERR_PTR(-EREMOTE);
 		}
 
 		err = -EAGAIN;

@@ -2195,6 +2195,22 @@ dropdst:
 }
 EXPORT_SYMBOL(xfrm_lookup);
 
+/* Callers of xfrm_lookup_route() must ensure a call to dst_output().
+ * Otherwise we may send out blackholed packets.
+ */
+struct dst_entry *xfrm_lookup_route(struct net *net, struct dst_entry *dst_orig,
+				    const struct flowi *fl,
+				    struct sock *sk, int flags)
+{
+	struct dst_entry *dst = xfrm_lookup(net, dst_orig, fl, sk, flags);
+
+	if (IS_ERR(dst) && PTR_ERR(dst) == -EREMOTE)
+		return make_blackhole(net, dst_orig->ops->family, dst_orig);
+
+	return dst;
+}
+EXPORT_SYMBOL(xfrm_lookup_route);
+
 static inline int
 xfrm_secpath_reject(int idx, struct sk_buff *skb, const struct flowi *fl)
 {

Signature · 1 line

1.9.1

Steffen Klassert

maintainer

2014-09-16 08:39 UTC #7 lore raw

In reply to Konstantinos Kolelis

Quoted 5 lines · Konstantinos Kolelis

You should also check what happens if xfrm_larval_drop is false. Allthough blackhole route was not used, you could still run into the same problem. It just took a while longer.

This one should fix the xfrm_larval_drop false case. It applies on top of my previous patch. Both patches are currenty in the ipsec testing branch at:

git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git testing

Subject: [PATCH] xfrm: Generate queueing routes only from route lookup
functions

Currently we genarate a queueing route if we have matching policies but can not resolve the states and the sysctl xfrm_larval_drop is disabled. Here we assume that dst_output() is called to kill the queued packets. Unfortunately this assumption is not true in all cases, so it is possible that these packets leave the system unwanted.

We fix this by generating queueing routes only from the route lookup functions, here we can guarantee a call to dst_output() afterwards.

Fixes a0073fe18e71 ("xfrm: Add a state resolution packet queue")

Reported-by Konstantinos Kolelis <[email protected]>

Signed-off-by Steffen Klassert <[email protected]>

---
 include/net/dst.h      |  1 +
 net/xfrm/xfrm_policy.c | 32 ++++++++++++++++++++++++--------
 2 files changed, 25 insertions(+), 8 deletions(-)

include/net/dst.h +1 -0

index fa11c90..a8ae4e7 100644
@@ -480,6 +480,7 @@ void dst_init(void);
 /* Flags for xfrm_lookup flags argument. */
 enum {
 	XFRM_LOOKUP_ICMP = 1 << 0,
+	XFRM_LOOKUP_QUEUE = 1 << 1,
 };
 
 struct flowi;

net/xfrm/xfrm_policy.c +24 -8

index 7505674..fdde51f 100644
@@ -39,6 +39,11 @@
 #define XFRM_QUEUE_TMO_MAX ((unsigned)(60*HZ))
 #define XFRM_MAX_QUEUE_LEN	100
 
+struct xfrm_flo {
+	struct dst_entry *dst_orig;
+	u8 flags;
+};
+
 static DEFINE_SPINLOCK(xfrm_policy_afinfo_lock);
 static struct xfrm_policy_afinfo __rcu *xfrm_policy_afinfo[NPROTO]
 						__read_mostly;

@@ -1877,13 +1882,14 @@ static int xdst_queue_output(struct sock *sk, struct sk_buff *skb)
 }
 
 static struct xfrm_dst *xfrm_create_dummy_bundle(struct net *net,
-						 struct dst_entry *dst,
+						 struct xfrm_flo *xflo,
 						 const struct flowi *fl,
 						 int num_xfrms,
 						 u16 family)
 {
 	int err;
 	struct net_device *dev;
+	struct dst_entry *dst;
 	struct dst_entry *dst1;
 	struct xfrm_dst *xdst;

@@ -1891,9 +1897,12 @@ static struct xfrm_dst *xfrm_create_dummy_bundle(struct net *net,
 	if (IS_ERR(xdst))
 		return xdst;
 
-	if (net->xfrm.sysctl_larval_drop || num_xfrms <= 0)
+	if (!(xflo->flags & XFRM_LOOKUP_QUEUE) ||
+	    net->xfrm.sysctl_larval_drop ||
+	    num_xfrms <= 0)
 		return xdst;
 
+	dst = xflo->dst_orig;
 	dst1 = &xdst->u.dst;
 	dst_hold(dst);
 	xdst->route = dst;

@@ -1935,7 +1944,7 @@ static struct flow_cache_object *
 xfrm_bundle_lookup(struct net *net, const struct flowi *fl, u16 family, u8 dir,
 		   struct flow_cache_object *oldflo, void *ctx)
 {
-	struct dst_entry *dst_orig = (struct dst_entry *)ctx;
+	struct xfrm_flo *xflo = (struct xfrm_flo *)ctx;
 	struct xfrm_policy *pols[XFRM_POLICY_TYPE_MAX];
 	struct xfrm_dst *xdst, *new_xdst;
 	int num_pols = 0, num_xfrms = 0, i, err, pol_dead;

@@ -1976,7 +1985,8 @@ xfrm_bundle_lookup(struct net *net, const struct flowi *fl, u16 family, u8 dir,
 			goto make_dummy_bundle;
 	}
 
-	new_xdst = xfrm_resolve_and_create_bundle(pols, num_pols, fl, family, dst_orig);
+	new_xdst = xfrm_resolve_and_create_bundle(pols, num_pols, fl, family,
+						  xflo->dst_orig);
 	if (IS_ERR(new_xdst)) {
 		err = PTR_ERR(new_xdst);
 		if (err != -EAGAIN)

@@ -2010,7 +2020,7 @@ make_dummy_bundle:
 	/* We found policies, but there's no bundles to instantiate:
 	 * either because the policy blocks, has no transformations or
 	 * we could not build template (no xfrm_states).*/
-	xdst = xfrm_create_dummy_bundle(net, dst_orig, fl, num_xfrms, family);
+	xdst = xfrm_create_dummy_bundle(net, xflo, fl, num_xfrms, family);
 	if (IS_ERR(xdst)) {
 		xfrm_pols_put(pols, num_pols);
 		return ERR_CAST(xdst);

@@ -2104,13 +2114,18 @@ struct dst_entry *xfrm_lookup(struct net *net, struct dst_entry *dst_orig,
 	}
 
 	if (xdst == NULL) {
+		struct xfrm_flo xflo;
+
+		xflo.dst_orig = dst_orig;
+		xflo.flags = flags;
+
 		/* To accelerate a bit...  */
 		if ((dst_orig->flags & DST_NOXFRM) ||
 		    !net->xfrm.policy_count[XFRM_POLICY_OUT])
 			goto nopol;
 
 		flo = flow_cache_lookup(net, fl, family, dir,
-					xfrm_bundle_lookup, dst_orig);
+					xfrm_bundle_lookup, &xflo);
 		if (flo == NULL)
 			goto nopol;
 		if (IS_ERR(flo)) {

@@ -2202,7 +2217,8 @@ struct dst_entry *xfrm_lookup_route(struct net *net, struct dst_entry *dst_orig,
 				    const struct flowi *fl,
 				    struct sock *sk, int flags)
 {
-	struct dst_entry *dst = xfrm_lookup(net, dst_orig, fl, sk, flags);
+	struct dst_entry *dst = xfrm_lookup(net, dst_orig, fl, sk,
+					    flags | XFRM_LOOKUP_QUEUE);
 
 	if (IS_ERR(dst) && PTR_ERR(dst) == -EREMOTE)
 		return make_blackhole(net, dst_orig->ops->family, dst_orig);

@@ -2476,7 +2492,7 @@ int __xfrm_route_forward(struct sk_buff *skb, unsigned short family)
 
 	skb_dst_force(skb);
 
-	dst = xfrm_lookup(net, skb_dst(skb), &fl, NULL, 0);
+	dst = xfrm_lookup(net, skb_dst(skb), &fl, NULL, XFRM_LOOKUP_QUEUE);
 	if (IS_ERR(dst)) {
 		res = 0;
 		dst = NULL;

Signature · 1 line

1.9.1