Re: [PATCH] audit: add feature audit_lost reset

On Friday, December 16, 2016 5:58:39 PM EST Paul Moore wrote: > On Fri, Dec 16, 2016 at 1:59 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > > Add a method to reset the audit_lost value. > > > > An AUDIT_SET message with the AUDIT_STATUS_LOST flag set by itself > > will return a positive value repesenting the current audit_lost value > > and reset the counter to zero. If AUDIT_STATUS_LOST is not the > > only flag set, the reset command will be ignored. The value sent with > > the command is ignored. > > > > An AUDIT_LOST_RESET message will be queued to the listening audit > > daemon. The message will be similar to a CONFIG_CHANGE message with the > > fields "lost=0" and "old=" containing the value of audit_lost at reset > > ending with a successful result code. > > > > See: https://github.com/linux-audit/audit-kernel/issues/3 > > > > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; > > --- > > v3: Switch, from returing a message to the initiating process, to > > queueing the message for the audit log. > > > > v2: Switch from AUDIT_GET to AUDIT_SET, adding a +ve return code and > > sending a dedicated AUDIT_LOST_RESET message. > > --- > > > > include/uapi/linux/audit.h | 2 ++ > > kernel/audit.c | 16 +++++++++++++++- > > 2 files changed, 17 insertions(+), 1 deletions(-) > > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > > index 208df7b..6d38bff 100644 > > --- a/include/uapi/linux/audit.h > > +++ b/include/uapi/linux/audit.h > > @@ -70,6 +70,7 @@ > > > > #define AUDIT_TTY_SET 1017 /* Set TTY auditing status */ > > #define AUDIT_SET_FEATURE 1018 /* Turn an audit feature on or off > > */ #define AUDIT_GET_FEATURE 1019 /* Get which features are > > enabled */> > > +#define AUDIT_LOST_RESET 1020 /* Reset the audit_lost value */ The 1000 block is for command and control, not logging. If this is used in logging, it should be in the 1300 block. But see below, this probably is not needed.

> > #define AUDIT_FIRST_USER_MSG 1100 /* Userspace messages mostly > > uninteresting to kernel */ #define AUDIT_USER_AVC 1107 /* We > > filter this differently */> > > @@ -325,6 +326,7 @@ enum { > > > > #define AUDIT_STATUS_RATE_LIMIT 0x0008 > > #define AUDIT_STATUS_BACKLOG_LIMIT 0x0010 > > #define AUDIT_STATUS_BACKLOG_WAIT_TIME 0x0020 > > > > +#define AUDIT_STATUS_LOST 0x0040 > > > > #define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT 0x00000001 > > #define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002 > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > index f1ca116..441e8c0 100644 > > --- a/kernel/audit.c > > +++ b/kernel/audit.c > > @@ -122,7 +122,7 @@ > > > > 3) suppressed due to audit_rate_limit > > 4) suppressed due to audit_backlog_limit > > > > */ > > > > -static atomic_t audit_lost = ATOMIC_INIT(0); > > +static atomic_t audit_lost = ATOMIC_INIT(0); > > > > /* The netlink socket. */ > > static struct sock *audit_sock; > > > > @@ -920,6 +920,20 @@ static int audit_receive_msg(struct sk_buff *skb, > > struct nlmsghdr *nlh)> > > if (err < 0) > > > > return err; > > > > } > > > > + if (s.mask == AUDIT_STATUS_LOST) { > > + struct audit_buffer *ab; > > + u32 lost = atomic_xchg(&audit_lost, 0); > > + > > + ab = audit_log_start(NULL, GFP_KERNEL, > > AUDIT_LOST_RESET); + if (unlikely(!ab)) > > + return lost; > > I'm generally not a fan of adding the likely/unlikely optimizations in > non-critial/control path code like this one, but don't respin the > patch just for this. However, if you do have to respin the patch > please fix this. > > > + audit_log_format(ab, "lost=0 old=%u", lost); > > + audit_log_session_info(ab); > > + audit_log_task_context(ab); > > + audit_log_format(ab, " res=1"); > > We're still need to userspace code, so no rush on this, but we should > get Steve's opinion on the format; he'll surely have something to say. So, I'm looking at this and wondering a few things. The config option right above this is audit_set_backlog_wait_time. Wouldn't you want to pattern this after it? IOW, make an audit_reset_lost function which calls audit_do_config_change() which calls audit_log_config_change()? This would make the event type CONFIG_CHANGE instead of LOST_RESET. Since no one is counting on this event at the moment, no one has software that would try to interpret LOST_RESET events so we can change it.

I'm thinking its probably more important to be consistent than creating a new event type. I admit, I didn't follow this whole thread in detail and maybe there was a good reason to separate out LOST_RESET. By using audit_do_config_change() you also become consistent with other rules like if config changes are disallowed because we are immutable.

These changes are on the logging side. This won't affect integration with auditctl. If you do want to keep LOST_RESET, then it affects all searching and reporting utilities.

From a search and reporting perspective CONFIG_CHANGE will make it much

-Steve > > + audit_log_end(ab); > > + return lost; > > + } > > > > break; > > > > } > > > > case AUDIT_GET_FEATURE: > > -- > > 1.7.1 > > > > -- > > Linux-audit mailing list > > Linux-audit(a)redhat.com > > https://www.redhat.com/mailman/listinfo/linux-audit