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

From: Casey Schaufler
Date: Tue Mar 15 2022 - 20:07:12 EST


On 3/15/2022 4:47 PM, Paul Moore wrote:
On Thu, Mar 10, 2022 at 6:59 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
---
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