netdev - Re: [PATCH net-next 2/2] vrf: run conntrack only in context of lower/physdev for locally generated packets

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: <dbbc274e-cf69-5207-6ddd-00c435d5a689@average.org>
Date:   Fri, 22 Oct 2021 01:03:05 +0200
From:   Eugene Crosser <crosser@...rage.org>
To:     Florian Westphal <fw@...len.de>, netdev@...r.kernel.org
Cc:     netfilter-devel@...r.kernel.org, dsahern@...nel.org,
        pablo@...filter.org, lschlesinger@...venets.com
Subject: Re: [PATCH net-next 2/2] vrf: run conntrack only in context of
 lower/physdev for locally generated packets

On 21/10/2021 16:48, Florian Westphal wrote:
> The VRF driver invokes netfilter for output+postrouting hooks so that users
> can create rules that check for 'oif $vrf' rather than lower device name.
> 
> This is a problem when NAT rules are configured.
> 
> To avoid any conntrack involvement in round 1, tag skbs as 'untracked'
> to prevent conntrack from picking them up.
> 
> This gets cleared before the packet gets handed to the ip stack so
> conntrack will be active on the second iteration.
> 
> For ingress, conntrack has already been done before the packet makes it
> to the vrf driver, with this patch egress does connection tracking with
> lower/physical device as well.
> 
> Signed-off-by: Florian Westphal <fw@...len.de>
> ---
>  drivers/net/vrf.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
> index bf2fac913942..c813d03159bf 100644
> --- a/drivers/net/vrf.c
> +++ b/drivers/net/vrf.c
> @@ -35,6 +35,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.1"
> @@ -424,12 +425,26 @@ static int vrf_local_xmit(struct sk_buff *skb, struct net_device *dev,
>  	return NETDEV_TX_OK;
>  }
>  
> +static void vrf_nf_set_untracked(struct sk_buff *skb)
> +{
> +	if (skb_get_nfct(skb) == 0)
> +		nf_ct_set(skb, 0, IP_CT_UNTRACKED);
> +}
> +
> +static void vrf_nf_reset_ct(struct sk_buff *skb)
> +{
> +	if (skb_get_nfct(skb) == IP_CT_UNTRACKED)
> +		nf_reset_ct(skb);
> +}
> +

Isn't it possible that skb was marked UNTRACKED before entering this path, by a
rule? In such case 'set_untrackd' will do nothing, but 'reset_ct' will clear
UNTRACKED status that was set elswhere. It seems wrong, am I missing something?

>  #if IS_ENABLED(CONFIG_IPV6)
>  static int vrf_ip6_local_out(struct net *net, struct sock *sk,
>  			     struct sk_buff *skb)
>  {
>  	int err;
>  
> +	vrf_nf_reset_ct(skb);
> +
>  	err = nf_hook(NFPROTO_IPV6, NF_INET_LOCAL_OUT, net,
>  		      sk, skb, NULL, skb_dst(skb)->dev, dst_output);
>  
> @@ -508,6 +523,8 @@ static int vrf_ip_local_out(struct net *net, struct sock *sk,
>  {
>  	int err;
>  
> +	vrf_nf_reset_ct(skb);
> +
>  	err = nf_hook(NFPROTO_IPV4, NF_INET_LOCAL_OUT, net, sk,
>  		      skb, NULL, skb_dst(skb)->dev, dst_output);
>  	if (likely(err == 1))
> @@ -626,8 +643,7 @@ static void vrf_finish_direct(struct sk_buff *skb)
>  		skb_pull(skb, ETH_HLEN);
>  	}
>  
> -	/* reset skb device */
> -	nf_reset_ct(skb);
> +	vrf_nf_reset_ct(skb);
>  }
>  
>  #if IS_ENABLED(CONFIG_IPV6)
> @@ -641,7 +657,7 @@ static int vrf_finish_output6(struct net *net, struct sock *sk,
>  	struct neighbour *neigh;
>  	int ret;
>  
> -	nf_reset_ct(skb);
> +	vrf_nf_reset_ct(skb);
>  
>  	skb->protocol = htons(ETH_P_IPV6);
>  	skb->dev = dev;
> @@ -752,6 +768,8 @@ static struct sk_buff *vrf_ip6_out_direct(struct net_device *vrf_dev,
>  
>  	skb->dev = vrf_dev;
>  
> +	vrf_nf_set_untracked(skb);
> +
>  	err = nf_hook(NFPROTO_IPV6, NF_INET_LOCAL_OUT, net, sk,
>  		      skb, NULL, vrf_dev, vrf_ip6_out_direct_finish);
>  
> @@ -858,7 +876,7 @@ static int vrf_finish_output(struct net *net, struct sock *sk, struct sk_buff *s
>  	struct neighbour *neigh;
>  	bool is_v6gw = false;
>  
> -	nf_reset_ct(skb);
> +	vrf_nf_reset_ct(skb);
>  
>  	/* Be paranoid, rather than too clever. */
>  	if (unlikely(skb_headroom(skb) < hh_len && dev->header_ops)) {
> @@ -980,6 +998,8 @@ static struct sk_buff *vrf_ip_out_direct(struct net_device *vrf_dev,
>  
>  	skb->dev = vrf_dev;
>  
> +	vrf_nf_set_untracked(skb);
> +
>  	err = nf_hook(NFPROTO_IPV4, NF_INET_LOCAL_OUT, net, sk,
>  		      skb, NULL, vrf_dev, vrf_ip_out_direct_finish);
>  
> 



Download attachment "OpenPGP_signature" of type "application/pgp-signature" (489 bytes)