[PATCH] audit: add feature audit_lost reset
Richard Guy Briggs
rgb at redhat.com
Thu Jan 12 04:19:42 UTC 2017
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 <rgb at redhat.com>
> 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 at redhat.com>
> > > > ---
> > > > 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
> > > > Linux-audit at redhat.com
> > > > https://www.redhat.com/mailman/listinfo/linux-audit
> >
> > --
> > Linux-audit mailing list
> > Linux-audit at redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
>
>
- RGB
--
Richard Guy Briggs <rgb at redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
More information about the Linux-audit
mailing list