Re: [PATCH nf] netfilter: nf_tables: Unbreak audit log reset

[Richard Guy Briggs <rgb@redhat.com>]

On 2023-09-06 14:20, Richard Guy Briggs wrote:
> 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.

And given that this could potentially affect userspace parsing tools,
the original audit mailing list linux-audit@redhat.com should also be
copied.  Sorry about that...

> 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@netfilter.org>
> > > 
> > > Acked-by: Phil Sutter <phil@nwl.cc>
> 
> - RGB

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
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