[PATCH nf,v2 1/2] netfilter: conntrack: make conntrack userspace helpers work again

[PATCH nf,v2 1/2] netfilter: conntrack: make conntrack userspace helpers work again

[Pablo Neira Ayuso]

Florian Westphal says:

"Problem is that after the helper hook was merged back into the confirm
one, the queueing itself occurs from the confirm hook, i.e. we queue
from the last netfilter callback in the hook-list.

Therefore, on return, the packet bypasses the confirm action and the
connection is never committed to the main conntrack table.

Therefore, on return, the packet bypasses the confirm action and the
connection is never committed to the main conntrack table.

To fix this there are several ways:
1. revert the 'Fixes' commit and have a extra helper hook again.
   Works, but has the drawback of adding another indirect call for
   everyone.

2. Special case this: split the hooks only when userspace helper
   gets added, so queueing occurs at a lower priority again,
   and normal nqueue reinject would eventually call the last hook.

3. Extend the existing nf_queue ct update hook to allow a forced
   confirmation (plus run the seqadj code).

This goes for 3)."

Fixes: 827318feb69cb ("netfilter: conntrack: remove helper hook again")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: call __nf_conntrack_update() before ct helper confirmation.

 net/netfilter/nf_conntrack_core.c | 78 ++++++++++++++++++++++++++++---
 1 file changed, 72 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 1d57b95d3481..08e0c19f6b39 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -2016,22 +2016,18 @@ static void nf_conntrack_attach(struct sk_buff *nskb, const struct sk_buff *skb)
 	nf_conntrack_get(skb_nfct(nskb));
 }
 
-static int nf_conntrack_update(struct net *net, struct sk_buff *skb)
+static int __nf_conntrack_update(struct net *net, struct sk_buff *skb,
+				 struct nf_conn *ct)
 {
 	struct nf_conntrack_tuple_hash *h;
 	struct nf_conntrack_tuple tuple;
 	enum ip_conntrack_info ctinfo;
 	struct nf_nat_hook *nat_hook;
 	unsigned int status;
-	struct nf_conn *ct;
 	int dataoff;
 	u16 l3num;
 	u8 l4num;
 
-	ct = nf_ct_get(skb, &ctinfo);
-	if (!ct || nf_ct_is_confirmed(ct))
-		return 0;
-
 	l3num = nf_ct_l3num(ct);
 
 	dataoff = get_l4proto(skb, skb_network_offset(skb), l3num, &l4num);
@@ -2088,6 +2084,76 @@ static int nf_conntrack_update(struct net *net, struct sk_buff *skb)
 	return 0;
 }
 
+/* This packet is coming from userspace via nf_queue, complete the packet
+ * processing after the helper invocation in nf_confirm().
+ */
+static int nf_confirm_cthelper(struct sk_buff *skb, struct nf_conn *ct,
+			       enum ip_conntrack_info ctinfo)
+{
+	const struct nf_conntrack_helper *helper;
+	const struct nf_conn_help *help;
+	unsigned int protoff;
+
+	help = nfct_help(ct);
+	if (!help)
+		return 0;
+
+	helper = rcu_dereference(help->helper);
+	if (!(helper->flags & NF_CT_HELPER_F_USERSPACE))
+		return 0;
+
+	switch (nf_ct_l3num(ct)) {
+	case NFPROTO_IPV4:
+		protoff = skb_network_offset(skb) + ip_hdrlen(skb);
+		break;
+#if IS_ENABLED(CONFIG_IPV6)
+	case NFPROTO_IPV6: {
+		__be16 frag_off;
+		u8 pnum;
+
+		pnum = ipv6_hdr(skb)->nexthdr;
+		protoff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &pnum,
+					   &frag_off);
+		if (protoff < 0 || (frag_off & htons(~0x7)) != 0)
+			return 0;
+		break;
+	}
+#endif
+	default:
+		return 0;
+	}
+
+	if (test_bit(IPS_SEQ_ADJUST_BIT, &ct->status) &&
+	    !nf_is_loopback_packet(skb)) {
+		if (!nf_ct_seq_adjust(skb, ct, ctinfo, protoff)) {
+			NF_CT_STAT_INC_ATOMIC(nf_ct_net(ct), drop);
+			return -1;
+		}
+	}
+
+	/* We've seen it coming out the other side: confirm it */
+	return nf_conntrack_confirm(skb) == NF_DROP ? - 1 : 0;
+}
+
+static int nf_conntrack_update(struct net *net, struct sk_buff *skb)
+{
+	enum ip_conntrack_info ctinfo;
+	struct nf_conn *ct;
+	int err;
+
+	ct = nf_ct_get(skb, &ctinfo);
+	if (!ct)
+		return 0;
+
+	if (!nf_ct_is_confirmed(ct)) {
+		err = __nf_conntrack_update(net, skb, ct);
+		if (err < 0)
+			return err;
+	}
+
+	return nf_confirm_cthelper(skb, ct, ctinfo);
+}
+
 static bool nf_conntrack_get_tuple_skb(struct nf_conntrack_tuple *dst_tuple,
 				       const struct sk_buff *skb)
 {
-- 
2.20.1

[PATCH nf,v2 2/2] netfilter: nfnetlink_cthelper: unbreak userspace helper support

[Pablo Neira Ayuso]

Restore helper data size initialization and fix memcopy of the helper
data size.

Fixes: 157ffffeb5dc ("netfilter: nfnetlink_cthelper: reject too large userspace allocation requests")
Reviewed-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: no changes.

 net/netfilter/nfnetlink_cthelper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
index a5f294aa8e4c..5b0d0a77379c 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -103,7 +103,7 @@ nfnl_cthelper_from_nlattr(struct nlattr *attr, struct nf_conn *ct)
 	if (help->helper->data_len == 0)
 		return -EINVAL;
 
-	nla_memcpy(help->data, nla_data(attr), sizeof(help->data));
+	nla_memcpy(help->data, attr, sizeof(help->data));
 	return 0;
 }
 
@@ -240,6 +240,7 @@ nfnl_cthelper_create(const struct nlattr * const tb[],
 		ret = -ENOMEM;
 		goto err2;
 	}
+	helper->data_len = size;
 
 	helper->flags |= NF_CT_HELPER_F_USERSPACE;
 	memcpy(&helper->tuple, tuple, sizeof(struct nf_conntrack_tuple));
-- 
2.20.1

Re: [PATCH nf,v2 1/2] netfilter: conntrack: make conntrack userspace helpers work again

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Florian Westphal says:
> 
> "Problem is that after the helper hook was merged back into the confirm
> one, the queueing itself occurs from the confirm hook, i.e. we queue
> from the last netfilter callback in the hook-list.
> 
> Therefore, on return, the packet bypasses the confirm action and the
> connection is never committed to the main conntrack table.
> 
> Therefore, on return, the packet bypasses the confirm action and the
> connection is never committed to the main conntrack table.
> 
> To fix this there are several ways:
> 1. revert the 'Fixes' commit and have a extra helper hook again.
>    Works, but has the drawback of adding another indirect call for
>    everyone.
> 
> 2. Special case this: split the hooks only when userspace helper
>    gets added, so queueing occurs at a lower priority again,
>    and normal nqueue reinject would eventually call the last hook.
> 
> 3. Extend the existing nf_queue ct update hook to allow a forced
>    confirmation (plus run the seqadj code).
> 
> This goes for 3)."
> 
> Fixes: 827318feb69cb ("netfilter: conntrack: remove helper hook again")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> v2: call __nf_conntrack_update() before ct helper confirmation.

Reviewed-by: Florian Westphal <fw@strlen.de>

Re: [PATCH nf, v2 1/2] netfilter: conntrack: make conntrack userspace helpers work again

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 7482 bytes --]

Hi Pablo,

I love your patch! Perhaps something to improve:

[auto build test WARNING on nf/master]
[also build test WARNING on nf-next/master v5.7-rc7]
[cannot apply to next-20200529]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Pablo-Neira-Ayuso/netfilter-conntrack-make-conntrack-userspace-helpers-work-again/20200525-194929
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git master
config: x86_64-randconfig-a013-20200529 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 2d068e534f1671459e1b135852c1b3c10502e929)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> net/netfilter/nf_conntrack_core.c:2068:21: warning: variable 'ctinfo' is uninitialized when used here [-Wuninitialized]
nf_ct_set(skb, ct, ctinfo);
^~~~~~
net/netfilter/nf_conntrack_core.c:2024:2: note: variable 'ctinfo' is declared here
enum ip_conntrack_info ctinfo;
^
1 warning generated.

vim +/ctinfo +2068 net/netfilter/nf_conntrack_core.c

9fb9cbb1082d6b Yasuyuki Kozakai  2005-11-09  2018  
5d417ce363ff2d Pablo Neira Ayuso 2020-05-25  2019  static int __nf_conntrack_update(struct net *net, struct sk_buff *skb,
5d417ce363ff2d Pablo Neira Ayuso 2020-05-25  2020  				 struct nf_conn *ct)
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2021  {
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2022  	struct nf_conntrack_tuple_hash *h;
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2023  	struct nf_conntrack_tuple tuple;
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2024  	enum ip_conntrack_info ctinfo;
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2025  	struct nf_nat_hook *nat_hook;
6816d931cab009 Florian Westphal  2018-06-29  2026  	unsigned int status;
6816d931cab009 Florian Westphal  2018-06-29  2027  	int dataoff;
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2028  	u16 l3num;
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2029  	u8 l4num;
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2030  
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2031  	l3num = nf_ct_l3num(ct);
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2032  
6816d931cab009 Florian Westphal  2018-06-29  2033  	dataoff = get_l4proto(skb, skb_network_offset(skb), l3num, &l4num);
6816d931cab009 Florian Westphal  2018-06-29  2034  	if (dataoff <= 0)
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2035  		return -1;
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2036  
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2037  	if (!nf_ct_get_tuple(skb, skb_network_offset(skb), dataoff, l3num,
303e0c5589592e Florian Westphal  2019-01-15  2038  			     l4num, net, &tuple))
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2039  		return -1;
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2040  
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2041  	if (ct->status & IPS_SRC_NAT) {
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2042  		memcpy(tuple.src.u3.all,
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2043  		       ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u3.all,
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2044  		       sizeof(tuple.src.u3.all));
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2045  		tuple.src.u.all =
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2046  			ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u.all;
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2047  	}
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2048  
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2049  	if (ct->status & IPS_DST_NAT) {
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2050  		memcpy(tuple.dst.u3.all,
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2051  		       ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.u3.all,
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2052  		       sizeof(tuple.dst.u3.all));
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2053  		tuple.dst.u.all =
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2054  			ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.u.all;
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2055  	}
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2056  
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2057  	h = nf_conntrack_find_get(net, nf_ct_zone(ct), &tuple);
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2058  	if (!h)
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2059  		return 0;
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2060  
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2061  	/* Store status bits of the conntrack that is clashing to re-do NAT
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2062  	 * mangling according to what it has been done already to this packet.
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2063  	 */
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2064  	status = ct->status;
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2065  
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2066  	nf_ct_put(ct);
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2067  	ct = nf_ct_tuplehash_to_ctrack(h);
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23 @2068  	nf_ct_set(skb, ct, ctinfo);
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2069  
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2070  	nat_hook = rcu_dereference(nf_nat_hook);
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2071  	if (!nat_hook)
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2072  		return 0;
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2073  
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2074  	if (status & IPS_SRC_NAT &&
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2075  	    nat_hook->manip_pkt(skb, ct, NF_NAT_MANIP_SRC,
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2076  				IP_CT_DIR_ORIGINAL) == NF_DROP)
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2077  		return -1;
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2078  
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2079  	if (status & IPS_DST_NAT &&
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2080  	    nat_hook->manip_pkt(skb, ct, NF_NAT_MANIP_DST,
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2081  				IP_CT_DIR_ORIGINAL) == NF_DROP)
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2082  		return -1;
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2083  
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2084  	return 0;
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2085  }
368982cd7d1bd4 Pablo Neira Ayuso 2018-05-23  2086  

:::::: The code at line 2068 was first introduced by commit
:::::: 368982cd7d1bd41cd39049c794990aca3770db44 netfilter: nfnetlink_queue: resolve clash for unconfirmed conntracks

:::::: TO: Pablo Neira Ayuso <pablo@netfilter.org>
:::::: CC: Pablo Neira Ayuso <pablo@netfilter.org>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 37687 bytes --]