On 2023-09-06 13:47, Pablo Neira Ayuso wrote:
> 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.
(I think I missed the start of this thread?)
Whatever you do, only ever add new fields to the *end* of the record to
avoid confusing the userspace parser. I vaguely remember looking at
adding a chain field and dismissed it.
So if this needs to be done, the string needs to be parsed to see if
extra fields have been added and print them at the end of the record.
The better approach is to add a parameter to carry that field and
optionally append it to the record.
> 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>
- RGB
--
Richard Guy Briggs <rgb@xxxxxxxxxx>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
Upstream IRC: SunRaycer
Voice: +1.613.860 2354 SMS: +1.613.518.6570
