On Wed, Sep 06, 2023 at 01:32:50PM +0200, Phil Sutter wrote:
> On Wed, Sep 06, 2023 at 11:42:02AM +0200, Pablo Neira Ayuso wrote:
> > Deliver audit log from __nf_tables_dump_rules(), table dereference at
> > the end of the table list loop might point to the list head, leading to
> > this crash.
>
> Ah, of course. Sorry for the mess. I missed the fact that one may just
> call 'reset rules' and have it apply to all existing tables.
>
> [...]
> > Deliver audit log only once at the end of the rule dump+reset for
> > consistency with the set dump+reset.
>
> This may seem like number of audit logs is reduced, when it is actually
> increased: With your patch, there will be at least one notification for
> each chain, multiple with large chains (due to skb exhaustion).
With your patch, this is called for each netlink_recvmsg() invocation,
which is more audit logs.
@@ -3540,9 +3544,6 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
done:
rcu_read_unlock();
- if (reset && idx > cb->args[0])
- audit_log_rule_reset(table, cb->seq, idx - cb->args[0]);
-
cb->args[0] = idx;
return skb->len;
}
netlink dump is composed of a series of recvmsg() from userspace to
fetch the multiple chunks that represent the rules in this table.
If I read fine above, as the netlink dump makes progress, idx will
always go over cb->args[0], which evaluates true for each recvmsg()
call. With my patch this is less audit logs because audit log is only
delivered once for each chain, not for each recvmsg() call.
So patch description is fine as it is :)
> Not necessarily a problem, but worth mentioning. Also, I wonder if
> one should go the extra mile and add the chain name to log entries.
> I had considered to pass a string like "mytable:123 chain=mychain"
> to audit_log_nfcfg() for that matter, but it's quite a hack.
Agreed. Audit folks can extend this later on according to their needs
in case they need more fine grain reporting.
> > Ensure audit reset access to table under rcu read side lock. The table
> > list iteration holds rcu read lock side, but recent audit code
> > dereferences table object out of the rcu read lock side.
> >
> > Fixes: ea078ae9108e ("netfilter: nf_tables: Audit log rule reset")
> > Fixes: 7e9be1124dbe ("netfilter: nf_tables: Audit log setelem reset")
> > Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
>
> Acked-by: Phil Sutter <phil@xxxxxx>
