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

On Mon, Apr 25, 2022 at 9:06 PM John Johansen <john.johansen(a)canonical.com&gt; wrote: > On 4/18/22 07:59, Casey Schaufler 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(a)paul-moore.com&gt; >> Signed-off-by: Casey Schaufler <casey(a)schaufler-ca.com&gt; > > I agree with Paul that audit_buffer_aux_new() and > audit_buffer_aux_end() belong in this patch > > >> --- >> kernel/audit.c | 62 +++++++++++++++++++++++++++++++------------------- >> 1 file changed, 39 insertions(+), 23 deletions(-) >> >> diff --git a/kernel/audit.c b/kernel/audit.c >> index 6b6c089512f7..4d44c05053b0 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; >> }; >> >> @@ -1765,10 +1767,13 @@ __setup("audit_backlog_limit=", audit_backlog_limit_set); >> >> static void audit_buffer_free(struct audit_buffer *ab) >> { >> + struct sk_buff *skb; >> + >> if (!ab) >> return; >> >> - kfree_skb(ab->skb); >> + while((skb = skb_dequeue(&ab->skb_list))) >> + kfree_skb(skb); > > we still have and ab->skb can we have a debug check that its freed by walking the queue? By definition ab->skb is always going to point at something on the list, if it doesn't we are likely to have failures elsewhere. The structure definition is private to kernel/audit.c and the allocation/creation is handled by an allocator function which always adds the new skb to the list so I think we're okay.

We could add additional checks, but with audit performance already a hot topic I would prefer to draw the debug-check line at input coming from outside the audit subsystem.