On 2017-01-11 18:35, Steve Grubb wrote: > On Wednesday, January 11, 2017 1:56:46 PM EST Steve Grubb wrote: > > 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 <[email protected]> > 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 <[email protected]> > > > > --- > > > > 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. > > OK. the code to support this is in svn. However, since we didn't use a > feature > bit like we normally do, there is absolutely no way to report that the > underlying kernel does not support this. It quietly fails and pretends > everything is fine. I'd prefer that we had a feature bit to output a proper > error message.
Do you still want to switch to CONFIG_CHANGE? (I think that is a good idea.) I agree detecting this feature is a destructive operation requiring an existing lost count and checking the positive return code, but not impossible, and would prefer a feature bit. As for audit being immutable, I could see an argument to have this feature usable even though the config is locked. What's your take? > -Steve > > > > > + audit_log_end(ab); > > > > + return lost; > > > > + } > > > > > > > > break; > > > > > > > > } > > > > > > > > case AUDIT_GET_FEATURE: > > > > -- > > > > 1.7.1 > > > > > > > > -- > > > > Linux-audit mailing list > > > > [email protected] > > > > https://www.redhat.com/mailman/listinfo/linux-audit > > > > -- > > Linux-audit mailing list > > [email protected] > > https://www.redhat.com/mailman/listinfo/linux-audit > > - RGB -- Richard Guy Briggs <[email protected]> Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635 -- Linux-audit mailing list [email protected] https://www.redhat.com/mailman/listinfo/linux-audit
