diff mbox series

[v33,25/29] Audit: Allow multiple records in an audit_buffer

Message ID 20220310234632.16194-26-casey@schaufler-ca.com (mailing list archive)
State New, archived
Headers show
Series LSM: Module stacking for AppArmor | expand

Commit Message

Casey Schaufler March 10, 2022, 11:46 p.m. UTC
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(-)

Comments

Paul Moore March 15, 2022, 11:47 p.m. UTC | #1
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
Casey Schaufler March 16, 2022, 12:06 a.m. UTC | #2
On 3/15/2022 4:47 PM, Paul Moore wrote:
> 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);

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
diff mbox series

Patch

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);
 	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;
+	}
 
 	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;
 
@@ -1825,7 +1830,6 @@  struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
 				     int type)
 {
 	struct audit_buffer *ab;
-	struct audit_stamp stamp;
 
 	if (audit_initialized != AUDIT_INITIALIZED)
 		return NULL;
@@ -1880,14 +1884,14 @@  struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
 		return NULL;
 	}
 
-	audit_get_stamp(ab->ctx, &stamp);
+	audit_get_stamp(ab->ctx, &ab->stamp);
 	/* cancel dummy context to enable supporting records */
 	if (ctx)
 		ctx->dummy = 0;
 	audit_log_format(ab, "audit(%llu.%03lu:%u): ",
-			 (unsigned long long)stamp.ctime.tv_sec,
-			 stamp.ctime.tv_nsec/1000000,
-			 stamp.serial);
+			 (unsigned long long)ab->stamp.ctime.tv_sec,
+			 ab->stamp.ctime.tv_nsec/1000000,
+			 ab->stamp.serial);
 
 	return ab;
 }
@@ -2378,26 +2382,19 @@  int audit_signal_info(int sig, struct task_struct *t)
 }
 
 /**
- * audit_log_end - end one audit record
- * @ab: the audit_buffer
+ * __audit_log_end - end one audit record
+ * @skb: the buffer to send
  *
  * We can not do a netlink send inside an irq context because it blocks (last
  * arg, flags, is not set to MSG_DONTWAIT), so the audit buffer is placed on a
  * queue and a kthread is scheduled to remove them from the queue outside the
  * irq context.  May be called in any context.
  */
-void audit_log_end(struct audit_buffer *ab)
+static void __audit_log_end(struct sk_buff *skb)
 {
-	struct sk_buff *skb;
 	struct nlmsghdr *nlh;
 
-	if (!ab)
-		return;
-
 	if (audit_rate_check()) {
-		skb = ab->skb;
-		ab->skb = NULL;
-
 		/* setup the netlink header, see the comments in
 		 * kauditd_send_multicast_skb() for length quirks */
 		nlh = nlmsg_hdr(skb);
@@ -2408,6 +2405,26 @@  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
+ *
+ * We can not do a netlink send inside an irq context because it blocks (last
+ * arg, flags, is not set to MSG_DONTWAIT), so the audit buffer is placed on a
+ * queue and a kthread is scheduled to remove them from the queue outside the
+ * irq context.  May be called in any context.
+ */
+void audit_log_end(struct audit_buffer *ab)
+{
+	struct sk_buff *skb;
+
+	if (!ab)
+		return;
+
+	while ((skb = skb_dequeue(&ab->skb_list)))
+		__audit_log_end(skb);
 
 	audit_buffer_free(ab);
 }