linux-kernel - Re: [PATCH v33 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: <86e7613c-6878-21e5-82af-2aa726e230f2@schaufler-ca.com>
Date:   Tue, 15 Mar 2022 17:06:55 -0700
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,
        stephen.smalley.work@...il.com, linux-kernel@...r.kernel.org,
        Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v33 25/29] Audit: Allow multiple records in an
 audit_buffer

On 3/15/2022 4:47 PM, Paul Moore wrote:
> On Thu, Mar 10, 2022 at 6:59 PM Casey Schaufler <casey@...aufler-ca.com> 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>
>> ---
>>   kernel/audit.c | 53 +++++++++++++++++++++++++++++++++-----------------
>>   1 file changed, 35 insertions(+), 18 deletions(-)
>>
>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index f012c3786264..4713e66a12af 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;
>>   };
>>
>> @@ -1744,7 +1746,6 @@ static void audit_buffer_free(struct audit_buffer *ab)
>>          if (!ab)
>>                  return;
>>
>> -       kfree_skb(ab->skb);
> I like the safety in knowing that audit_buffer_free() would free the
> ab->skb memory, I'm not sure I want to get rid of that.  With the
> understanding that ab->skb is always going to be present somewhere in
> ab->skb_list, any reason not to do something like this?
>
>    while ((skb = skb_dequeue(&ab->skb_list)))
>      kfree_skb(skb);

Sure, I'll give this a try. Thanks for the review and suggestions.

>
>>          kmem_cache_free(audit_buffer_cache, ab);
>>   }
>>
>> @@ -1760,11 +1761,15 @@ static struct audit_buffer *audit_buffer_alloc(struct audit_context *ctx,
>>          ab->skb = nlmsg_new(AUDIT_BUFSIZ, gfp_mask);
>>          if (!ab->skb)
>>                  goto err;
>> -       if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
>> +       if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0)) {
>> +               kfree_skb(ab->skb);
>>                  goto err;
>> +       }
> Assuming we restore the audit_buffer_free() functionality as mentioned
> above, if we move the ab->skb_list init and enqueue calls before we
> attempt the nlmsg_put() we can drop the kfree_skb() call and just use
> the existing audit_buffer_free() call at the err target.
>
>
>>          ab->ctx = ctx;
>>          ab->gfp_mask = gfp_mask;
>> +       skb_queue_head_init(&ab->skb_list);
>> +       skb_queue_tail(&ab->skb_list, ab->skb);
>>
>>          return ab;
>>
> --
> paul-moore.com