[nf PATCH 0/2] nf_tables: follow-up on audit fix, propose kselftest

[nf PATCH 0/2] nf_tables: follow-up on audit fix, propose kselftest

[Phil Sutter]

Patch 1 fixes/improves Pablo's fix for the bug I built into nf_tables'
audit support code. I propose it for nf, but nf-next is OK from my PoV
if it is deemed too risky.

Patch 2 adds a selftest for the audit notifications in nf_tables. Since
audit logging can not be turned off, it may cause problems requiring a
reboot to resolve. So I'd like to hear your opinions first before
attempting an "official" proposal.

Phil Sutter (2):
  netfilter: nf_tables: Fix entries val in rule reset audit log
  selftests: netfilter: Test nf_tables audit logging

 net/netfilter/nf_tables_api.c                 | 21 ++++--
 .../testing/selftests/netfilter/nft_audit.sh  | 75 +++++++++++++++++++
 2 files changed, 88 insertions(+), 8 deletions(-)
 create mode 100755 tools/testing/selftests/netfilter/nft_audit.sh

-- 
2.41.0

[nf PATCH 1/2] 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>
---
 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..d429270676131 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(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 1/2] netfilter: nf_tables: Fix entries val in rule reset audit log

[kernel test robot]

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-082530
base:   git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git main
patch link:    https://lore.kernel.org/r/20230908002229.1409-2-phil%40nwl.cc
patch subject: [nf PATCH 1/2] netfilter: nf_tables: Fix entries val in rule reset audit log
config: mips-randconfig-r002-20230908 (https://download.01.org/0day-ci/archive/20230908/202309081138.IpMoJwFy-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230908/202309081138.IpMoJwFy-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>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309081138.IpMoJwFy-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> net/netfilter/nf_tables_api.c:3536:11: warning: comparison of distinct pointer types ('typeof (idx) *' (aka 'unsigned int *') and 'typeof (cb->args[0]) *' (aka 'long *')) [-Wcompare-distinct-pointer-types]
    3536 |                 s_idx = max(idx, cb->args[0]);
         |                         ^~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:74:19: note: expanded from macro 'max'
      74 | #define max(x, y)       __careful_cmp(x, y, >)
         |                         ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:36:24: note: expanded from macro '__careful_cmp'
      36 |         __builtin_choose_expr(__safe_cmp(x, y), \
         |                               ^~~~~~~~~~~~~~~~
   include/linux/minmax.h:26:4: note: expanded from macro '__safe_cmp'
      26 |                 (__typecheck(x, y) && __no_side_effects(x, y))
         |                  ^~~~~~~~~~~~~~~~~
   include/linux/minmax.h:20:28: note: expanded from macro '__typecheck'
      20 |         (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
         |                    ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
   1 warning generated.


vim +3536 net/netfilter/nf_tables_api.c

  3486	
  3487	static int nf_tables_dump_rules(struct sk_buff *skb,
  3488					struct netlink_callback *cb)
  3489	{
  3490		const struct nfgenmsg *nfmsg = nlmsg_data(cb->nlh);
  3491		const struct nft_rule_dump_ctx *ctx = cb->data;
  3492		struct nft_table *table;
  3493		const struct nft_chain *chain;
  3494		unsigned int idx = 0, s_idx;
  3495		struct net *net = sock_net(skb->sk);
  3496		int family = nfmsg->nfgen_family;
  3497		struct nftables_pernet *nft_net;
  3498		bool reset = false;
  3499		int ret;
  3500	
  3501		if (NFNL_MSG_TYPE(cb->nlh->nlmsg_type) == NFT_MSG_GETRULE_RESET)
  3502			reset = true;
  3503	
  3504		rcu_read_lock();
  3505		nft_net = nft_pernet(net);
  3506		cb->seq = READ_ONCE(nft_net->base_seq);
  3507	
  3508		list_for_each_entry_rcu(table, &nft_net->tables, list) {
  3509			if (family != NFPROTO_UNSPEC && family != table->family)
  3510				continue;
  3511	
  3512			if (ctx && ctx->table && strcmp(ctx->table, table->name) != 0)
  3513				continue;
  3514	
  3515			if (ctx && ctx->table && ctx->chain) {
  3516				struct rhlist_head *list, *tmp;
  3517	
  3518				list = rhltable_lookup(&table->chains_ht, ctx->chain,
  3519						       nft_chain_ht_params);
  3520				if (!list)
  3521					goto done;
  3522	
  3523				rhl_for_each_entry_rcu(chain, tmp, list, rhlhead) {
  3524					if (!nft_is_active(net, chain))
  3525						continue;
  3526					__nf_tables_dump_rules(skb, &idx,
  3527							       cb, table, chain, reset);
  3528					break;
  3529				}
  3530				if (reset && idx > cb->args[0])
  3531					audit_log_rule_reset(table, cb->seq,
  3532							     idx - cb->args[0]);
  3533				goto done;
  3534			}
  3535	
> 3536			s_idx = max(idx, cb->args[0]);
  3537			list_for_each_entry_rcu(chain, &table->chains, list) {
  3538				ret = __nf_tables_dump_rules(skb, &idx,
  3539							     cb, table, chain, reset);
  3540				if (ret)
  3541					break;
  3542			}
  3543			if (reset && idx > s_idx)
  3544				audit_log_rule_reset(table, cb->seq, idx - s_idx);
  3545	
  3546			if ((ctx && ctx->table) || ret)
  3547				break;
  3548		}
  3549	done:
  3550		rcu_read_unlock();
  3551	
  3552		cb->args[0] = idx;
  3553		return skb->len;
  3554	}
  3555	

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

[nf-next RFC 2/2] selftests: netfilter: Test nf_tables audit logging

[Phil Sutter]

Perform ruleset modifications and compare the NETFILTER_CFG type
notifications emitted by auditd match expectations.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Calling auditd means enabling audit logging in kernel for the remaining
uptime. So this test will slow down following ones or even cause
spurious failures due to unexpected kernel log entries, timeouts, etc.

Is there a way to test this in a less intrusive way? Maybe fence this
test so it does not run automatically (is it any good having it in
kernel then)?
---
 .../testing/selftests/netfilter/nft_audit.sh  | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100755 tools/testing/selftests/netfilter/nft_audit.sh

diff --git a/tools/testing/selftests/netfilter/nft_audit.sh b/tools/testing/selftests/netfilter/nft_audit.sh
new file mode 100755
index 0000000000000..55c750720137f
--- /dev/null
+++ b/tools/testing/selftests/netfilter/nft_audit.sh
@@ -0,0 +1,75 @@
+#!/bin/bash
+
+SKIP_RC=4
+RC=0
+
+nft --version >/dev/null 2>&1 || {
+	echo "SKIP: missing nft tool"
+	exit $SKIP_RC
+}
+
+auditd --help >/dev/null 2>&1
+[ $? -eq 2 ] || {
+	echo "SKIP: missing auditd tool"
+	exit $SKIP_RC
+}
+
+tmpdir=$(mktemp -d)
+audit_log="$tmpdir/audit.log"
+cat >"$tmpdir/auditd.conf" <<EOF
+write_logs = no
+space_left = 75
+EOF
+auditd -f -c "$tmpdir" >"$audit_log" &
+audit_pid=$!
+trap 'kill $audit_pid; rm -rf $tmpdir' EXIT
+sleep 1
+
+logread() {
+	grep 'type=NETFILTER_CFG' "$audit_log" | \
+		sed -e 's/\(type\|msg\|pid\)=[^ ]* //g' \
+		    -e 's/\(table=[^:]*\):[0-9]*/\1/'
+}
+
+do_test() { # (cmd, log)
+	echo -n "testing for cmd: $1 ... "
+	echo >"$audit_log"
+	$1 >/dev/null || exit 1
+	diff -q <(echo "$2") <(logread) >/dev/null && { echo "OK"; return; }
+	echo "FAIL"
+	diff -u <(echo "$2") <(logread)
+	((RC++))
+}
+
+nft flush ruleset
+
+for table in t1 t2; do
+	echo "add table $table"
+	for chain in c1 c2 c3; do
+		echo "add chain $table $chain"
+		echo "add rule $table $chain counter accept"
+		echo "add rule $table $chain counter accept"
+		echo "add rule $table $chain counter accept"
+	done
+done | nft -f - || exit 1
+
+do_test 'nft reset rules t1 c2' \
+	'table=t1 family=2 entries=3 op=nft_reset_rule subj=kernel comm="nft"'
+
+do_test 'nft reset rules table t1' \
+	'table=t1 family=2 entries=9 op=nft_reset_rule subj=kernel comm="nft"'
+
+do_test 'nft reset rules' \
+	'table=t1 family=2 entries=9 op=nft_reset_rule subj=kernel comm="nft"
+table=t2 family=2 entries=9 op=nft_reset_rule subj=kernel comm="nft"'
+
+for ((i = 0; i < 500; i++)); do
+	echo "add rule t2 c3 counter accept comment \"rule $i\""
+done | nft -f - || exit 1
+
+do_test 'nft reset rules t2 c3' \
+	'table=t2 family=2 entries=189 op=nft_reset_rule subj=kernel comm="nft"
+table=t2 family=2 entries=188 op=nft_reset_rule subj=kernel comm="nft"
+table=t2 family=2 entries=126 op=nft_reset_rule subj=kernel comm="nft"'
+
+exit $RC
-- 
2.41.0

Re: [nf-next RFC 2/2] selftests: netfilter: Test nf_tables audit logging

[Pablo Neira Ayuso]

On Fri, Sep 08, 2023 at 02:22:29AM +0200, Phil Sutter wrote:
> Perform ruleset modifications and compare the NETFILTER_CFG type
> notifications emitted by auditd match expectations.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> Calling auditd means enabling audit logging in kernel for the remaining
> uptime. So this test will slow down following ones or even cause
> spurious failures due to unexpected kernel log entries, timeouts, etc.
> 
> Is there a way to test this in a less intrusive way? Maybe fence this
> test so it does not run automatically (is it any good having it in
> kernel then)?

I think you could make a small libmnl program to listen to
NETLINK_AUDIT events and filter only the logs you need from there. We
already have a few programs like this in the selftest folder.

> ---
>  .../testing/selftests/netfilter/nft_audit.sh  | 75 +++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100755 tools/testing/selftests/netfilter/nft_audit.sh
> 
> diff --git a/tools/testing/selftests/netfilter/nft_audit.sh b/tools/testing/selftests/netfilter/nft_audit.sh
> new file mode 100755
> index 0000000000000..55c750720137f
> --- /dev/null
> +++ b/tools/testing/selftests/netfilter/nft_audit.sh
> @@ -0,0 +1,75 @@
> +#!/bin/bash
> +
> +SKIP_RC=4
> +RC=0
> +
> +nft --version >/dev/null 2>&1 || {
> +	echo "SKIP: missing nft tool"
> +	exit $SKIP_RC
> +}
> +
> +auditd --help >/dev/null 2>&1
> +[ $? -eq 2 ] || {
> +	echo "SKIP: missing auditd tool"
> +	exit $SKIP_RC
> +}
> +
> +tmpdir=$(mktemp -d)
> +audit_log="$tmpdir/audit.log"
> +cat >"$tmpdir/auditd.conf" <<EOF
> +write_logs = no
> +space_left = 75
> +EOF
> +auditd -f -c "$tmpdir" >"$audit_log" &
> +audit_pid=$!
> +trap 'kill $audit_pid; rm -rf $tmpdir' EXIT
> +sleep 1
> +
> +logread() {
> +	grep 'type=NETFILTER_CFG' "$audit_log" | \
> +		sed -e 's/\(type\|msg\|pid\)=[^ ]* //g' \
> +		    -e 's/\(table=[^:]*\):[0-9]*/\1/'
> +}
> +
> +do_test() { # (cmd, log)
> +	echo -n "testing for cmd: $1 ... "
> +	echo >"$audit_log"
> +	$1 >/dev/null || exit 1
> +	diff -q <(echo "$2") <(logread) >/dev/null && { echo "OK"; return; }
> +	echo "FAIL"
> +	diff -u <(echo "$2") <(logread)
> +	((RC++))
> +}
> +
> +nft flush ruleset
> +
> +for table in t1 t2; do
> +	echo "add table $table"
> +	for chain in c1 c2 c3; do
> +		echo "add chain $table $chain"
> +		echo "add rule $table $chain counter accept"
> +		echo "add rule $table $chain counter accept"
> +		echo "add rule $table $chain counter accept"
> +	done
> +done | nft -f - || exit 1
> +
> +do_test 'nft reset rules t1 c2' \
> +	'table=t1 family=2 entries=3 op=nft_reset_rule subj=kernel comm="nft"'
> +
> +do_test 'nft reset rules table t1' \
> +	'table=t1 family=2 entries=9 op=nft_reset_rule subj=kernel comm="nft"'
> +
> +do_test 'nft reset rules' \
> +	'table=t1 family=2 entries=9 op=nft_reset_rule subj=kernel comm="nft"
> +table=t2 family=2 entries=9 op=nft_reset_rule subj=kernel comm="nft"'
> +
> +for ((i = 0; i < 500; i++)); do
> +	echo "add rule t2 c3 counter accept comment \"rule $i\""
> +done | nft -f - || exit 1
> +
> +do_test 'nft reset rules t2 c3' \
> +	'table=t2 family=2 entries=189 op=nft_reset_rule subj=kernel comm="nft"
> +table=t2 family=2 entries=188 op=nft_reset_rule subj=kernel comm="nft"
> +table=t2 family=2 entries=126 op=nft_reset_rule subj=kernel comm="nft"'
> +
> +exit $RC
> -- 
> 2.41.0
> 

Re: [nf-next RFC 2/2] selftests: netfilter: Test nf_tables audit logging

[Phil Sutter]

On Fri, Sep 08, 2023 at 04:56:05PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Sep 08, 2023 at 02:22:29AM +0200, Phil Sutter wrote:
> > Perform ruleset modifications and compare the NETFILTER_CFG type
> > notifications emitted by auditd match expectations.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> > Calling auditd means enabling audit logging in kernel for the remaining
> > uptime. So this test will slow down following ones or even cause
> > spurious failures due to unexpected kernel log entries, timeouts, etc.
> > 
> > Is there a way to test this in a less intrusive way? Maybe fence this
> > test so it does not run automatically (is it any good having it in
> > kernel then)?
> 
> I think you could make a small libmnl program to listen to
> NETLINK_AUDIT events and filter only the logs you need from there. We
> already have a few programs like this in the selftest folder.

Turns out it is indeed possible to turn audit logging off again. I was
obviously misled from auditd not doing it (when killed at least).
Calling 'auditctl -e 0' inside the EXIT trap does the trick.
Implementing a custom audit listener tailored to our case is probably
still a good idea, but at least the biggest obstacle is gone IMO.

Thanks, Phil

Re: [nf-next RFC 2/2] selftests: netfilter: Test nf_tables audit logging

[Paul Moore]

On Fri, Sep 8, 2023 at 10:56 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Sep 08, 2023 at 02:22:29AM +0200, Phil Sutter wrote:
> > Perform ruleset modifications and compare the NETFILTER_CFG type
> > notifications emitted by auditd match expectations.
> >
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> > Calling auditd means enabling audit logging in kernel for the remaining
> > uptime. So this test will slow down following ones or even cause
> > spurious failures due to unexpected kernel log entries, timeouts, etc.
> >
> > Is there a way to test this in a less intrusive way? Maybe fence this
> > test so it does not run automatically (is it any good having it in
> > kernel then)?
>
> I think you could make a small libmnl program to listen to
> NETLINK_AUDIT events and filter only the logs you need from there. We
> already have a few programs like this in the selftest folder.

Just a heads-up that the kernel sends the unicast netlink messages
with a bogus nlmsghdr::nlmsg_len field, see the comments in
audit_log_end() and kauditd_send_multicast_skb() for the details.

-- 
paul-moore.com