linux-kernel - Re: [PATCH v35 25/29] Audit: Allow multiple records in an audit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHC9VhRX+VSw+-PsCwhHceQ9MpE4E-D-OnaO0CGqar44xc3a1w@mail.gmail.com>
Date:   Tue, 26 Apr 2022 14:12:44 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     John Johansen <john.johansen@...onical.com>
Cc:     Casey Schaufler <casey@...aufler-ca.com>,
        casey.schaufler@...el.com, jmorris@...ei.org,
        linux-security-module@...r.kernel.org, selinux@...r.kernel.org,
        linux-audit@...hat.com, keescook@...omium.org,
        penguin-kernel@...ove.sakura.ne.jp, stephen.smalley.work@...il.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v35 25/29] Audit: Allow multiple records in an audit_buffer

On Mon, Apr 25, 2022 at 9:06 PM John Johansen
<john.johansen@...onical.com> wrote:
> On 4/18/22 07:59, Casey Schaufler wrote:
> > Replace the single skb pointer in an audit_buffer with
> > a list of skb pointers. Add the audit_stamp information
> > to the audit_buffer as there's no guarantee that there
> > will be an audit_context containing the stamp associated
> > with the event. At audit_log_end() time create auxiliary
> > records (none are currently defined) as have been added
> > to the list.
> >
> > Suggested-by: Paul Moore <paul@...l-moore.com>
> > Signed-off-by: Casey Schaufler <casey@...aufler-ca.com>
>
> I agree with Paul that audit_buffer_aux_new() and
> audit_buffer_aux_end() belong in this patch
>
>
> > ---
> >  kernel/audit.c | 62 +++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 39 insertions(+), 23 deletions(-)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 6b6c089512f7..4d44c05053b0 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -197,8 +197,10 @@ static struct audit_ctl_mutex {
> >   * to place it on a transmit queue.  Multiple audit_buffers can be in
> >   * use simultaneously. */
> >  struct audit_buffer {
> > -     struct sk_buff       *skb;      /* formatted skb ready to send */
> > +     struct sk_buff       *skb;      /* the skb for audit_log functions */
> > +     struct sk_buff_head  skb_list;  /* formatted skbs, ready to send */
> >       struct audit_context *ctx;      /* NULL or associated context */
> > +     struct audit_stamp   stamp;     /* audit stamp for these records */
> >       gfp_t                gfp_mask;
> >  };
> >
> > @@ -1765,10 +1767,13 @@ __setup("audit_backlog_limit=", audit_backlog_limit_set);
> >
> >  static void audit_buffer_free(struct audit_buffer *ab)
> >  {
> > +     struct sk_buff *skb;
> > +
> >       if (!ab)
> >               return;
> >
> > -     kfree_skb(ab->skb);
> > +     while((skb = skb_dequeue(&ab->skb_list)))
> > +             kfree_skb(skb);
>
> we still have and ab->skb can we have a debug check that its freed by walking the queue?

By definition ab->skb is always going to point at something on the
list, if it doesn't we are likely to have failures elsewhere.  The
structure definition is private to kernel/audit.c and the
allocation/creation is handled by an allocator function which always
adds the new skb to the list so I think we're okay.

We could add additional checks, but with audit performance already a
hot topic I would prefer to draw the debug-check line at input coming
from outside the audit subsystem.

-- 
paul-moore.com