[BUG REPORT] Unencrypted packets after SNAT, allthough IPSEC-Policies are present

[BUG REPORT] Unencrypted packets after SNAT, allthough IPSEC-Policies are present

[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.

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.


diff -rupN a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
--- a/net/ipv4/ip_output.c    2014-09-06 01:37:11.000000000 +0200
+++ b/net/ipv4/ip_output.c    2014-09-10 16:27:12.287893706 +0200
@@ -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))
diff -rupN a/net/ipv4/route.c b/net/ipv4/route.c
--- a/net/ipv4/route.c    2014-09-06 01:37:11.000000000 +0200
+++ b/net/ipv4/route.c    2014-09-10 16:13:07.179847637 +0200
@@ -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

Re: [BUG REPORT] Unencrypted packets after SNAT, allthough IPSEC-Policies are present

[Steffen Klassert]

On Wed, Sep 10, 2014 at 07:26:53PM +0200, Konstantinos Kolelis wrote:
> 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.

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

Re: [BUG REPORT] Unencrypted packets after SNAT, allthough IPSEC-Policies are present

[Konstantinos Kolelis]

Am 11.09.2014 13:54, schrieb Steffen Klassert:
> On Wed, Sep 10, 2014 at 07:26:53PM +0200, Konstantinos Kolelis wrote:
>> 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.
> 

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

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

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.

Re: [BUG REPORT] Unencrypted packets after SNAT, allthough IPSEC-Policies are present

[Steffen Klassert]

On Thu, Sep 11, 2014 at 03:11:17PM +0200, Konstantinos Kolelis wrote:
> Am 11.09.2014 13:54, schrieb Steffen Klassert:
> > On Wed, Sep 10, 2014 at 07:26:53PM +0200, Konstantinos Kolelis wrote:
> >> 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.
> > 
> 
> 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 <davem@davemloft.net>
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 <davem@davemloft.net>

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.

> 
> >>
> >> 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 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.
> > 
> 
> 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!

Re: [BUG REPORT] Unencrypted packets after SNAT, allthough IPSEC-Policies are present

[Steffen Klassert]

On Fri, Sep 12, 2014 at 11:31:43AM +0200, Steffen Klassert wrote:
> On Thu, Sep 11, 2014 at 03:11:17PM +0200, Konstantinos Kolelis wrote:
> > Am 11.09.2014 13:54, schrieb Steffen Klassert:
> > > On Wed, Sep 10, 2014 at 07:26:53PM +0200, Konstantinos Kolelis wrote:
> > >> 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.
> > > 
> > 
> > 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 <davem@davemloft.net>
> 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 <davem@davemloft.net>
> 
> 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.

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 <k.kolelis@sirrix.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 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(-)

diff --git a/include/net/dst.h b/include/net/dst.h
index 71c60f4..fa11c90 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -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)
 {
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index eaa4b00..173e7ea 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -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;
 }
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 315a55d..0a3448b 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -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);
 
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index beeed60..e041822 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -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)
 {
-- 
1.9.1

Re: [BUG REPORT] Unencrypted packets after SNAT, allthough IPSEC-Policies are present

[Steffen Klassert]

On Mon, Sep 15, 2014 at 10:09:41AM +0200, Steffen Klassert wrote:
>  
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index beeed60..e041822 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -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.

Re: [BUG REPORT] Unencrypted packets after SNAT, allthough IPSEC-Policies are present

[Steffen Klassert]

On Mon, Sep 15, 2014 at 02:04:07PM +0200, Steffen Klassert wrote:
> On Mon, Sep 15, 2014 at 10:09:41AM +0200, Steffen Klassert wrote:
> >  
> > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> > index beeed60..e041822 100644
> > --- a/net/xfrm/xfrm_policy.c
> > +++ b/net/xfrm/xfrm_policy.c
> > @@ -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.
> 

Below is the updated version of the patch.

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 <k.kolelis@sirrix.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 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(-)

diff --git a/include/net/dst.h b/include/net/dst.h
index 71c60f4..fa11c90 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -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)
 {
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index eaa4b00..173e7ea 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -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;
 }
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 315a55d..0a3448b 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -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);
 
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index beeed60..7505674 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -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)
 {
-- 
1.9.1

Re: [BUG REPORT] Unencrypted packets after SNAT, allthough IPSEC-Policies are present

[Steffen Klassert]

On Thu, Sep 11, 2014 at 03:11:17PM +0200, Konstantinos Kolelis wrote:
> 
> 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 <k.kolelis@sirrix.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 include/net/dst.h      |  1 +
 net/xfrm/xfrm_policy.c | 32 ++++++++++++++++++++++++--------
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/include/net/dst.h b/include/net/dst.h
index fa11c90..a8ae4e7 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -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;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 7505674..fdde51f 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -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;
-- 
1.9.1