| Message ID | 20220310234632.16194-26-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v33,01/29] integrity: disassociate ima_filter_rule from security_audit_rule | expand |
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
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 --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); }
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(-)