[nf PATCH v2] netfilter: nf_tables: Fix entries val in rule reset audit log

[nf PATCH v2] netfilter: nf_tables: Fix entries val in rule reset audit log

[Phil Sutter]

The value in idx and the number of rules handled in that particular
__nf_tables_dump_rules() call is not identical. The former is a cursor
to pick up from if multiple netlink messages are needed, so its value is
ever increasing. Fixing this is not just a matter of subtracting s_idx
from it, though: When resetting rules in multiple chains,
__nf_tables_dump_rules() is called for each and cb->args[0] is not
adjusted in between.

The audit notification in __nf_tables_dump_rules() had another problem:
If nf_tables_fill_rule_info() failed (e.g. due to buffer exhaustion), no
notification was sent - despite the rules having been reset already.

To catch all the above and return to a single (if possible) notification
per table again, move audit logging back into the caller but into the
table loop instead of past it to avoid the potential null-pointer
deref.

This requires to trigger the notification in two spots. Care has to be
taken in the second case as cb->args[0] is also not updated in between
tables. This requires a helper variable as either it is the first table
(with potential non-zero cb->args[0] cursor) or a consecutive one (with
idx holding the current cursor already).

Fixes: 9b5ba5c9c5109 ("netfilter: nf_tables: Unbreak audit log reset")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Use max_t() to eliminate the kernel warning
---
 net/netfilter/nf_tables_api.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index e429ebba74b3d..5a1ff10d1d2a5 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3481,9 +3481,6 @@ static int __nf_tables_dump_rules(struct sk_buff *skb,
 		(*idx)++;
 	}
 
-	if (reset && *idx)
-		audit_log_rule_reset(table, cb->seq, *idx);
-
 	return 0;
 }
 
@@ -3494,11 +3491,12 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
 	const struct nft_rule_dump_ctx *ctx = cb->data;
 	struct nft_table *table;
 	const struct nft_chain *chain;
-	unsigned int idx = 0;
+	unsigned int idx = 0, s_idx;
 	struct net *net = sock_net(skb->sk);
 	int family = nfmsg->nfgen_family;
 	struct nftables_pernet *nft_net;
 	bool reset = false;
+	int ret;
 
 	if (NFNL_MSG_TYPE(cb->nlh->nlmsg_type) == NFT_MSG_GETRULE_RESET)
 		reset = true;
@@ -3529,16 +3527,23 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
 						       cb, table, chain, reset);
 				break;
 			}
+			if (reset && idx > cb->args[0])
+				audit_log_rule_reset(table, cb->seq,
+						     idx - cb->args[0]);
 			goto done;
 		}
 
+		s_idx = max_t(long, idx, cb->args[0]);
 		list_for_each_entry_rcu(chain, &table->chains, list) {
-			if (__nf_tables_dump_rules(skb, &idx,
-						   cb, table, chain, reset))
-				goto done;
+			ret = __nf_tables_dump_rules(skb, &idx,
+						     cb, table, chain, reset);
+			if (ret)
+				break;
 		}
+		if (reset && idx > s_idx)
+			audit_log_rule_reset(table, cb->seq, idx - s_idx);
 
-		if (ctx && ctx->table)
+		if ((ctx && ctx->table) || ret)
 			break;
 	}
 done:
-- 
2.41.0

Re: [nf PATCH v2] netfilter: nf_tables: Fix entries val in rule reset audit log

[Pablo Neira Ayuso]

Hi Phil,

On Fri, Sep 08, 2023 at 10:10:33AM +0200, Phil Sutter wrote:
> The value in idx and the number of rules handled in that particular
> __nf_tables_dump_rules() call is not identical. The former is a cursor
> to pick up from if multiple netlink messages are needed, so its value is
> ever increasing.

My (buggy) intention was to display this audit log once per chain, at
the end of the chain dump.

> Fixing this is not just a matter of subtracting s_idx
> from it, though: When resetting rules in multiple chains,
> __nf_tables_dump_rules() is called for each and cb->args[0] is not
> adjusted in between.
> 
> The audit notification in __nf_tables_dump_rules() had another problem:
> If nf_tables_fill_rule_info() failed (e.g. due to buffer exhaustion), no
> notification was sent - despite the rules having been reset already.

Hm. that should not happen, when nf_tables_fill_rule_info() fails,
that means buffer is full and userspace will invoke recvmsg() again.
The next buffer resumes from the last entry that could not fit into
the buffer.

> To catch all the above and return to a single (if possible) notification
> per table again, move audit logging back into the caller but into the
> table loop instead of past it to avoid the potential null-pointer
> deref.
> 
> This requires to trigger the notification in two spots. Care has to be
> taken in the second case as cb->args[0] is also not updated in between
> tables. This requires a helper variable as either it is the first table
> (with potential non-zero cb->args[0] cursor) or a consecutive one (with
> idx holding the current cursor already).

Your intention is to trigger one single audit log per table, right?
Did you test a chain with a large ruleset that needs several buffers
to be delivered to userspace in the netlink dump?

I would be inclined to do this once per-chain, so this can be extended
later on to display the chain. Yes, that means this will send one
audit log per chain, but this is where follow up updates will go?

> Fixes: 9b5ba5c9c5109 ("netfilter: nf_tables: Unbreak audit log reset")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> Changes since v1:
> - Use max_t() to eliminate the kernel warning
> ---
>  net/netfilter/nf_tables_api.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index e429ebba74b3d..5a1ff10d1d2a5 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -3481,9 +3481,6 @@ static int __nf_tables_dump_rules(struct sk_buff *skb,
>  		(*idx)++;
>  	}
>  
> -	if (reset && *idx)
> -		audit_log_rule_reset(table, cb->seq, *idx);
> -
>  	return 0;
>  }
>  
> @@ -3494,11 +3491,12 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
>  	const struct nft_rule_dump_ctx *ctx = cb->data;
>  	struct nft_table *table;
>  	const struct nft_chain *chain;
> -	unsigned int idx = 0;
> +	unsigned int idx = 0, s_idx;
>  	struct net *net = sock_net(skb->sk);
>  	int family = nfmsg->nfgen_family;
>  	struct nftables_pernet *nft_net;
>  	bool reset = false;
> +	int ret;
>  
>  	if (NFNL_MSG_TYPE(cb->nlh->nlmsg_type) == NFT_MSG_GETRULE_RESET)
>  		reset = true;
> @@ -3529,16 +3527,23 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
>  						       cb, table, chain, reset);
>  				break;
>  			}
> +			if (reset && idx > cb->args[0])
> +				audit_log_rule_reset(table, cb->seq,
> +						     idx - cb->args[0]);
>  			goto done;
>  		}
>  
> +		s_idx = max_t(long, idx, cb->args[0]);
>  		list_for_each_entry_rcu(chain, &table->chains, list) {
> -			if (__nf_tables_dump_rules(skb, &idx,
> -						   cb, table, chain, reset))
> -				goto done;
> +			ret = __nf_tables_dump_rules(skb, &idx,
> +						     cb, table, chain, reset);
> +			if (ret)
> +				break;
>  		}
> +		if (reset && idx > s_idx)
> +			audit_log_rule_reset(table, cb->seq, idx - s_idx);
>  
> -		if (ctx && ctx->table)
> +		if ((ctx && ctx->table) || ret)
>  			break;
>  	}
>  done:
> -- 
> 2.41.0
> 

Re: [nf PATCH v2] netfilter: nf_tables: Fix entries val in rule reset audit log

[Phil Sutter]

On Fri, Sep 08, 2023 at 04:01:02PM +0200, Pablo Neira Ayuso wrote:
> Hi Phil,
> 
> On Fri, Sep 08, 2023 at 10:10:33AM +0200, Phil Sutter wrote:
> > The value in idx and the number of rules handled in that particular
> > __nf_tables_dump_rules() call is not identical. The former is a cursor
> > to pick up from if multiple netlink messages are needed, so its value is
> > ever increasing.
> 
> My (buggy) intention was to display this audit log once per chain, at
> the end of the chain dump.

Ah, I wasn't aware you did that on purpose.

> > Fixing this is not just a matter of subtracting s_idx
> > from it, though: When resetting rules in multiple chains,
> > __nf_tables_dump_rules() is called for each and cb->args[0] is not
> > adjusted in between.
> > 
> > The audit notification in __nf_tables_dump_rules() had another problem:
> > If nf_tables_fill_rule_info() failed (e.g. due to buffer exhaustion), no
> > notification was sent - despite the rules having been reset already.
> 
> Hm. that should not happen, when nf_tables_fill_rule_info() fails,
> that means buffer is full and userspace will invoke recvmsg() again.
> The next buffer resumes from the last entry that could not fit into
> the buffer.

I didn't explicitly test for this case, but __nf_tables_dump_rules()
calls nf_tables_fill_rule_info() in a loop for reach rule. If it fails,
the function immediately returns 1 without calling
audit_log_rule_reset(). So while the last (failing) rule dump/reset will
be repeated after the detour to user space, the preceding rules
successfully dumped/reset slip through.

> > To catch all the above and return to a single (if possible) notification
> > per table again, move audit logging back into the caller but into the
> > table loop instead of past it to avoid the potential null-pointer
> > deref.
> > 
> > This requires to trigger the notification in two spots. Care has to be
> > taken in the second case as cb->args[0] is also not updated in between
> > tables. This requires a helper variable as either it is the first table
> > (with potential non-zero cb->args[0] cursor) or a consecutive one (with
> > idx holding the current cursor already).
> 
> Your intention is to trigger one single audit log per table, right?
> Did you test a chain with a large ruleset that needs several buffers
> to be delivered to userspace in the netlink dump?

Yes, see the last part in the proposed kselftest[1]: Resetting rules in
a chain with 503 rules causes three notifications to be sent, for 189,
188 and 126 rules each.

> I would be inclined to do this once per-chain, so this can be extended
> later on to display the chain. Yes, that means this will send one
> audit log per chain, but this is where follow up updates will go?

If you prefer that, no problem. I'll prepare a v3 then.

Cheers, Phil

[1] https://lore.kernel.org/netfilter-devel/20230908002229.1409-3-phil@nwl.cc/

Re: [nf PATCH v2] netfilter: nf_tables: Fix entries val in rule reset audit log

[Pablo Neira Ayuso]

On Fri, Sep 08, 2023 at 04:42:33PM +0200, Phil Sutter wrote:
> On Fri, Sep 08, 2023 at 04:01:02PM +0200, Pablo Neira Ayuso wrote:
> > Hi Phil,
> > 
> > On Fri, Sep 08, 2023 at 10:10:33AM +0200, Phil Sutter wrote:
> > > The value in idx and the number of rules handled in that particular
> > > __nf_tables_dump_rules() call is not identical. The former is a cursor
> > > to pick up from if multiple netlink messages are needed, so its value is
> > > ever increasing.
> > 
> > My (buggy) intention was to display this audit log once per chain, at
> > the end of the chain dump.
> 
> Ah, I wasn't aware you did that on purpose.
> 
> > > Fixing this is not just a matter of subtracting s_idx
> > > from it, though: When resetting rules in multiple chains,
> > > __nf_tables_dump_rules() is called for each and cb->args[0] is not
> > > adjusted in between.
> > > 
> > > The audit notification in __nf_tables_dump_rules() had another problem:
> > > If nf_tables_fill_rule_info() failed (e.g. due to buffer exhaustion), no
> > > notification was sent - despite the rules having been reset already.
> > 
> > Hm. that should not happen, when nf_tables_fill_rule_info() fails,
> > that means buffer is full and userspace will invoke recvmsg() again.
> > The next buffer resumes from the last entry that could not fit into
> > the buffer.
> 
> I didn't explicitly test for this case, but __nf_tables_dump_rules()
> calls nf_tables_fill_rule_info() in a loop for reach rule. If it fails,
> the function immediately returns 1 without calling
> audit_log_rule_reset(). So while the last (failing) rule dump/reset will
> be repeated after the detour to user space, the preceding rules
> successfully dumped/reset slip through.

I see, note that "failing" in this case means, "dump is still in
progress" and "userspace will invoke recvmsg() again to resume from
where we stopped.

> > > To catch all the above and return to a single (if possible) notification
> > > per table again, move audit logging back into the caller but into the
> > > table loop instead of past it to avoid the potential null-pointer
> > > deref.
> > > 
> > > This requires to trigger the notification in two spots. Care has to be
> > > taken in the second case as cb->args[0] is also not updated in between
> > > tables. This requires a helper variable as either it is the first table
> > > (with potential non-zero cb->args[0] cursor) or a consecutive one (with
> > > idx holding the current cursor already).
> > 
> > Your intention is to trigger one single audit log per table, right?
> > Did you test a chain with a large ruleset that needs several buffers
> > to be delivered to userspace in the netlink dump?
> 
> Yes, see the last part in the proposed kselftest[1]: Resetting rules in
> a chain with 503 rules causes three notifications to be sent, for 189,
> 188 and 126 rules each.
> 
> > I would be inclined to do this once per-chain, so this can be extended
> > later on to display the chain. Yes, that means this will send one
> > audit log per chain, but this is where follow up updates will go?
> 
> If you prefer that, no problem. I'll prepare a v3 then.

If patch is smaller and we agree that chains are useful to be in the
audit log (in follow up updates), then I'd suggest to go for a v3, yes.

Thanks.

Re: [nf PATCH v2] netfilter: nf_tables: Fix entries val in rule reset audit log

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20230908081033.30806-1-phil@nwl.cc>
References: <20230908081033.30806-1-phil@nwl.cc>
TO: Phil Sutter <phil@nwl.cc>

Hi Phil,

kernel test robot noticed the following build warnings:

[auto build test WARNING on netfilter-nf/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Phil-Sutter/netfilter-nf_tables-Fix-entries-val-in-rule-reset-audit-log/20230908-161249
base:   git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git main
patch link:    https://lore.kernel.org/r/20230908081033.30806-1-phil%40nwl.cc
patch subject: [nf PATCH v2] netfilter: nf_tables: Fix entries val in rule reset audit log
:::::: branch date: 2 weeks ago
:::::: commit date: 2 weeks ago
config: i386-randconfig-141-20230922 (https://download.01.org/0day-ci/archive/20230923/202309230628.jYkaSoee-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230923/202309230628.jYkaSoee-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202309230628.jYkaSoee-lkp@intel.com/

smatch warnings:
net/netfilter/nf_tables_api.c:3546 nf_tables_dump_rules() error: uninitialized symbol 'ret'.

vim +/ret +3546 net/netfilter/nf_tables_api.c

241faeceb849cb Phil Sutter       2018-12-12  3486  
96518518cc417b Patrick McHardy   2013-10-14  3487  static int nf_tables_dump_rules(struct sk_buff *skb,
96518518cc417b Patrick McHardy   2013-10-14  3488  				struct netlink_callback *cb)
96518518cc417b Patrick McHardy   2013-10-14  3489  {
96518518cc417b Patrick McHardy   2013-10-14  3490  	const struct nfgenmsg *nfmsg = nlmsg_data(cb->nlh);
6e1f760e13c75e Pablo Neira Ayuso 2016-07-19  3491  	const struct nft_rule_dump_ctx *ctx = cb->data;
241faeceb849cb Phil Sutter       2018-12-12  3492  	struct nft_table *table;
96518518cc417b Patrick McHardy   2013-10-14  3493  	const struct nft_chain *chain;
35118755b61ff5 Phil Sutter       2023-09-08  3494  	unsigned int idx = 0, s_idx;
99633ab29b2131 Pablo Neira Ayuso 2013-10-10  3495  	struct net *net = sock_net(skb->sk);
96518518cc417b Patrick McHardy   2013-10-14  3496  	int family = nfmsg->nfgen_family;
0854db2aaef3fc Florian Westphal  2021-04-01  3497  	struct nftables_pernet *nft_net;
8daa8fde3fc3f0 Phil Sutter       2022-10-14  3498  	bool reset = false;
35118755b61ff5 Phil Sutter       2023-09-08  3499  	int ret;
8daa8fde3fc3f0 Phil Sutter       2022-10-14  3500  
8daa8fde3fc3f0 Phil Sutter       2022-10-14  3501  	if (NFNL_MSG_TYPE(cb->nlh->nlmsg_type) == NFT_MSG_GETRULE_RESET)
8daa8fde3fc3f0 Phil Sutter       2022-10-14  3502  		reset = true;
96518518cc417b Patrick McHardy   2013-10-14  3503  
e688a7f8c6cb7a Pablo Neira Ayuso 2014-07-01  3504  	rcu_read_lock();
d59d2f82f984df Pablo Neira Ayuso 2021-04-23  3505  	nft_net = nft_pernet(net);
3400278328285a Pablo Neira Ayuso 2022-08-09  3506  	cb->seq = READ_ONCE(nft_net->base_seq);
38e029f14a9702 Pablo Neira Ayuso 2014-07-01  3507  
0854db2aaef3fc Florian Westphal  2021-04-01  3508  	list_for_each_entry_rcu(table, &nft_net->tables, list) {
98319cb9089844 Pablo Neira Ayuso 2018-01-09  3509  		if (family != NFPROTO_UNSPEC && family != table->family)
96518518cc417b Patrick McHardy   2013-10-14  3510  			continue;
96518518cc417b Patrick McHardy   2013-10-14  3511  
36596dadf54a92 Pablo Neira Ayuso 2018-01-09  3512  		if (ctx && ctx->table && strcmp(ctx->table, table->name) != 0)
6e1f760e13c75e Pablo Neira Ayuso 2016-07-19  3513  			continue;
6e1f760e13c75e Pablo Neira Ayuso 2016-07-19  3514  
715849ab31f8e5 Pablo Neira Ayuso 2019-01-08  3515  		if (ctx && ctx->table && ctx->chain) {
241faeceb849cb Phil Sutter       2018-12-12  3516  			struct rhlist_head *list, *tmp;
6e1f760e13c75e Pablo Neira Ayuso 2016-07-19  3517  
241faeceb849cb Phil Sutter       2018-12-12  3518  			list = rhltable_lookup(&table->chains_ht, ctx->chain,
241faeceb849cb Phil Sutter       2018-12-12  3519  					       nft_chain_ht_params);
241faeceb849cb Phil Sutter       2018-12-12  3520  			if (!list)
96518518cc417b Patrick McHardy   2013-10-14  3521  				goto done;
38e029f14a9702 Pablo Neira Ayuso 2014-07-01  3522  
241faeceb849cb Phil Sutter       2018-12-12  3523  			rhl_for_each_entry_rcu(chain, tmp, list, rhlhead) {
241faeceb849cb Phil Sutter       2018-12-12  3524  				if (!nft_is_active(net, chain))
241faeceb849cb Phil Sutter       2018-12-12  3525  					continue;
241faeceb849cb Phil Sutter       2018-12-12  3526  				__nf_tables_dump_rules(skb, &idx,
8daa8fde3fc3f0 Phil Sutter       2022-10-14  3527  						       cb, table, chain, reset);
241faeceb849cb Phil Sutter       2018-12-12  3528  				break;
96518518cc417b Patrick McHardy   2013-10-14  3529  			}
35118755b61ff5 Phil Sutter       2023-09-08  3530  			if (reset && idx > cb->args[0])
35118755b61ff5 Phil Sutter       2023-09-08  3531  				audit_log_rule_reset(table, cb->seq,
35118755b61ff5 Phil Sutter       2023-09-08  3532  						     idx - cb->args[0]);
241faeceb849cb Phil Sutter       2018-12-12  3533  			goto done;
96518518cc417b Patrick McHardy   2013-10-14  3534  		}
241faeceb849cb Phil Sutter       2018-12-12  3535  
35118755b61ff5 Phil Sutter       2023-09-08  3536  		s_idx = max_t(long, idx, cb->args[0]);
241faeceb849cb Phil Sutter       2018-12-12  3537  		list_for_each_entry_rcu(chain, &table->chains, list) {
35118755b61ff5 Phil Sutter       2023-09-08  3538  			ret = __nf_tables_dump_rules(skb, &idx,
35118755b61ff5 Phil Sutter       2023-09-08  3539  						     cb, table, chain, reset);
35118755b61ff5 Phil Sutter       2023-09-08  3540  			if (ret)
35118755b61ff5 Phil Sutter       2023-09-08  3541  				break;
241faeceb849cb Phil Sutter       2018-12-12  3542  		}
35118755b61ff5 Phil Sutter       2023-09-08  3543  		if (reset && idx > s_idx)
35118755b61ff5 Phil Sutter       2023-09-08  3544  			audit_log_rule_reset(table, cb->seq, idx - s_idx);
241faeceb849cb Phil Sutter       2018-12-12  3545  
35118755b61ff5 Phil Sutter       2023-09-08 @3546  		if ((ctx && ctx->table) || ret)
241faeceb849cb Phil Sutter       2018-12-12  3547  			break;
96518518cc417b Patrick McHardy   2013-10-14  3548  	}
96518518cc417b Patrick McHardy   2013-10-14  3549  done:
e688a7f8c6cb7a Pablo Neira Ayuso 2014-07-01  3550  	rcu_read_unlock();
310529e663ed97 Phil Sutter       2018-12-30  3551  
310529e663ed97 Phil Sutter       2018-12-30  3552  	cb->args[0] = idx;
96518518cc417b Patrick McHardy   2013-10-14  3553  	return skb->len;
96518518cc417b Patrick McHardy   2013-10-14  3554  }
96518518cc417b Patrick McHardy   2013-10-14  3555  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki