[PATCH net 0/5] netfilter: bugfixes for net

[PATCH net 0/5] netfilter: bugfixes for net

[Florian Westphal]

Hello,

The following set contains netfilter fixes for the *net* tree.

Regressions (rc only):
recent ebtables crash fix was incomplete, it added a memory leak.

The patch to fix possible buffer overrun for BIG TCP in ftp conntrack
tried to be too clever, we cannot re-use ct->lock: NAT engine might
grab it again -> deadlock.  Revert back to a global spinlock.
Both from myself.

Remove the documentation for the recently removed
'nf_conntrack_helper' sysctl as well, from Pablo Neira.

The static_branch_inc() that guards the 'chain stats enabled' path
needs to be deferred further, until the entire transaction was created.
From Tetsuo Handa.

Older bugs:
Since 5.3:
nf_tables_addchain may leak pcpu memory in error path when
offloading fails. Also from Tetsuo Handa.

Please consider pulling these changes from
  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git

----------------------------------------------------------------
The following changes since commit 603ccb3aca717d04a4b1a04e3a7bc3b91eba33e8:

  MAINTAINERS: Add myself as a reviewer for Qualcomm ETHQOS Ethernet driver (2022-09-20 13:42:55 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git master

for you to fetch changes up to d25088932227680988a6b794221e031a7232f137:

  netfilter: nf_ct_ftp: fix deadlock when nat rewrite is needed (2022-09-20 23:50:03 +0200)

----------------------------------------------------------------
Florian Westphal (2):
      netfilter: ebtables: fix memory leak when blob is malformed
      netfilter: nf_ct_ftp: fix deadlock when nat rewrite is needed

Pablo Neira Ayuso (1):
      netfilter: conntrack: remove nf_conntrack_helper documentation

Tetsuo Handa (2):
      netfilter: nf_tables: fix nft_counters_enabled underflow at nf_tables_addchain()
      netfilter: nf_tables: fix percpu memory leak at nf_tables_addchain()

 Documentation/networking/nf_conntrack-sysctl.rst | 9 ---------
 net/bridge/netfilter/ebtables.c                  | 4 +++-
 net/netfilter/nf_conntrack_ftp.c                 | 6 ++++--
 net/netfilter/nf_tables_api.c                    | 8 ++++----
 4 files changed, 11 insertions(+), 16 deletions(-)

[PATCH net 1/5] netfilter: conntrack: remove nf_conntrack_helper documentation

[Florian Westphal]

From: Pablo Neira Ayuso <pablo@netfilter.org>

This toggle has been already remove by b118509076b3 ("netfilter: remove
nf_conntrack_helper sysctl and modparam toggles").

Remove the documentation entry for this toggle too.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Documentation/networking/nf_conntrack-sysctl.rst | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/Documentation/networking/nf_conntrack-sysctl.rst b/Documentation/networking/nf_conntrack-sysctl.rst
index 834945ebc4cd..1120d71f28d7 100644
--- a/Documentation/networking/nf_conntrack-sysctl.rst
+++ b/Documentation/networking/nf_conntrack-sysctl.rst
@@ -70,15 +70,6 @@ nf_conntrack_generic_timeout - INTEGER (seconds)
 	Default for generic timeout.  This refers to layer 4 unknown/unsupported
 	protocols.
 
-nf_conntrack_helper - BOOLEAN
-	- 0 - disabled (default)
-	- not 0 - enabled
-
-	Enable automatic conntrack helper assignment.
-	If disabled it is required to set up iptables rules to assign
-	helpers to connections.  See the CT target description in the
-	iptables-extensions(8) man page for further information.
-
 nf_conntrack_icmp_timeout - INTEGER (seconds)
 	default 30
 
-- 
2.35.1

Re: [PATCH net 1/5] netfilter: conntrack: remove nf_conntrack_helper documentation

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by Florian Westphal <fw@strlen.de>:

On Wed, 21 Sep 2022 09:38:21 +0200 you wrote:
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> 
> This toggle has been already remove by b118509076b3 ("netfilter: remove
> nf_conntrack_helper sysctl and modparam toggles").
> 
> Remove the documentation entry for this toggle too.
> 
> [...]

Here is the summary with links:
  - [net,1/5] netfilter: conntrack: remove nf_conntrack_helper documentation
    https://git.kernel.org/netdev/net/c/76b907ee00c4
  - [net,2/5] netfilter: nf_tables: fix nft_counters_enabled underflow at nf_tables_addchain()
    https://git.kernel.org/netdev/net/c/921ebde3c0d2
  - [net,3/5] netfilter: nf_tables: fix percpu memory leak at nf_tables_addchain()
    https://git.kernel.org/netdev/net/c/9a4d6dd554b8
  - [net,4/5] netfilter: ebtables: fix memory leak when blob is malformed
    https://git.kernel.org/netdev/net/c/62ce44c4fff9
  - [net,5/5] netfilter: nf_ct_ftp: fix deadlock when nat rewrite is needed
    https://git.kernel.org/netdev/net/c/d25088932227

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

[PATCH net 2/5] netfilter: nf_tables: fix nft_counters_enabled underflow at nf_tables_addchain()

[Florian Westphal]

From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

syzbot is reporting underflow of nft_counters_enabled counter at
nf_tables_addchain() [1], for commit 43eb8949cfdffa76 ("netfilter:
nf_tables: do not leave chain stats enabled on error") missed that
nf_tables_chain_destroy() after nft_basechain_init() in the error path of
nf_tables_addchain() decrements the counter because nft_basechain_init()
makes nft_is_base_chain() return true by setting NFT_CHAIN_BASE flag.

Increment the counter immediately after returning from
nft_basechain_init().

Link:  https://syzkaller.appspot.com/bug?extid=b5d82a651b71cd8a75ab [1]
Reported-by: syzbot <syzbot+b5d82a651b71cd8a75ab@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: syzbot <syzbot+b5d82a651b71cd8a75ab@syzkaller.appspotmail.com>
Fixes: 43eb8949cfdffa76 ("netfilter: nf_tables: do not leave chain stats enabled on error")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_tables_api.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 816052089b33..e062754dc6cc 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2197,7 +2197,6 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 genmask,
 			      struct netlink_ext_ack *extack)
 {
 	const struct nlattr * const *nla = ctx->nla;
-	struct nft_stats __percpu *stats = NULL;
 	struct nft_table *table = ctx->table;
 	struct nft_base_chain *basechain;
 	struct net *net = ctx->net;
@@ -2212,6 +2211,7 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 genmask,
 		return -EOVERFLOW;
 
 	if (nla[NFTA_CHAIN_HOOK]) {
+		struct nft_stats __percpu *stats = NULL;
 		struct nft_chain_hook hook;
 
 		if (flags & NFT_CHAIN_BINDING)
@@ -2245,6 +2245,8 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 genmask,
 			kfree(basechain);
 			return err;
 		}
+		if (stats)
+			static_branch_inc(&nft_counters_enabled);
 	} else {
 		if (flags & NFT_CHAIN_BASE)
 			return -EINVAL;
@@ -2319,9 +2321,6 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 genmask,
 		goto err_unregister_hook;
 	}
 
-	if (stats)
-		static_branch_inc(&nft_counters_enabled);
-
 	table->use++;
 
 	return 0;
-- 
2.35.1

[PATCH net 3/5] netfilter: nf_tables: fix percpu memory leak at nf_tables_addchain()

[Florian Westphal]

From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

It seems to me that percpu memory for chain stats started leaking since
commit 3bc158f8d0330f0a ("netfilter: nf_tables: map basechain priority to
hardware priority") when nft_chain_offload_priority() returned an error.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: 3bc158f8d0330f0a ("netfilter: nf_tables: map basechain priority to hardware priority")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_tables_api.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index e062754dc6cc..63c70141b3e5 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2243,6 +2243,7 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 genmask,
 		if (err < 0) {
 			nft_chain_release_hook(&hook);
 			kfree(basechain);
+			free_percpu(stats);
 			return err;
 		}
 		if (stats)
-- 
2.35.1

[PATCH net 4/5] netfilter: ebtables: fix memory leak when blob is malformed

[Florian Westphal]

The bug fix was incomplete, it "replaced" crash with a memory leak.
The old code had an assignment to "ret" embedded into the conditional,
restore this.

Fixes: 7997eff82828 ("netfilter: ebtables: reject blobs that don't provide all entry points")
Reported-and-tested-by: syzbot+a24c5252f3e3ab733464@syzkaller.appspotmail.com
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/bridge/netfilter/ebtables.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 9a0ae59cdc50..4f385d52a1c4 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1040,8 +1040,10 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
 		goto free_iterate;
 	}
 
-	if (repl->valid_hooks != t->valid_hooks)
+	if (repl->valid_hooks != t->valid_hooks) {
+		ret = -EINVAL;
 		goto free_unlock;
+	}
 
 	if (repl->num_counters && repl->num_counters != t->private->nentries) {
 		ret = -EINVAL;
-- 
2.35.1

[PATCH net 5/5] netfilter: nf_ct_ftp: fix deadlock when nat rewrite is needed

[Florian Westphal]

We can't use ct->lock, this is already used by the seqadj internals.
When using ftp helper + nat, seqadj will attempt to acquire ct->lock
again.

Revert back to a global lock for now.

Fixes: c783a29c7e59 ("netfilter: nf_ct_ftp: prefer skb_linearize")
Reported-by: Bruno de Paula Larini <bruno.larini@riosoft.com.br>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_conntrack_ftp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index 0d9332e9cf71..617f744a2e3a 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -33,6 +33,7 @@ MODULE_AUTHOR("Rusty Russell <rusty@rustcorp.com.au>");
 MODULE_DESCRIPTION("ftp connection tracking helper");
 MODULE_ALIAS("ip_conntrack_ftp");
 MODULE_ALIAS_NFCT_HELPER(HELPER_NAME);
+static DEFINE_SPINLOCK(nf_ftp_lock);
 
 #define MAX_PORTS 8
 static u_int16_t ports[MAX_PORTS];
@@ -409,7 +410,8 @@ static int help(struct sk_buff *skb,
 	}
 	datalen = skb->len - dataoff;
 
-	spin_lock_bh(&ct->lock);
+	/* seqadj (nat) uses ct->lock internally, nf_nat_ftp would cause deadlock */
+	spin_lock_bh(&nf_ftp_lock);
 	fb_ptr = skb->data + dataoff;
 
 	ends_in_nl = (fb_ptr[datalen - 1] == '\n');
@@ -538,7 +540,7 @@ static int help(struct sk_buff *skb,
 	if (ends_in_nl)
 		update_nl_seq(ct, seq, ct_ftp_info, dir, skb);
  out:
-	spin_unlock_bh(&ct->lock);
+	spin_unlock_bh(&nf_ftp_lock);
 	return ret;
 }
 
-- 
2.35.1