Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220310234632.16194-1-casey@schaufler-ca.com> <20220310234632.16194-26-casey@schaufler-ca.com>
In-Reply-To: <20220310234632.16194-26-casey@schaufler-ca.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Tue, 15 Mar 2022 19:47:36 -0400
Message-ID: <CAHC9VhTkXaJ6nsJU9hf9KO22bGSpyr8EeBQKef-f6jhy_6OEkA@mail.gmail.com>
Subject: Re: [PATCH v33 25/29] Audit: Allow multiple records in an audit_buffer
To:     Casey Schaufler <casey@schaufler-ca.com>
Cc:     casey.schaufler@intel.com, jmorris@namei.org,
        linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
        linux-audit@redhat.com, keescook@chromium.org,
        john.johansen@canonical.com, penguin-kernel@i-love.sakura.ne.jp,
        stephen.smalley.work@gmail.com, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Mar 10, 2022 at 6:59 PM Casey Schaufler <casey@schaufler-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@paul-moore.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-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);

>         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