netdev - Re: [PATCH v2] vrf: Fix conntrack-dnat conflict in vrf-device PREROUTING hook

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6fd2cad0-0340-d4c1-458e-5df3f4d31593@gmail.com>
Date:   Fri, 11 Jan 2019 08:30:00 -0700
From:   David Ahern <dsahern@...il.com>
To:     wenxu@...oud.cn
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH v2] vrf: Fix conntrack-dnat conflict in vrf-device
 PREROUTING hook

On 1/10/19 10:53 PM, wenxu@...oud.cn wrote:
> From: wenxu <wenxu@...oud.cn>
> 
> In the ip_rcv the skb go through the PREROUTING hook first,
> Then jump in vrf device go through the same hook again.
> When conntrack dnat work with vrf, there will be some conflict for rules.
> Because the package go through the hook twice with different nf status
> 
> ip link add user1 type vrf table 1
> ip link add user2 type vrf table 2
> ip l set dev tun1 master user1
> ip l set dev tun2 master user2
> 
> nft add table firewall
> nft add chain firewall zones { type filter hook prerouting  priority - 300 \; }
> nft add rule firewall zones counter ct zone set iif map { "tun1" : 1, "tun2" : 2 }
> nft add chain firewall rule-1000-ingress
> nft add rule firewall rule-1000-ingress ct zone 1 tcp dport 22 ct state new counter accept
> nft add rule firewall rule-1000-ingress counter drop
> nft add chain firewall rule-1000-egress
> nft add rule firewall rule-1000-egress tcp dport 22 ct state new counter drop
> nft add rule firewall rule-1000-egress counter accept
> 
> nft add chain firewall rules-all { type filter hook prerouting priority - 150 \; }
> nft add rule firewall rules-all ip daddr vmap { "2.2.2.11" : jump rule-1000-ingress }
> nft add rule firewall rules-all ct zone vmap { 1 : jump rule-1000-egress }
> 
> nft add rule firewall dnat-all ct zone vmap { 1 : jump dnat-1000 }
> nft add rule firewall dnat-1000 ip daddr 2.2.2.11 counter dnat to 10.0.0.7
> 
> For a package with ip daddr 2.2.2.11 and tcp dport 22, first time accept in the
> rule-1000-ingress and dnat to 10.0.0.7. Then second time the packet goto the wrong
> chain rule-1000-egress which leads the packet drop
> 
> So This patch avoid already dnat packet go through the prerouting hook for the
> second time.
> 
> Signed-off-by: wenxu <wenxu@...oud.cn>
> ---
>  drivers/net/vrf.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
> index 95909e2..e61a045 100644
> --- a/drivers/net/vrf.c
> +++ b/drivers/net/vrf.c
> @@ -37,6 +37,7 @@
>  #include <net/l3mdev.h>
>  #include <net/fib_rules.h>
>  #include <net/netns/generic.h>
> +#include <net/netfilter/nf_conntrack.h>
>  
>  #define DRV_NAME	"vrf"
>  #define DRV_VERSION	"1.0"
> @@ -898,6 +899,12 @@ static struct sk_buff *vrf_rcv_nfhook(u8 pf, unsigned int hook,
>  				      struct net_device *dev)
>  {
>  	struct net *net = dev_net(dev);
> +	enum ip_conntrack_info ctinfo;
> +	struct nf_conn *ct;
> +
> +	ct = nf_ct_get(skb, &ctinfo);
> +	if (ct && (ct->status & IPS_DST_NAT))
> +		return skb;
>  
>  	if (nf_hook(pf, hook, net, NULL, skb, dev, NULL, vrf_rcv_finish) != 1)
>  		skb = NULL;    /* kfree_skb(skb) handled by nf code */
> 

That looks better to me. I am not familiar enough with netfilter
internals to know if this is acceptable; you should cc the netfilter
list to get opinions.

And for this to go into net and back to stable releases, you need a
Fixes tag:

Fixes: 73e20b761acf8 ("net: vrf: Add support for PREROUTING rules on vrf
device")