Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <cover.1577830902.git.rgb@redhat.com> <b1b2e6f917816c4ae85b53d7f93c10c3d1df4a53.1577830902.git.rgb@redhat.com>
In-Reply-To: <b1b2e6f917816c4ae85b53d7f93c10c3d1df4a53.1577830902.git.rgb@redhat.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Thu, 30 Jan 2020 22:18:07 -0500
Message-ID: <CAHC9VhRSRggBD9QgXD7-YEx=qY7Ym_1D12y3anAihE=9P7r-6w@mail.gmail.com>
Subject: Re: [PATCH ghak25 v2 4/9] audit: record nfcfg params
To:     Richard Guy Briggs <rgb@redhat.com>
Cc:     Linux-Audit Mailing List <linux-audit@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>,
        netfilter-devel@vger.kernel.org, sgrubb@redhat.com,
        omosnace@redhat.com, fw@strlen.de, twoerner@redhat.com,
        Eric Paris <eparis@parisplace.org>, ebiederm@xmission.com,
        tgraf@infradead.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jan 6, 2020 at 1:55 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Record the auditable parameters of any non-empty netfilter table
> configuration change.
>
> See: https://github.com/linux-audit/audit-kernel/issues/25
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/linux/audit.h | 11 +++++++++++
>  kernel/auditsc.c      | 16 ++++++++++++++++
>  2 files changed, 27 insertions(+)

I can not see a good reason why this patch is separate from patches 5
and 6, please squash them down into one patch.  As it currently stands
the logging function introduced here has no caller so it is pointless
by itself.  Strive to make an individual patch have some significance
on its own whenever possible.

This will also help you write a better commit description, right now
the commit description tells me nothing, but if you bring in the other
patches you can talk about consolidating similar code into a common
function.

> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index f9ceae57ca8d..96cabb095eed 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -379,6 +379,7 @@ extern int __audit_log_bprm_fcaps(struct linux_binprm *bprm,
>  extern void __audit_fanotify(unsigned int response);
>  extern void __audit_tk_injoffset(struct timespec64 offset);
>  extern void __audit_ntp_log(const struct audit_ntp_data *ad);
> +extern void __audit_nf_cfg(const char *name, u8 af, int nentries);
>
>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>  {
> @@ -514,6 +515,12 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad)
>                 __audit_ntp_log(ad);
>  }
>
> +static inline void audit_nf_cfg(const char *name, u8 af, int nentries)
> +{
> +       if (!audit_dummy_context())
> +               __audit_nf_cfg(name, af, nentries);

See my comments below about audit_enabled.

> +}
> +
>  extern int audit_n_rules;
>  extern int audit_signals;
>  #else /* CONFIG_AUDITSYSCALL */
> @@ -646,6 +653,10 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad)
>
>  static inline void audit_ptrace(struct task_struct *t)
>  { }
> +
> +static inline void audit_nf_cfg(const char *name, u8 af, int nentries)
> +{ }
> +
>  #define audit_n_rules 0
>  #define audit_signals 0
>  #endif /* CONFIG_AUDITSYSCALL */
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 4effe01ebbe2..4e1df4233cd3 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2545,6 +2545,22 @@ void __audit_ntp_log(const struct audit_ntp_data *ad)
>         audit_log_ntp_val(ad, "adjust", AUDIT_NTP_ADJUST);
>  }
>
> +void __audit_nf_cfg(const char *name, u8 af, int nentries)

Should nentries be an unsigned int?

> +{
> +       struct audit_buffer *ab;
> +       struct audit_context *context = audit_context();

This is a good example of why the context of a caller matters; taken
alone I would say that we need a check for audit_enabled here, but if
we look at the latter patches we can see that the caller already has
the audit_enabled check.

Considering that the caller is already doing an audit_enabled check,
we might want to consider moving the audit_enabled check into
audit_nf_cfg() where we do the dummy context check.  It's a static
inline so there shouldn't be a performance impact and it makes the
caller's code cleaner.

> +       if (!nentries)
> +               return;
> +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_NETFILTER_CFG);

Why do we need the context variable, why not just call audit_context()
here directly?

> +       if (!ab)
> +               return; /* audit_panic or being filtered */

We generally don't add comments when audit_log_start() fails
elsewhere, please don't do it here.

> +       audit_log_format(ab, "table=%s family=%u entries=%u",
> +                        name, af, nentries);
> +       audit_log_end(ab);
> +}
> +EXPORT_SYMBOL_GPL(__audit_nf_cfg);
> +
>  static void audit_log_task(struct audit_buffer *ab)
>  {
>         kuid_t auid, uid;
> --
> 1.8.3.1

--
paul moore
www.paul-moore.com