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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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
>