Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 24 Jun 2020 09:26:12 -0400
From:   Richard Guy Briggs <rgb@redhat.com>
To:     Pablo Neira Ayuso <pablo@netfilter.org>
Cc:     Linux-Audit Mailing List <linux-audit@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>,
        netfilter-devel@vger.kernel.org, Paul Moore <paul@paul-moore.com>,
        sgrubb@redhat.com, omosnace@redhat.com, fw@strlen.de,
        twoerner@redhat.com, eparis@parisplace.org, tgraf@infradead.org
Subject: Re: [PATCH ghak124 v3] audit: log nftables configuration change
 events
Message-ID: <20200624132612.fj36hwgom7qryvn7@madcap2.tricolour.ca>
References: <f9da8b5dbf2396b621c77c17b5b1123be5aa484e.1591275439.git.rgb@redhat.com>
 <20200624100346.GA11986@salvia>
 <20200624123423.r2gypsdii6xgiywy@madcap2.tricolour.ca>
 <20200624130304.GA549@salvia>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200624130304.GA549@salvia>
User-Agent: NeoMutt/20180716
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2020-06-24 15:03, Pablo Neira Ayuso wrote:
> On Wed, Jun 24, 2020 at 08:34:23AM -0400, Richard Guy Briggs wrote:
> > On 2020-06-24 12:03, Pablo Neira Ayuso wrote:
> > > On Thu, Jun 04, 2020 at 09:20:49AM -0400, Richard Guy Briggs wrote:
> [...]
> > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > > > index 3558e76e2733..b9e7440cc87d 100644
> > > > --- a/net/netfilter/nf_tables_api.c
> > > > +++ b/net/netfilter/nf_tables_api.c
> > > > @@ -12,6 +12,7 @@
> > > >  #include <linux/netlink.h>
> > > >  #include <linux/vmalloc.h>
> > > >  #include <linux/rhashtable.h>
> > > > +#include <linux/audit.h>
> > > >  #include <linux/netfilter.h>
> > > >  #include <linux/netfilter/nfnetlink.h>
> > > >  #include <linux/netfilter/nf_tables.h>
> > > > @@ -693,6 +694,16 @@ static void nf_tables_table_notify(const struct nft_ctx *ctx, int event)
> > > >  {
> > > >  	struct sk_buff *skb;
> > > >  	int err;
> > > > +	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;?:0",
> > > > +			      ctx->table->name, ctx->table->handle);
> > > > +
> > > > +	audit_log_nfcfg(buf,
> > > > +			ctx->family,
> > > > +			ctx->table->use,
> > > > +			event == NFT_MSG_NEWTABLE ?
> > > > +				AUDIT_NFT_OP_TABLE_REGISTER :
> > > > +				AUDIT_NFT_OP_TABLE_UNREGISTER);
> > > > +	kfree(buf);
> > > 
> > > As a follow up: Would you wrap this code into a function?
> > > 
> > >         nft_table_audit()
> > > 
> > > Same thing for other pieces of code below.
> > 
> > If I'm guessing right, you are asking for a supplementary follow-up
> > cleanup patch to this one (or are you nacking this patch)?
> 
> No nack, it's just that I'd prefer to see this wrapped in a function.
> I think your patch is already in the audit tree.
> 
> > Also, I gather you would like to see the kasprintf and kfree hidden in
> > nft_table_audit(), handing this function at least 8 parameters?  This
> > sounds pretty messy given the format of the table field.
> 
> I think you can pass ctx and the specific object, e.g. table, in most
> cases? There is also event and the gfp_flags. That counts 4 here, but
> maybe I'm overlooking something.

Since every event is sufficiently different, it isn't as simple as
passing ctx, unfortunately, and the table field I've overloaded with 4
bits of information for tracking the chain as well, some of which are ?
that would need an in-band representation (such as -1? that might
already be valid).  So 4 right there, family, nentries, event, gfp for 8.

I did try in the first patch to make it just one call keyed on event,
but there was enough variety of information available for each message
type that it became necessary to break it out.

> Thanks.

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635