[PATCH] audit: add feature audit_lost reset

[Steve Grubb]

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.

-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