[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.