Message-ID: <0bbd2d61-415f-08f2-251e-2dd6b8991d6a@schaufler-ca.com>
Date:   Thu, 3 Mar 2022 18:13:47 -0800
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     casey.schaufler@...el.com, jmorris@...ei.org,
        linux-security-module@...r.kernel.org, selinux@...r.kernel.org,
        linux-audit@...hat.com, keescook@...omium.org,
        john.johansen@...onical.com, penguin-kernel@...ove.sakura.ne.jp,
        sds@...ho.nsa.gov, linux-kernel@...r.kernel.org,
        Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v32 24/28] Audit: Add framework for auxiliary records

On 3/3/2022 3:36 PM, Paul Moore wrote:
> On Wed, Feb 2, 2022 at 7:20 PM Casey Schaufler <casey@...aufler-ca.com> wrote:
>> Add a list for auxiliary record data to the audit_buffer structure.
>> 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.
>>
>> Signed-off-by: Casey Schaufler <casey@...aufler-ca.com>
>> ---
>>   kernel/audit.c | 84 ++++++++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 74 insertions(+), 10 deletions(-)
>>
>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index f012c3786264..559fb14e0380 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
>> @@ -191,15 +191,25 @@ static struct audit_ctl_mutex {
>>    * should be at least that large. */
>>   #define AUDIT_BUFSIZ 1024
>>
>> +/* The audit_context_entry contains data required to create an
>> + * auxiliary record.
>> + */
>> +struct audit_context_entry {
>> +       struct list_head        list;
>> +       int                     type;   /* Audit record type */
>> +};
> Looking at how this ends up being used later in the patchset I think
> we would be better off if we stored a fully formed audit_buffer in the
> struct above instead of data fields which we would use to generate an
> audit_buffer in audit_log_end().  This helps tie the buffer generation
> logic in with the existing code with which it is most closely related,
> it allows us to report errors back to the caller as audit_log_end()
> doesn't historically return an error code, and it helps us get ahead
> of any future data lifetime issues we might run into by storing the
> data in this audit struct.

OK, I'll buy that.

> This would also simplify things with respect to the audit_buffer
> struct.  Instead of having a dedicated struct for the aux data, you
> could simply leverage the existing sk_buff list mechanisms:

I can't say that "simply" is the adverb I'd choose, but sure,
I can do this.

>    struct audit_buffer {
>      struct sk_buff *skb;  /* part of @skb_list, kept for audit_log funcs */
>      struct sk_buff_head skb_list;
>      struct audit_context *ctx;
>      struct audit_stamp stamp;
>      gfp_t gfp_mask;
>    }
>
> The only sneaky bit in the struct above is that we likely want to
> preserve audit_buffer::skb as a dedicated skb pointer so we don't have
> to modify all of the audit_log_*() functions; you could of course, but
> I'm guessing there is little appetite for that in the context of this
> patchset.

I will give it a go without making the massive interface change.

> Adding a new aux record would involve calling some private audit
> function (no one outside of the audit subsystem should need access)
> that would allocate a new skb similar to what we do in
> audit_buffer_alloc() and add it to the end of the sk_buff_head list
> via skb_queue_tail() and resetting audit_buffer::skb to point to the
> newly allocated skb.

Good naming may be tricky as we need to indicate that a new buffer is
being allocated for an attached aux record and that the buffer to which
it's being attached is going to temporarily be in a curious state.
audit_buffer_add_aux() seems wordy, but it's what I'll start with lacking
a better suggestion.

>    This would allow all of the existing
> audit_log*() functions to work correctly, and when you are done you
> can restore the "main" skb with skb_peek().

audit_buffer_close_aux()

>    If for some reason you
> need to fail the new aux record mid-creation you just dequeue the list
> tail, free the skb, and skb_peek() the "main" skb back into place.

Why do I always get nervous when I hear "just" and "skb" in the
same sentence?

>>   /* The audit_buffer is used when formatting an audit record.  The caller
>>    * locks briefly to get the record off the freelist or to allocate the
>>    * buffer, and locks briefly to send the buffer to the netlink layer or
>>    * 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 audit_context *ctx;      /* NULL or associated context */
>> -       gfp_t                gfp_mask;
>> +       struct sk_buff          *skb;   /* formatted skb ready to send */
>> +       struct audit_context    *ctx;   /* NULL or associated context */
>> +       struct list_head        aux_records;    /* aux record data */
>> +       struct audit_stamp      stamp;  /* event stamp */
>> +       gfp_t                   gfp_mask;
>>   };
> ...
>
>> @@ -2408,6 +2418,60 @@ void audit_log_end(struct audit_buffer *ab)
>>                  wake_up_interruptible(&kauditd_wait);
>>          } else
>>                  audit_log_lost("rate limit exceeded");
>> +}
>> +
>> +/**
>> + * audit_log_end - end one audit record
>> + * @ab: the audit_buffer
>> + *
>> + * Let __audit_log_end() handle the message while the buffer housekeeping
>> + * is done here.
>> + * If there are other records that have been deferred for the event
>> + * create them here.
>> + */
>> +void audit_log_end(struct audit_buffer *ab)
>> +{
>> +       struct audit_context_entry *entry;
>> +       struct audit_context mcontext;
>> +       struct audit_context *mctx;
>> +       struct audit_buffer *mab;
>> +       struct list_head *l;
>> +       struct list_head *n;
>> +
>> +       if (!ab)
>> +               return;
>> +
>> +       __audit_log_end(ab);
>> +
>> +       if (list_empty(&ab->aux_records)) {
>> +               audit_buffer_free(ab);
>> +               return;
>> +       }
>> +
>> +       if (ab->ctx == NULL) {
>> +               mcontext.stamp = ab->stamp;
>> +               mctx = &mcontext;
>> +       } else
>> +               mctx = ab->ctx;
>> +
>> +       list_for_each_safe(l, n, &ab->aux_records) {
>> +               entry = list_entry(l, struct audit_context_entry, list);
>> +               mab = audit_log_start(mctx, ab->gfp_mask, entry->type);
>> +               if (!mab) {
>> +                       audit_panic("alloc error in audit_log_end");
>> +                       continue;
>> +               }
>> +               switch (entry->type) {
>> +               /* Don't know of any quite yet. */
>> +               default:
>> +                       audit_panic("Unknown type in audit_log_end");
>> +                       break;
>> +               }
>> +               __audit_log_end(mab);
>> +               audit_buffer_free(mab);
>> +               list_del(&entry->list);
>> +               kfree(entry);
>> +       }
>>
>>          audit_buffer_free(ab);
>>   }
> This would also allow you to simplify audit_log_end() greatly, I'm
> sure I'm missing a detail or two, but I suspect it would end up
> looking something like this:

Agreed. That is a much better fit for the existing code flow.

>
>    void __audit_log_end(skb)
>    {
>      /* ... current audit_log_end() but with only the sk_buff ... */
>    }
>
>    void audit_log_end(ab)
>    {
>      if (!ab)
>        return;
>      while ((skb = skb_dequeue(ab->skb_list)))
>        __audit_log_end(skb);
>      audit_buffer_free(ab);
>    }
>
>
> --
> paul-moore.com

Powered by blists - more mailing lists