[PATCH] audit: add feature audit_lost reset
Richard Guy Briggs
rgb at redhat.com
Thu Jan 12 04:12:47 UTC 2017
On 2017-01-11 13:56, 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.
Fair enough. This won't even be needed if we use CONFIG_CHANGE.
> > > #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.
It isn't upstream yet so now is the time to get it right.
It does in many ways fit into CONFIG_CHANGE. It was suggested by Paul
Moore that it use a message type similar to CONFIG_CHANGE. It isn't
strictly a config change. Do we want it treated like a config change in
that the operation is prevented if audit_enabled == AUDIT_LOCKED? This
does seem reasonable. We could switch it seperately from config
immutable mode by using the set feature facility (not to be confused
with the feature bitmap).
> 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.
The thread wasn't *that* long...
Slotting it in to CONFIG_CHANGE does make sense to me.
> 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.
Can you define "on the logging side" and what implications that has?
Do you not want to be able to trigger this from auditctl? I agree
putting this in CONFIG_CHANGE will likely make your job easier. There
are some minor differences including checking that the feature exists
either by verifying that the operation succeeded the first time you try
it or by using the feature bitmap or set feature and actually using the
positive return code lost value. There is also the question of how to
respond when it isn't the only flag set in the AUDIT_SET command.
Silently exit having executed the other flags? Return an error before
processing any of the commands? The latter makes more sense to me.
>From a search and reporting perspective CONFIG_CHANGE will make it much
easier.
> -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
>
>
- 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