| Message ID | 20220418145945.38797-26-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v35,01/29] integrity: disassociate ima_filter_rule from security_audit_rule | expand |
On Mon, Apr 18, 2022 at 11:12 AM 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 | 62 +++++++++++++++++++++++++++++++------------------- > 1 file changed, 39 insertions(+), 23 deletions(-) I believe the audit_buffer_aux_new() and audit_buffer_aux_end() functions from patch 26/29 belong in this patch, but otherwise it looks okay to me. Acked-by: Paul Moore <paul@paul-moore.com>
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@paul-moore.com> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> 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? > kmem_cache_free(audit_buffer_cache, ab); > } > > @@ -1784,8 +1789,12 @@ static struct audit_buffer *audit_buffer_alloc(struct audit_context *ctx, > ab->skb = nlmsg_new(AUDIT_BUFSIZ, gfp_mask); > if (!ab->skb) > goto err; > + > + skb_queue_head_init(&ab->skb_list); > + skb_queue_tail(&ab->skb_list, ab->skb); > + > if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0)) > - goto err; > + kfree_skb(ab->skb); why is this no longer an error? > > ab->ctx = ctx; > ab->gfp_mask = gfp_mask; > @@ -1849,7 +1858,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; > @@ -1904,14 +1912,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; > } > @@ -2402,26 +2410,14 @@ int audit_signal_info(int sig, struct task_struct *t) > } > > /** > - * 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. > + * __audit_log_end - enqueue one audit record > + * @skb: the buffer to send > */ > -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); > @@ -2432,6 +2428,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); > }
On Mon, Apr 25, 2022 at 9:06 PM John Johansen <john.johansen@canonical.com> 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@paul-moore.com> > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > > 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.
On 4/26/22 11:12, Paul Moore wrote: > On Mon, Apr 25, 2022 at 9:06 PM John Johansen > <john.johansen@canonical.com> 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@paul-moore.com> >>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> >> 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. > yeah I got that eventually, though it wasn't immediately obvious > 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. > and that is why I asked for a debug check. But its not a hard requirement just a nice to have because I have been bitten by internal consistency issues all to often.
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); kmem_cache_free(audit_buffer_cache, ab); } @@ -1784,8 +1789,12 @@ static struct audit_buffer *audit_buffer_alloc(struct audit_context *ctx, ab->skb = nlmsg_new(AUDIT_BUFSIZ, gfp_mask); if (!ab->skb) goto err; + + skb_queue_head_init(&ab->skb_list); + skb_queue_tail(&ab->skb_list, ab->skb); + if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0)) - goto err; + kfree_skb(ab->skb); ab->ctx = ctx; ab->gfp_mask = gfp_mask; @@ -1849,7 +1858,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; @@ -1904,14 +1912,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; } @@ -2402,26 +2410,14 @@ int audit_signal_info(int sig, struct task_struct *t) } /** - * 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. + * __audit_log_end - enqueue one audit record + * @skb: the buffer to send */ -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); @@ -2432,6 +2428,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 | 62 +++++++++++++++++++++++++++++++------------------- 1 file changed, 39 insertions(+), 23 deletions(-)