[RFC][PATCH] audit: add feature audit_lost reset

[RFC][PATCH] audit: add feature audit_lost reset

[Richard Guy Briggs]

Add a method to reset the audit_lost value.

An AUDIT_GET message will get the current audit_lost value and reset the
counter to zero iff (if and only if) the AUDIT_FEATURE_LOST_RESET
feature is set.

If the flag AUDIT_FEATURE_BITMAP_LOST_RESET is present in the audit
feature bitmap, the feature is settable by setting the
AUDIT_FEATURE_LOST_RESET flag in the audit feature list with an
AUDIT_SET_FEATURE call.  This setting is lockable.

See: https://github.com/linux-audit/audit-kernel/issues/3

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
Note: The AUDIT_FEATURE_BITMAP_LOST_RESET check may not be necessary if
it is possible to read all the entries from audit_feature_names from
userspace.
---
 include/uapi/linux/audit.h |    7 +++++--
 kernel/audit.c             |    9 ++++++---
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 208df7b..5eb2dc2 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -330,10 +330,12 @@ enum {
 #define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME	0x00000002
 #define AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH	0x00000004
 #define AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND	0x00000008
+#define AUDIT_FEATURE_BITMAP_LOST_RESET		0x00000010
 #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
 				  AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
 				  AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH | \
-				  AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND)
+				  AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
+				  AUDIT_FEATURE_BITMAP_LOST_RESET)
 
 /* deprecated: AUDIT_VERSION_* */
 #define AUDIT_VERSION_LATEST 		AUDIT_FEATURE_BITMAP_ALL
@@ -440,7 +442,8 @@ struct audit_features {
 
 #define AUDIT_FEATURE_ONLY_UNSET_LOGINUID	0
 #define AUDIT_FEATURE_LOGINUID_IMMUTABLE	1
-#define AUDIT_LAST_FEATURE			AUDIT_FEATURE_LOGINUID_IMMUTABLE
+#define AUDIT_FEATURE_LOST_RESET		2
+#define AUDIT_LAST_FEATURE			AUDIT_FEATURE_LOST_RESET
 
 #define audit_feature_valid(x)		((x) >= 0 && (x) <= AUDIT_LAST_FEATURE)
 #define AUDIT_FEATURE_TO_MASK(x)	(1 << ((x) & 31)) /* mask for __u32 */
diff --git a/kernel/audit.c b/kernel/audit.c
index f1ca116..6b52da6 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;
@@ -150,9 +150,10 @@
 				   .features = 0,
 				   .lock = 0,};
 
-static char *audit_feature_names[2] = {
+static char *audit_feature_names[3] = {
 	"only_unset_loginuid",
 	"loginuid_immutable",
+	"lost_reset",
 };
 
 
@@ -854,7 +855,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		s.pid			= audit_pid;
 		s.rate_limit		= audit_rate_limit;
 		s.backlog_limit		= audit_backlog_limit;
-		s.lost			= atomic_read(&audit_lost);
+		s.lost			= is_audit_feature_set(AUDIT_FEATURE_LOST_RESET) ?
+						atomic_xchg(&audit_lost, 0) :
+						atomic_read(&audit_lost);
 		s.backlog		= skb_queue_len(&audit_skb_queue);
 		s.feature_bitmap	= AUDIT_FEATURE_BITMAP_ALL;
 		s.backlog_wait_time	= audit_backlog_wait_time_master;
-- 
1.7.1

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Paul Moore]

On Mon, Dec 5, 2016 at 3:02 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Add a method to reset the audit_lost value.
>
> An AUDIT_GET message will get the current audit_lost value and reset the
> counter to zero iff (if and only if) the AUDIT_FEATURE_LOST_RESET
> feature is set.
>
> If the flag AUDIT_FEATURE_BITMAP_LOST_RESET is present in the audit
> feature bitmap, the feature is settable by setting the
> AUDIT_FEATURE_LOST_RESET flag in the audit feature list with an
> AUDIT_SET_FEATURE call.  This setting is lockable.
>
> See: https://github.com/linux-audit/audit-kernel/issues/3
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> Note: The AUDIT_FEATURE_BITMAP_LOST_RESET check may not be necessary if
> it is possible to read all the entries from audit_feature_names from
> userspace.
> ---
>  include/uapi/linux/audit.h |    7 +++++--
>  kernel/audit.c             |    9 ++++++---
>  2 files changed, 11 insertions(+), 5 deletions(-)

Instead of resetting the lost counter on an AUDIT_GET if the reset
feature is set, how about preserving the AUDIT_GET behavior, skipping
the AUDIT_FEATURE_* addition, and simply reset the lost value by
sending a AUDIT_SET message with AUDIT_STATUS_LOST (you obviously have
to add this to the uapi header).

I'm mixed on adding this to the feature bitmap, it shouldn't be
strictly necessary as old kernels will simply ignore the
AUDIT_SET/AUDIT_STATUS_LOST bit, but I can understand if userspace
might want it ... I just hate to burn a bit in the bitmap for
something that has no ill effect on behavior.

> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 208df7b..5eb2dc2 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -330,10 +330,12 @@ enum {
>  #define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002
>  #define AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH   0x00000004
>  #define AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND    0x00000008
> +#define AUDIT_FEATURE_BITMAP_LOST_RESET                0x00000010
>  #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
>                                   AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
>                                   AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH | \
> -                                 AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND)
> +                                 AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
> +                                 AUDIT_FEATURE_BITMAP_LOST_RESET)
>
>  /* deprecated: AUDIT_VERSION_* */
>  #define AUDIT_VERSION_LATEST           AUDIT_FEATURE_BITMAP_ALL
> @@ -440,7 +442,8 @@ struct audit_features {
>
>  #define AUDIT_FEATURE_ONLY_UNSET_LOGINUID      0
>  #define AUDIT_FEATURE_LOGINUID_IMMUTABLE       1
> -#define AUDIT_LAST_FEATURE                     AUDIT_FEATURE_LOGINUID_IMMUTABLE
> +#define AUDIT_FEATURE_LOST_RESET               2
> +#define AUDIT_LAST_FEATURE                     AUDIT_FEATURE_LOST_RESET
>
>  #define audit_feature_valid(x)         ((x) >= 0 && (x) <= AUDIT_LAST_FEATURE)
>  #define AUDIT_FEATURE_TO_MASK(x)       (1 << ((x) & 31)) /* mask for __u32 */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index f1ca116..6b52da6 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;
> @@ -150,9 +150,10 @@
>                                    .features = 0,
>                                    .lock = 0,};
>
> -static char *audit_feature_names[2] = {
> +static char *audit_feature_names[3] = {
>         "only_unset_loginuid",
>         "loginuid_immutable",
> +       "lost_reset",
>  };
>
>
> @@ -854,7 +855,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>                 s.pid                   = audit_pid;
>                 s.rate_limit            = audit_rate_limit;
>                 s.backlog_limit         = audit_backlog_limit;
> -               s.lost                  = atomic_read(&audit_lost);
> +               s.lost                  = is_audit_feature_set(AUDIT_FEATURE_LOST_RESET) ?
> +                                               atomic_xchg(&audit_lost, 0) :
> +                                               atomic_read(&audit_lost);
>                 s.backlog               = skb_queue_len(&audit_skb_queue);
>                 s.feature_bitmap        = AUDIT_FEATURE_BITMAP_ALL;
>                 s.backlog_wait_time     = audit_backlog_wait_time_master;
> --
> 1.7.1
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit



-- 
paul moore
www.paul-moore.com

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Richard Guy Briggs]

On 2016-12-05 11:02, Paul Moore wrote:
> On Mon, Dec 5, 2016 at 3:02 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Add a method to reset the audit_lost value.
> >
> > An AUDIT_GET message will get the current audit_lost value and reset the
> > counter to zero iff (if and only if) the AUDIT_FEATURE_LOST_RESET
> > feature is set.
> >
> > If the flag AUDIT_FEATURE_BITMAP_LOST_RESET is present in the audit
> > feature bitmap, the feature is settable by setting the
> > AUDIT_FEATURE_LOST_RESET flag in the audit feature list with an
> > AUDIT_SET_FEATURE call.  This setting is lockable.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/3
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > Note: The AUDIT_FEATURE_BITMAP_LOST_RESET check may not be necessary if
> > it is possible to read all the entries from audit_feature_names from
> > userspace.
> > ---
> >  include/uapi/linux/audit.h |    7 +++++--
> >  kernel/audit.c             |    9 ++++++---
> >  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> Instead of resetting the lost counter on an AUDIT_GET if the reset
> feature is set, how about preserving the AUDIT_GET behavior, skipping
> the AUDIT_FEATURE_* addition, and simply reset the lost value by
> sending a AUDIT_SET message with AUDIT_STATUS_LOST (you obviously have
> to add this to the uapi header).

I realized as I was coding it up that we would potentially lose an
accurate count if the read and reset were not atomic.  This was the
reason for using atomic_xchg().

> I'm mixed on adding this to the feature bitmap, it shouldn't be
> strictly necessary as old kernels will simply ignore the
> AUDIT_SET/AUDIT_STATUS_LOST bit, but I can understand if userspace
> might want it ... I just hate to burn a bit in the bitmap for
> something that has no ill effect on behavior.

As pointed out, we may not need the bitmap addition if we can read the
array of audit_feature_names.

> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 208df7b..5eb2dc2 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -330,10 +330,12 @@ enum {
> >  #define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002
> >  #define AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH   0x00000004
> >  #define AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND    0x00000008
> > +#define AUDIT_FEATURE_BITMAP_LOST_RESET                0x00000010
> >  #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> >                                   AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
> >                                   AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH | \
> > -                                 AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND)
> > +                                 AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
> > +                                 AUDIT_FEATURE_BITMAP_LOST_RESET)
> >
> >  /* deprecated: AUDIT_VERSION_* */
> >  #define AUDIT_VERSION_LATEST           AUDIT_FEATURE_BITMAP_ALL
> > @@ -440,7 +442,8 @@ struct audit_features {
> >
> >  #define AUDIT_FEATURE_ONLY_UNSET_LOGINUID      0
> >  #define AUDIT_FEATURE_LOGINUID_IMMUTABLE       1
> > -#define AUDIT_LAST_FEATURE                     AUDIT_FEATURE_LOGINUID_IMMUTABLE
> > +#define AUDIT_FEATURE_LOST_RESET               2
> > +#define AUDIT_LAST_FEATURE                     AUDIT_FEATURE_LOST_RESET
> >
> >  #define audit_feature_valid(x)         ((x) >= 0 && (x) <= AUDIT_LAST_FEATURE)
> >  #define AUDIT_FEATURE_TO_MASK(x)       (1 << ((x) & 31)) /* mask for __u32 */
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index f1ca116..6b52da6 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;
> > @@ -150,9 +150,10 @@
> >                                    .features = 0,
> >                                    .lock = 0,};
> >
> > -static char *audit_feature_names[2] = {
> > +static char *audit_feature_names[3] = {
> >         "only_unset_loginuid",
> >         "loginuid_immutable",
> > +       "lost_reset",
> >  };
> >
> >
> > @@ -854,7 +855,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> >                 s.pid                   = audit_pid;
> >                 s.rate_limit            = audit_rate_limit;
> >                 s.backlog_limit         = audit_backlog_limit;
> > -               s.lost                  = atomic_read(&audit_lost);
> > +               s.lost                  = is_audit_feature_set(AUDIT_FEATURE_LOST_RESET) ?
> > +                                               atomic_xchg(&audit_lost, 0) :
> > +                                               atomic_read(&audit_lost);
> >                 s.backlog               = skb_queue_len(&audit_skb_queue);
> >                 s.feature_bitmap        = AUDIT_FEATURE_BITMAP_ALL;
> >                 s.backlog_wait_time     = audit_backlog_wait_time_master;
> > --
> > 1.7.1
> >
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
> 
> 
> 
> -- 
> paul moore
> www.paul-moore.com

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Paul Moore]

On Mon, Dec 5, 2016 at 11:52 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2016-12-05 11:02, Paul Moore wrote:
>> On Mon, Dec 5, 2016 at 3:02 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > Add a method to reset the audit_lost value.
>> >
>> > An AUDIT_GET message will get the current audit_lost value and reset the
>> > counter to zero iff (if and only if) the AUDIT_FEATURE_LOST_RESET
>> > feature is set.
>> >
>> > If the flag AUDIT_FEATURE_BITMAP_LOST_RESET is present in the audit
>> > feature bitmap, the feature is settable by setting the
>> > AUDIT_FEATURE_LOST_RESET flag in the audit feature list with an
>> > AUDIT_SET_FEATURE call.  This setting is lockable.
>> >
>> > See: https://github.com/linux-audit/audit-kernel/issues/3
>> >
>> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> > ---
>> > Note: The AUDIT_FEATURE_BITMAP_LOST_RESET check may not be necessary if
>> > it is possible to read all the entries from audit_feature_names from
>> > userspace.
>> > ---
>> >  include/uapi/linux/audit.h |    7 +++++--
>> >  kernel/audit.c             |    9 ++++++---
>> >  2 files changed, 11 insertions(+), 5 deletions(-)
>>
>> Instead of resetting the lost counter on an AUDIT_GET if the reset
>> feature is set, how about preserving the AUDIT_GET behavior, skipping
>> the AUDIT_FEATURE_* addition, and simply reset the lost value by
>> sending a AUDIT_SET message with AUDIT_STATUS_LOST (you obviously have
>> to add this to the uapi header).
>
> I realized as I was coding it up that we would potentially lose an
> accurate count if the read and reset were not atomic.  This was the
> reason for using atomic_xchg().

Well, okay, but that isn't what I was talking about ... ?  The
audit_cmd_mutex is held for both AUDIT_GET and AUDIT_SET so we should
never process these requests at the same time.

-- 
paul moore
www.paul-moore.com

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Richard Guy Briggs]

On 2016-12-05 12:48, Paul Moore wrote:
> On Mon, Dec 5, 2016 at 11:52 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2016-12-05 11:02, Paul Moore wrote:
> >> On Mon, Dec 5, 2016 at 3:02 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> > Add a method to reset the audit_lost value.
> >> >
> >> > An AUDIT_GET message will get the current audit_lost value and reset the
> >> > counter to zero iff (if and only if) the AUDIT_FEATURE_LOST_RESET
> >> > feature is set.
> >> >
> >> > If the flag AUDIT_FEATURE_BITMAP_LOST_RESET is present in the audit
> >> > feature bitmap, the feature is settable by setting the
> >> > AUDIT_FEATURE_LOST_RESET flag in the audit feature list with an
> >> > AUDIT_SET_FEATURE call.  This setting is lockable.
> >> >
> >> > See: https://github.com/linux-audit/audit-kernel/issues/3
> >> >
> >> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >> > ---
> >> > Note: The AUDIT_FEATURE_BITMAP_LOST_RESET check may not be necessary if
> >> > it is possible to read all the entries from audit_feature_names from
> >> > userspace.
> >> > ---
> >> >  include/uapi/linux/audit.h |    7 +++++--
> >> >  kernel/audit.c             |    9 ++++++---
> >> >  2 files changed, 11 insertions(+), 5 deletions(-)
> >>
> >> Instead of resetting the lost counter on an AUDIT_GET if the reset
> >> feature is set, how about preserving the AUDIT_GET behavior, skipping
> >> the AUDIT_FEATURE_* addition, and simply reset the lost value by
> >> sending a AUDIT_SET message with AUDIT_STATUS_LOST (you obviously have
> >> to add this to the uapi header).
> >
> > I realized as I was coding it up that we would potentially lose an
> > accurate count if the read and reset were not atomic.  This was the
> > reason for using atomic_xchg().
> 
> Well, okay, but that isn't what I was talking about ... ?  The
> audit_cmd_mutex is held for both AUDIT_GET and AUDIT_SET so we should
> never process these requests at the same time.

I guess I still don't follow what you are talking about...  I hadn't
thought of including both an AUDIT_GET and an AUDIT_SET in the same skb,
but that would ensure that no other command reads or resets the lost
value.  However, the lost value could still be incrementing on another
CPU between these two commands, losing an accurate lost count.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Paul Moore]

On Tue, Dec 6, 2016 at 12:13 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2016-12-05 12:48, Paul Moore wrote:
>> On Mon, Dec 5, 2016 at 11:52 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > On 2016-12-05 11:02, Paul Moore wrote:
>> >> On Mon, Dec 5, 2016 at 3:02 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> >> > Add a method to reset the audit_lost value.
>> >> >
>> >> > An AUDIT_GET message will get the current audit_lost value and reset the
>> >> > counter to zero iff (if and only if) the AUDIT_FEATURE_LOST_RESET
>> >> > feature is set.
>> >> >
>> >> > If the flag AUDIT_FEATURE_BITMAP_LOST_RESET is present in the audit
>> >> > feature bitmap, the feature is settable by setting the
>> >> > AUDIT_FEATURE_LOST_RESET flag in the audit feature list with an
>> >> > AUDIT_SET_FEATURE call.  This setting is lockable.
>> >> >
>> >> > See: https://github.com/linux-audit/audit-kernel/issues/3
>> >> >
>> >> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> >> > ---
>> >> > Note: The AUDIT_FEATURE_BITMAP_LOST_RESET check may not be necessary if
>> >> > it is possible to read all the entries from audit_feature_names from
>> >> > userspace.
>> >> > ---
>> >> >  include/uapi/linux/audit.h |    7 +++++--
>> >> >  kernel/audit.c             |    9 ++++++---
>> >> >  2 files changed, 11 insertions(+), 5 deletions(-)
>> >>
>> >> Instead of resetting the lost counter on an AUDIT_GET if the reset
>> >> feature is set, how about preserving the AUDIT_GET behavior, skipping
>> >> the AUDIT_FEATURE_* addition, and simply reset the lost value by
>> >> sending a AUDIT_SET message with AUDIT_STATUS_LOST (you obviously have
>> >> to add this to the uapi header).
>> >
>> > I realized as I was coding it up that we would potentially lose an
>> > accurate count if the read and reset were not atomic.  This was the
>> > reason for using atomic_xchg().
>>
>> Well, okay, but that isn't what I was talking about ... ?  The
>> audit_cmd_mutex is held for both AUDIT_GET and AUDIT_SET so we should
>> never process these requests at the same time.
>
> I guess I still don't follow what you are talking about...  I hadn't
> thought of including both an AUDIT_GET and an AUDIT_SET in the same skb,
> but that would ensure that no other command reads or resets the lost
> value.  However, the lost value could still be incrementing on another
> CPU between these two commands, losing an accurate lost count.

Okay, back up ... this whole mess about atomic_xchg() was always
unrelated to my original suggestion, let's focus on my original
comment ... don't reset the counter on a AUDIT_GET, reset it on a
AUDIT_SET with an AUDIT_STATUS_LOST, does that make sense?

-- 
paul moore
www.paul-moore.com

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Richard Guy Briggs]

On 2016-12-06 19:17, Paul Moore wrote:
> On Tue, Dec 6, 2016 at 12:13 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2016-12-05 12:48, Paul Moore wrote:
> >> On Mon, Dec 5, 2016 at 11:52 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> > On 2016-12-05 11:02, Paul Moore wrote:
> >> >> On Mon, Dec 5, 2016 at 3:02 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> >> > Add a method to reset the audit_lost value.
> >> >> >
> >> >> > An AUDIT_GET message will get the current audit_lost value and reset the
> >> >> > counter to zero iff (if and only if) the AUDIT_FEATURE_LOST_RESET
> >> >> > feature is set.
> >> >> >
> >> >> > If the flag AUDIT_FEATURE_BITMAP_LOST_RESET is present in the audit
> >> >> > feature bitmap, the feature is settable by setting the
> >> >> > AUDIT_FEATURE_LOST_RESET flag in the audit feature list with an
> >> >> > AUDIT_SET_FEATURE call.  This setting is lockable.
> >> >> >
> >> >> > See: https://github.com/linux-audit/audit-kernel/issues/3
> >> >> >
> >> >> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >> >> > ---
> >> >> > Note: The AUDIT_FEATURE_BITMAP_LOST_RESET check may not be necessary if
> >> >> > it is possible to read all the entries from audit_feature_names from
> >> >> > userspace.
> >> >> > ---
> >> >> >  include/uapi/linux/audit.h |    7 +++++--
> >> >> >  kernel/audit.c             |    9 ++++++---
> >> >> >  2 files changed, 11 insertions(+), 5 deletions(-)
> >> >>
> >> >> Instead of resetting the lost counter on an AUDIT_GET if the reset
> >> >> feature is set, how about preserving the AUDIT_GET behavior, skipping
> >> >> the AUDIT_FEATURE_* addition, and simply reset the lost value by
> >> >> sending a AUDIT_SET message with AUDIT_STATUS_LOST (you obviously have
> >> >> to add this to the uapi header).
> >> >
> >> > I realized as I was coding it up that we would potentially lose an
> >> > accurate count if the read and reset were not atomic.  This was the
> >> > reason for using atomic_xchg().
> >>
> >> Well, okay, but that isn't what I was talking about ... ?  The
> >> audit_cmd_mutex is held for both AUDIT_GET and AUDIT_SET so we should
> >> never process these requests at the same time.
> >
> > I guess I still don't follow what you are talking about...  I hadn't
> > thought of including both an AUDIT_GET and an AUDIT_SET in the same skb,
> > but that would ensure that no other command reads or resets the lost
> > value.  However, the lost value could still be incrementing on another
> > CPU between these two commands, losing an accurate lost count.
> 
> Okay, back up ... this whole mess about atomic_xchg() was always
> unrelated to my original suggestion, let's focus on my original
> comment ... don't reset the counter on a AUDIT_GET, reset it on a
> AUDIT_SET with an AUDIT_STATUS_LOST, does that make sense?

I understood that.  It sounds like a nice simple and straightforward
method to do it but for the question of accuracy.  Please rewind to my
fundamental point: How do we get an accurate reading of the last value
of audit_lost before resetting it?

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Paul Moore]

On Tue, Dec 6, 2016 at 10:32 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2016-12-06 19:17, Paul Moore wrote:
>> On Tue, Dec 6, 2016 at 12:13 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > On 2016-12-05 12:48, Paul Moore wrote:
>> >> On Mon, Dec 5, 2016 at 11:52 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> >> > On 2016-12-05 11:02, Paul Moore wrote:
>> >> >> On Mon, Dec 5, 2016 at 3:02 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> >> >> > Add a method to reset the audit_lost value.
>> >> >> >
>> >> >> > An AUDIT_GET message will get the current audit_lost value and reset the
>> >> >> > counter to zero iff (if and only if) the AUDIT_FEATURE_LOST_RESET
>> >> >> > feature is set.
>> >> >> >
>> >> >> > If the flag AUDIT_FEATURE_BITMAP_LOST_RESET is present in the audit
>> >> >> > feature bitmap, the feature is settable by setting the
>> >> >> > AUDIT_FEATURE_LOST_RESET flag in the audit feature list with an
>> >> >> > AUDIT_SET_FEATURE call.  This setting is lockable.
>> >> >> >
>> >> >> > See: https://github.com/linux-audit/audit-kernel/issues/3
>> >> >> >
>> >> >> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> >> >> > ---
>> >> >> > Note: The AUDIT_FEATURE_BITMAP_LOST_RESET check may not be necessary if
>> >> >> > it is possible to read all the entries from audit_feature_names from
>> >> >> > userspace.
>> >> >> > ---
>> >> >> >  include/uapi/linux/audit.h |    7 +++++--
>> >> >> >  kernel/audit.c             |    9 ++++++---
>> >> >> >  2 files changed, 11 insertions(+), 5 deletions(-)
>> >> >>
>> >> >> Instead of resetting the lost counter on an AUDIT_GET if the reset
>> >> >> feature is set, how about preserving the AUDIT_GET behavior, skipping
>> >> >> the AUDIT_FEATURE_* addition, and simply reset the lost value by
>> >> >> sending a AUDIT_SET message with AUDIT_STATUS_LOST (you obviously have
>> >> >> to add this to the uapi header).
>> >> >
>> >> > I realized as I was coding it up that we would potentially lose an
>> >> > accurate count if the read and reset were not atomic.  This was the
>> >> > reason for using atomic_xchg().
>> >>
>> >> Well, okay, but that isn't what I was talking about ... ?  The
>> >> audit_cmd_mutex is held for both AUDIT_GET and AUDIT_SET so we should
>> >> never process these requests at the same time.
>> >
>> > I guess I still don't follow what you are talking about...  I hadn't
>> > thought of including both an AUDIT_GET and an AUDIT_SET in the same skb,
>> > but that would ensure that no other command reads or resets the lost
>> > value.  However, the lost value could still be incrementing on another
>> > CPU between these two commands, losing an accurate lost count.
>>
>> Okay, back up ... this whole mess about atomic_xchg() was always
>> unrelated to my original suggestion, let's focus on my original
>> comment ... don't reset the counter on a AUDIT_GET, reset it on a
>> AUDIT_SET with an AUDIT_STATUS_LOST, does that make sense?
>
> I understood that.  It sounds like a nice simple and straightforward
> method to do it but for the question of accuracy.  Please rewind to my
> fundamental point: How do we get an accurate reading of the last value
> of audit_lost before resetting it?

Okay, I thought you were worried about a different race, which is why
this discussion wasn't making much sense to me.  I understand your
point, but I really dislike the API; although that's not your fault,
it's really the only way to do it via AUDIT_GET.

I'd much prefer we go with the cleaner AUDIT_SET approach and just not
worry about the small race window.  It would only be an issue if you
reset the count under heavy audit load, and why would you reset the
lost value if you were under a heavy audit load?  That just doesn't
make sense.

I suppose we should hear from Steve on this since he was the one who
has been asking for this feature, although I'm pretty sure I know what
he is going to say.

-- 
paul moore
www.paul-moore.com

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Steve Grubb]

On Wednesday, December 7, 2016 10:05:30 AM EST Paul Moore wrote:
> On Tue, Dec 6, 2016 at 10:32 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2016-12-06 19:17, Paul Moore wrote:
> >> On Tue, Dec 6, 2016 at 12:13 AM, Richard Guy Briggs <rgb@redhat.com> 
> >> Okay, back up ... this whole mess about atomic_xchg() was always
> >> unrelated to my original suggestion, let's focus on my original
> >> comment ... don't reset the counter on a AUDIT_GET, reset it on a
> >> AUDIT_SET with an AUDIT_STATUS_LOST, does that make sense?
> > 
> > I understood that.  It sounds like a nice simple and straightforward
> > method to do it but for the question of accuracy.  Please rewind to my
> > fundamental point: How do we get an accurate reading of the last value
> > of audit_lost before resetting it?
> 
> Okay, I thought you were worried about a different race, which is why
> this discussion wasn't making much sense to me.  I understand your
> point, but I really dislike the API; although that's not your fault,
> it's really the only way to do it via AUDIT_GET.
> 
> I'd much prefer we go with the cleaner AUDIT_SET approach and just not
> worry about the small race window.  It would only be an issue if you
> reset the count under heavy audit load, and why would you reset the
> lost value if you were under a heavy audit load?  That just doesn't
> make sense.
> 
> I suppose we should hear from Steve on this since he was the one who
> has been asking for this feature, although I'm pretty sure I know what
> he is going to say.

To start with, this request comes from users of the audit system. I just 
passed along the request. The issue is that when you do auditctl -s, you get 
the number of records lost. If you do it the next day, you have to do math to 
see what the one day delta is. So, to make reporting easy, they want it to be 
reset whenever they do audictl -s.

You could also make a AUDIT_GET_RESET that gets the status and resets the 
number atomically. Then I can add another commandline option to auditctl that 
allows an admin to say also reset the counters. If that command line option is 
passed, I call AUDIT_GET_RESET otherwise I call AUDIT_GET. Thought?

-Steve

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Richard Guy Briggs]

On 2016-12-07 10:05, Paul Moore wrote:
> On Tue, Dec 6, 2016 at 10:32 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2016-12-06 19:17, Paul Moore wrote:
> >> On Tue, Dec 6, 2016 at 12:13 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> > On 2016-12-05 12:48, Paul Moore wrote:
> >> >> On Mon, Dec 5, 2016 at 11:52 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> >> > On 2016-12-05 11:02, Paul Moore wrote:
> >> >> >> On Mon, Dec 5, 2016 at 3:02 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> >> >> > Add a method to reset the audit_lost value.
> >> >> >> >
> >> >> >> > An AUDIT_GET message will get the current audit_lost value and reset the
> >> >> >> > counter to zero iff (if and only if) the AUDIT_FEATURE_LOST_RESET
> >> >> >> > feature is set.
> >> >> >> >
> >> >> >> > If the flag AUDIT_FEATURE_BITMAP_LOST_RESET is present in the audit
> >> >> >> > feature bitmap, the feature is settable by setting the
> >> >> >> > AUDIT_FEATURE_LOST_RESET flag in the audit feature list with an
> >> >> >> > AUDIT_SET_FEATURE call.  This setting is lockable.
> >> >> >> >
> >> >> >> > See: https://github.com/linux-audit/audit-kernel/issues/3
> >> >> >> >
> >> >> >> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >> >> >> > ---
> >> >> >> > Note: The AUDIT_FEATURE_BITMAP_LOST_RESET check may not be necessary if
> >> >> >> > it is possible to read all the entries from audit_feature_names from
> >> >> >> > userspace.
> >> >> >> > ---
> >> >> >> >  include/uapi/linux/audit.h |    7 +++++--
> >> >> >> >  kernel/audit.c             |    9 ++++++---
> >> >> >> >  2 files changed, 11 insertions(+), 5 deletions(-)
> >> >> >>
> >> >> >> Instead of resetting the lost counter on an AUDIT_GET if the reset
> >> >> >> feature is set, how about preserving the AUDIT_GET behavior, skipping
> >> >> >> the AUDIT_FEATURE_* addition, and simply reset the lost value by
> >> >> >> sending a AUDIT_SET message with AUDIT_STATUS_LOST (you obviously have
> >> >> >> to add this to the uapi header).
> >> >> >
> >> >> > I realized as I was coding it up that we would potentially lose an
> >> >> > accurate count if the read and reset were not atomic.  This was the
> >> >> > reason for using atomic_xchg().
> >> >>
> >> >> Well, okay, but that isn't what I was talking about ... ?  The
> >> >> audit_cmd_mutex is held for both AUDIT_GET and AUDIT_SET so we should
> >> >> never process these requests at the same time.
> >> >
> >> > I guess I still don't follow what you are talking about...  I hadn't
> >> > thought of including both an AUDIT_GET and an AUDIT_SET in the same skb,
> >> > but that would ensure that no other command reads or resets the lost
> >> > value.  However, the lost value could still be incrementing on another
> >> > CPU between these two commands, losing an accurate lost count.
> >>
> >> Okay, back up ... this whole mess about atomic_xchg() was always
> >> unrelated to my original suggestion, let's focus on my original
> >> comment ... don't reset the counter on a AUDIT_GET, reset it on a
> >> AUDIT_SET with an AUDIT_STATUS_LOST, does that make sense?
> >
> > I understood that.  It sounds like a nice simple and straightforward
> > method to do it but for the question of accuracy.  Please rewind to my
> > fundamental point: How do we get an accurate reading of the last value
> > of audit_lost before resetting it?
> 
> Okay, I thought you were worried about a different race, which is why
> this discussion wasn't making much sense to me.  I understand your
> point, but I really dislike the API; although that's not your fault,
> it's really the only way to do it via AUDIT_GET.
> 
> I'd much prefer we go with the cleaner AUDIT_SET approach and just not
> worry about the small race window.  It would only be an issue if you
> reset the count under heavy audit load, and why would you reset the
> lost value if you were under a heavy audit load?  That just doesn't
> make sense.

I agree the AUDIT_SET approach is much more elegant, which is the
original way I had envisioned doing it, but it is lossy, and I'd lean
towards accuracy and reliability than uncertainty and doubt.

> I suppose we should hear from Steve on this since he was the one who
> has been asking for this feature, although I'm pretty sure I know what
> he is going to say.
> 
> -- 
> paul moore
> www.paul-moore.com

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Richard Guy Briggs]

On 2016-12-07 10:53, Steve Grubb wrote:
> On Wednesday, December 7, 2016 10:05:30 AM EST Paul Moore wrote:
> > On Tue, Dec 6, 2016 at 10:32 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2016-12-06 19:17, Paul Moore wrote:
> > >> On Tue, Dec 6, 2016 at 12:13 AM, Richard Guy Briggs <rgb@redhat.com> 
> > >> Okay, back up ... this whole mess about atomic_xchg() was always
> > >> unrelated to my original suggestion, let's focus on my original
> > >> comment ... don't reset the counter on a AUDIT_GET, reset it on a
> > >> AUDIT_SET with an AUDIT_STATUS_LOST, does that make sense?
> > > 
> > > I understood that.  It sounds like a nice simple and straightforward
> > > method to do it but for the question of accuracy.  Please rewind to my
> > > fundamental point: How do we get an accurate reading of the last value
> > > of audit_lost before resetting it?
> > 
> > Okay, I thought you were worried about a different race, which is why
> > this discussion wasn't making much sense to me.  I understand your
> > point, but I really dislike the API; although that's not your fault,
> > it's really the only way to do it via AUDIT_GET.
> > 
> > I'd much prefer we go with the cleaner AUDIT_SET approach and just not
> > worry about the small race window.  It would only be an issue if you
> > reset the count under heavy audit load, and why would you reset the
> > lost value if you were under a heavy audit load?  That just doesn't
> > make sense.
> > 
> > I suppose we should hear from Steve on this since he was the one who
> > has been asking for this feature, although I'm pretty sure I know what
> > he is going to say.
> 
> To start with, this request comes from users of the audit system. I just 
> passed along the request. The issue is that when you do auditctl -s, you get 
> the number of records lost. If you do it the next day, you have to do math to 
> see what the one day delta is. So, to make reporting easy, they want it to be 
> reset whenever they do audictl -s.
> 
> You could also make a AUDIT_GET_RESET that gets the status and resets the 
> number atomically. Then I can add another commandline option to auditctl that 
> allows an admin to say also reset the counters. If that command line option is 
> passed, I call AUDIT_GET_RESET otherwise I call AUDIT_GET. Thought?

This would be slightly simpler in kernel implementation than the method
I proposed and would work fine, off the top of my head.

> -Steve

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Paul Moore]

On Wed, Dec 7, 2016 at 10:58 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2016-12-07 10:53, Steve Grubb wrote:
>> On Wednesday, December 7, 2016 10:05:30 AM EST Paul Moore wrote:
>> > On Tue, Dec 6, 2016 at 10:32 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > > On 2016-12-06 19:17, Paul Moore wrote:
>> > >> On Tue, Dec 6, 2016 at 12:13 AM, Richard Guy Briggs <rgb@redhat.com>
>> > >> Okay, back up ... this whole mess about atomic_xchg() was always
>> > >> unrelated to my original suggestion, let's focus on my original
>> > >> comment ... don't reset the counter on a AUDIT_GET, reset it on a
>> > >> AUDIT_SET with an AUDIT_STATUS_LOST, does that make sense?
>> > >
>> > > I understood that.  It sounds like a nice simple and straightforward
>> > > method to do it but for the question of accuracy.  Please rewind to my
>> > > fundamental point: How do we get an accurate reading of the last value
>> > > of audit_lost before resetting it?
>> >
>> > Okay, I thought you were worried about a different race, which is why
>> > this discussion wasn't making much sense to me.  I understand your
>> > point, but I really dislike the API; although that's not your fault,
>> > it's really the only way to do it via AUDIT_GET.
>> >
>> > I'd much prefer we go with the cleaner AUDIT_SET approach and just not
>> > worry about the small race window.  It would only be an issue if you
>> > reset the count under heavy audit load, and why would you reset the
>> > lost value if you were under a heavy audit load?  That just doesn't
>> > make sense.
>> >
>> > I suppose we should hear from Steve on this since he was the one who
>> > has been asking for this feature, although I'm pretty sure I know what
>> > he is going to say.
>>
>> To start with, this request comes from users of the audit system. I just
>> passed along the request. The issue is that when you do auditctl -s, you get
>> the number of records lost. If you do it the next day, you have to do math to
>> see what the one day delta is. So, to make reporting easy, they want it to be
>> reset whenever they do audictl -s.
>>
>> You could also make a AUDIT_GET_RESET that gets the status and resets the
>> number atomically. Then I can add another commandline option to auditctl that
>> allows an admin to say also reset the counters. If that command line option is
>> passed, I call AUDIT_GET_RESET otherwise I call AUDIT_GET. Thought?
>
> This would be slightly simpler in kernel implementation than the method
> I proposed and would work fine, off the top of my head.

I'd prefer not to introduce another command message type for something
small like this.

Steve, do you have any objection to the AUDIT_SET based approach?
Based on what you've said above, it would seem like the potential race
condition with AUDIT_SET wouldn't be a significant issue.

-- 
paul moore
www.paul-moore.com

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Steve Grubb]

On Wednesday, December 7, 2016 6:10:49 PM EST Paul Moore wrote:
> On Wed, Dec 7, 2016 at 10:58 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2016-12-07 10:53, Steve Grubb wrote:
> >> On Wednesday, December 7, 2016 10:05:30 AM EST Paul Moore wrote:
> >> > On Tue, Dec 6, 2016 at 10:32 PM, Richard Guy Briggs <rgb@redhat.com> 
wrote:
> >> > > On 2016-12-06 19:17, Paul Moore wrote:
> >> > >> On Tue, Dec 6, 2016 at 12:13 AM, Richard Guy Briggs <rgb@redhat.com>
> >> > >> Okay, back up ... this whole mess about atomic_xchg() was always
> >> > >> unrelated to my original suggestion, let's focus on my original
> >> > >> comment ... don't reset the counter on a AUDIT_GET, reset it on a
> >> > >> AUDIT_SET with an AUDIT_STATUS_LOST, does that make sense?
> >> > > 
> >> > > I understood that.  It sounds like a nice simple and straightforward
> >> > > method to do it but for the question of accuracy.  Please rewind to
> >> > > my
> >> > > fundamental point: How do we get an accurate reading of the last
> >> > > value
> >> > > of audit_lost before resetting it?
> >> > 
> >> > Okay, I thought you were worried about a different race, which is why
> >> > this discussion wasn't making much sense to me.  I understand your
> >> > point, but I really dislike the API; although that's not your fault,
> >> > it's really the only way to do it via AUDIT_GET.
> >> > 
> >> > I'd much prefer we go with the cleaner AUDIT_SET approach and just not
> >> > worry about the small race window.  It would only be an issue if you
> >> > reset the count under heavy audit load, and why would you reset the
> >> > lost value if you were under a heavy audit load?  That just doesn't
> >> > make sense.
> >> > 
> >> > I suppose we should hear from Steve on this since he was the one who
> >> > has been asking for this feature, although I'm pretty sure I know what
> >> > he is going to say.
> >> 
> >> To start with, this request comes from users of the audit system. I just
> >> passed along the request. The issue is that when you do auditctl -s, you
> >> get the number of records lost. If you do it the next day, you have to
> >> do math to see what the one day delta is. So, to make reporting easy,
> >> they want it to be reset whenever they do audictl -s.
> >> 
> >> You could also make a AUDIT_GET_RESET that gets the status and resets the
> >> number atomically. Then I can add another commandline option to auditctl
> >> that allows an admin to say also reset the counters. If that command
> >> line option is passed, I call AUDIT_GET_RESET otherwise I call
> >> AUDIT_GET. Thought?> 
> > This would be slightly simpler in kernel implementation than the method
> > I proposed and would work fine, off the top of my head.
> 
> I'd prefer not to introduce another command message type for something
> small like this.
> 
> Steve, do you have any objection to the AUDIT_SET based approach?

Either way, we'd need a feature flag so that I can tell if the kernel supports 
this or not. Also, it should accept "0" as the only valid value. We can do 
this and I can make auditctl do the two back to back before displaying the 
results to minimize the window of risk.

> Based on what you've said above, it would seem like the potential race
> condition with AUDIT_SET wouldn't be a significant issue.

All a matter of perspective. What I think is a reasonable risk someone else 
may disagree. Does anyone else on the list object? If not I'd say go with it.

-Steve

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Paul Moore]

On Wed, Dec 7, 2016 at 6:30 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Wednesday, December 7, 2016 6:10:49 PM EST Paul Moore wrote:
>> On Wed, Dec 7, 2016 at 10:58 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > On 2016-12-07 10:53, Steve Grubb wrote:
>> >> On Wednesday, December 7, 2016 10:05:30 AM EST Paul Moore wrote:
>> >> > On Tue, Dec 6, 2016 at 10:32 PM, Richard Guy Briggs <rgb@redhat.com>
> wrote:
>> >> > > On 2016-12-06 19:17, Paul Moore wrote:
>> >> > >> On Tue, Dec 6, 2016 at 12:13 AM, Richard Guy Briggs <rgb@redhat.com>
>> >> > >> Okay, back up ... this whole mess about atomic_xchg() was always
>> >> > >> unrelated to my original suggestion, let's focus on my original
>> >> > >> comment ... don't reset the counter on a AUDIT_GET, reset it on a
>> >> > >> AUDIT_SET with an AUDIT_STATUS_LOST, does that make sense?
>> >> > >
>> >> > > I understood that.  It sounds like a nice simple and straightforward
>> >> > > method to do it but for the question of accuracy.  Please rewind to
>> >> > > my
>> >> > > fundamental point: How do we get an accurate reading of the last
>> >> > > value
>> >> > > of audit_lost before resetting it?
>> >> >
>> >> > Okay, I thought you were worried about a different race, which is why
>> >> > this discussion wasn't making much sense to me.  I understand your
>> >> > point, but I really dislike the API; although that's not your fault,
>> >> > it's really the only way to do it via AUDIT_GET.
>> >> >
>> >> > I'd much prefer we go with the cleaner AUDIT_SET approach and just not
>> >> > worry about the small race window.  It would only be an issue if you
>> >> > reset the count under heavy audit load, and why would you reset the
>> >> > lost value if you were under a heavy audit load?  That just doesn't
>> >> > make sense.
>> >> >
>> >> > I suppose we should hear from Steve on this since he was the one who
>> >> > has been asking for this feature, although I'm pretty sure I know what
>> >> > he is going to say.
>> >>
>> >> To start with, this request comes from users of the audit system. I just
>> >> passed along the request. The issue is that when you do auditctl -s, you
>> >> get the number of records lost. If you do it the next day, you have to
>> >> do math to see what the one day delta is. So, to make reporting easy,
>> >> they want it to be reset whenever they do audictl -s.
>> >>
>> >> You could also make a AUDIT_GET_RESET that gets the status and resets the
>> >> number atomically. Then I can add another commandline option to auditctl
>> >> that allows an admin to say also reset the counters. If that command
>> >> line option is passed, I call AUDIT_GET_RESET otherwise I call
>> >> AUDIT_GET. Thought?>
>> > This would be slightly simpler in kernel implementation than the method
>> > I proposed and would work fine, off the top of my head.
>>
>> I'd prefer not to introduce another command message type for something
>> small like this.
>>
>> Steve, do you have any objection to the AUDIT_SET based approach?
>
> Either way, we'd need a feature flag so that I can tell if the kernel supports
> this or not.

I think we are okay without a specific feature flag as sending a
AUDIT_SET/reset on an old kernel will be harmless; it won't do
anything, but it shouldn't return an error either.

> Also, it should accept "0" as the only valid value.

Of course.

> We can do this and I can make auditctl do the two back to back before displaying the
> results to minimize the window of risk.
>
>> Based on what you've said above, it would seem like the potential race
>> condition with AUDIT_SET wouldn't be a significant issue.
>
> All a matter of perspective. What I think is a reasonable risk someone else
> may disagree. Does anyone else on the list object? If not I'd say go with it.

Judgement calls are always a matter of perspective, since you are the
only one who has asked for this (even if it is by proxy) I was asking
for your perspective.  Anyway, it looks like we're probably okay here;
if we don't hear anything in the next day or two lets go ahead with
the AUDIT_SET approach.  If it proves to be a problem we can always
introduce one of the other approaches later.

-- 
paul moore
www.paul-moore.com

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Richard Guy Briggs]

On 2016-12-07 18:45, Paul Moore wrote:
> On Wed, Dec 7, 2016 at 6:30 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > On Wednesday, December 7, 2016 6:10:49 PM EST Paul Moore wrote:
> >> On Wed, Dec 7, 2016 at 10:58 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> > On 2016-12-07 10:53, Steve Grubb wrote:
> >> >> On Wednesday, December 7, 2016 10:05:30 AM EST Paul Moore wrote:
> >> >> > On Tue, Dec 6, 2016 at 10:32 PM, Richard Guy Briggs <rgb@redhat.com>
> > wrote:
> >> >> > > On 2016-12-06 19:17, Paul Moore wrote:
> >> >> > >> On Tue, Dec 6, 2016 at 12:13 AM, Richard Guy Briggs <rgb@redhat.com>
> >> >> > >> Okay, back up ... this whole mess about atomic_xchg() was always
> >> >> > >> unrelated to my original suggestion, let's focus on my original
> >> >> > >> comment ... don't reset the counter on a AUDIT_GET, reset it on a
> >> >> > >> AUDIT_SET with an AUDIT_STATUS_LOST, does that make sense?
> >> >> > >
> >> >> > > I understood that.  It sounds like a nice simple and straightforward
> >> >> > > method to do it but for the question of accuracy.  Please rewind to
> >> >> > > my
> >> >> > > fundamental point: How do we get an accurate reading of the last
> >> >> > > value
> >> >> > > of audit_lost before resetting it?
> >> >> >
> >> >> > Okay, I thought you were worried about a different race, which is why
> >> >> > this discussion wasn't making much sense to me.  I understand your
> >> >> > point, but I really dislike the API; although that's not your fault,
> >> >> > it's really the only way to do it via AUDIT_GET.
> >> >> >
> >> >> > I'd much prefer we go with the cleaner AUDIT_SET approach and just not
> >> >> > worry about the small race window.  It would only be an issue if you
> >> >> > reset the count under heavy audit load, and why would you reset the
> >> >> > lost value if you were under a heavy audit load?  That just doesn't
> >> >> > make sense.
> >> >> >
> >> >> > I suppose we should hear from Steve on this since he was the one who
> >> >> > has been asking for this feature, although I'm pretty sure I know what
> >> >> > he is going to say.
> >> >>
> >> >> To start with, this request comes from users of the audit system. I just
> >> >> passed along the request. The issue is that when you do auditctl -s, you
> >> >> get the number of records lost. If you do it the next day, you have to
> >> >> do math to see what the one day delta is. So, to make reporting easy,
> >> >> they want it to be reset whenever they do audictl -s.
> >> >>
> >> >> You could also make a AUDIT_GET_RESET that gets the status and resets the
> >> >> number atomically. Then I can add another commandline option to auditctl
> >> >> that allows an admin to say also reset the counters. If that command
> >> >> line option is passed, I call AUDIT_GET_RESET otherwise I call
> >> >> AUDIT_GET. Thought?>
> >> > This would be slightly simpler in kernel implementation than the method
> >> > I proposed and would work fine, off the top of my head.
> >>
> >> I'd prefer not to introduce another command message type for something
> >> small like this.
> >>
> >> Steve, do you have any objection to the AUDIT_SET based approach?
> >
> > Either way, we'd need a feature flag so that I can tell if the kernel supports
> > this or not.
> 
> I think we are okay without a specific feature flag as sending a
> AUDIT_SET/reset on an old kernel will be harmless; it won't do
> anything, but it shouldn't return an error either.

Ok, so userspace is still left wondering if it worked until the next
time it reads that value, and even then it can't be certain if that
value is the same or higher than it was when userspace thought it reset
that value.  At this point, an old kernel will not return an error and
simply ignore any new AUDIT_STATUS_* flag since each flag is treated
independently and extra flags are ignored and not blocked.  This seems
sloppy since we have two ways of fixing this uncertainty pretty easily.

> > Also, it should accept "0" as the only valid value.
> 
> Of course.

No problem.

> > We can do this and I can make auditctl do the two back to back before displaying the
> > results to minimize the window of risk.

Having it depend on userspace implementation to minimize the risk of a 
race strikes me as unsound design.

> >> Based on what you've said above, it would seem like the potential race
> >> condition with AUDIT_SET wouldn't be a significant issue.
> >
> > All a matter of perspective. What I think is a reasonable risk someone else
> > may disagree. Does anyone else on the list object? If not I'd say go with it.
> 
> Judgement calls are always a matter of perspective, since you are the
> only one who has asked for this (even if it is by proxy) I was asking
> for your perspective.  Anyway, it looks like we're probably okay here;
> if we don't hear anything in the next day or two lets go ahead with
> the AUDIT_SET approach.  If it proves to be a problem we can always
> introduce one of the other approaches later.

If it isn't dependable and accurate, what's the point?  Why not do it
right the first time?

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Paul Moore]

On Wed, Dec 7, 2016 at 10:53 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2016-12-07 18:45, Paul Moore wrote:
>> On Wed, Dec 7, 2016 at 6:30 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>> > On Wednesday, December 7, 2016 6:10:49 PM EST Paul Moore wrote:
>> >> On Wed, Dec 7, 2016 at 10:58 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> >> > On 2016-12-07 10:53, Steve Grubb wrote:
>> >> >> On Wednesday, December 7, 2016 10:05:30 AM EST Paul Moore wrote:
>> >> >> > On Tue, Dec 6, 2016 at 10:32 PM, Richard Guy Briggs <rgb@redhat.com>
>> > wrote:
>> >> >> > > On 2016-12-06 19:17, Paul Moore wrote:
>> >> >> > >> On Tue, Dec 6, 2016 at 12:13 AM, Richard Guy Briggs <rgb@redhat.com>
>> >> >> > >> Okay, back up ... this whole mess about atomic_xchg() was always
>> >> >> > >> unrelated to my original suggestion, let's focus on my original
>> >> >> > >> comment ... don't reset the counter on a AUDIT_GET, reset it on a
>> >> >> > >> AUDIT_SET with an AUDIT_STATUS_LOST, does that make sense?
>> >> >> > >
>> >> >> > > I understood that.  It sounds like a nice simple and straightforward
>> >> >> > > method to do it but for the question of accuracy.  Please rewind to
>> >> >> > > my
>> >> >> > > fundamental point: How do we get an accurate reading of the last
>> >> >> > > value
>> >> >> > > of audit_lost before resetting it?
>> >> >> >
>> >> >> > Okay, I thought you were worried about a different race, which is why
>> >> >> > this discussion wasn't making much sense to me.  I understand your
>> >> >> > point, but I really dislike the API; although that's not your fault,
>> >> >> > it's really the only way to do it via AUDIT_GET.
>> >> >> >
>> >> >> > I'd much prefer we go with the cleaner AUDIT_SET approach and just not
>> >> >> > worry about the small race window.  It would only be an issue if you
>> >> >> > reset the count under heavy audit load, and why would you reset the
>> >> >> > lost value if you were under a heavy audit load?  That just doesn't
>> >> >> > make sense.
>> >> >> >
>> >> >> > I suppose we should hear from Steve on this since he was the one who
>> >> >> > has been asking for this feature, although I'm pretty sure I know what
>> >> >> > he is going to say.
>> >> >>
>> >> >> To start with, this request comes from users of the audit system. I just
>> >> >> passed along the request. The issue is that when you do auditctl -s, you
>> >> >> get the number of records lost. If you do it the next day, you have to
>> >> >> do math to see what the one day delta is. So, to make reporting easy,
>> >> >> they want it to be reset whenever they do audictl -s.
>> >> >>
>> >> >> You could also make a AUDIT_GET_RESET that gets the status and resets the
>> >> >> number atomically. Then I can add another commandline option to auditctl
>> >> >> that allows an admin to say also reset the counters. If that command
>> >> >> line option is passed, I call AUDIT_GET_RESET otherwise I call
>> >> >> AUDIT_GET. Thought?>
>> >> > This would be slightly simpler in kernel implementation than the method
>> >> > I proposed and would work fine, off the top of my head.
>> >>
>> >> I'd prefer not to introduce another command message type for something
>> >> small like this.
>> >>
>> >> Steve, do you have any objection to the AUDIT_SET based approach?
>> >
>> > Either way, we'd need a feature flag so that I can tell if the kernel supports
>> > this or not.
>>
>> I think we are okay without a specific feature flag as sending a
>> AUDIT_SET/reset on an old kernel will be harmless; it won't do
>> anything, but it shouldn't return an error either.
>
> Ok, so userspace is still left wondering if it worked until the next
> time it reads that value, and even then it can't be certain if that
> value is the same or higher than it was when userspace thought it reset
> that value.  At this point, an old kernel will not return an error and
> simply ignore any new AUDIT_STATUS_* flag since each flag is treated
> independently and extra flags are ignored and not blocked.  This seems
> sloppy since we have two ways of fixing this uncertainty pretty easily.

Comments like the above aren't helpful, they are just annoying.  The
drawbacks to the AUDIT_SET/reset approach have already been discussed
on this thread, if you want to do something constructive think about
how you can resolve these limitations within the context of others
comments/feedback.

I've already mentioned that I didn't like the AUDIT_GET/reset approach
because I thought the interface was bad.  As I'm sure you know, the
audit kernel/userspace interface is a bit of a hot-button topic with
me; I think it has a lot of problems and I'm very intent on not making
it worse (in my opinion, I will admit that API design is not entirely
objective).  Continuing to argue for a interface design that I've
already expressed a dislike for is not likely to win me over to your
side; regardless of the outcome you will end up frustrating both
yourself and the maintainer, neither are good things.

We all agree that there is a potential race window with respect to
reading/resetting the lost counter, where we disagree is the
likelihood of that happening in practice.  You feel very strongly that
the window is of grave concern, Steve and myself much less so.  If you
still feel strongly about this, think about some different ways in
which you can avoid losing a lost message counter bump.  Off the top
of my head, there are really only two ways for the kernel's audit
subsystem to send information back to userspace in this case, via a
netlink return/error message or an audit record.  We could possibly do
something with the netlink error message by returning the lost counter
as a positive integer (negative integer is a failure code, zero is
success), but that might get tricky in the future, although we could
mitigate that risk by forcing the AUDIT_SET/reset to happen by itself
(in other words, don't simply check to see if the bit is set in the
bitmask, e.g. (s.mask & AUDIT_STATUS_LOST), check to see it is equal,
e.g. (s.mask == AUDIT_STATUS_LOST)).  We could also mitigate the race
via an audit record by emitting a record indicating that the lost
counter was reset and record the lost counter (before the reset) in
that record; honestly, now that I'm writing this, it seems like
something we should be doing regardless, as tampering with the lost
counter seems like a security relevant event.

There you go, two possible solutions for eliminating/mitigating the
potential race while sticking with the simpler AUDIT_SET/reset
interface.  I suppose you could even implement both of the solutions
above, they aren't mutually exclusive; that would depend on what
Steve/userspace would prefer.  Finally, as for the feature bitmap to
signal to userspace that we support this new feature: if you can't
live without it, go ahead and add it in.  As I said before, I'm a
little concerned at the rate we are consuming this bitmap, but I'll
admit we still have plenty of room before we have to start worrying
about alternatives.

-- 
paul moore
www.paul-moore.com

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Richard Guy Briggs]

On 2016-12-08 09:05, Paul Moore wrote:
> On Wed, Dec 7, 2016 at 10:53 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2016-12-07 18:45, Paul Moore wrote:
> >> On Wed, Dec 7, 2016 at 6:30 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> >> > On Wednesday, December 7, 2016 6:10:49 PM EST Paul Moore wrote:
> >> >> On Wed, Dec 7, 2016 at 10:58 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> >> > On 2016-12-07 10:53, Steve Grubb wrote:
> >> >> >> On Wednesday, December 7, 2016 10:05:30 AM EST Paul Moore wrote:
> >> >> >> > On Tue, Dec 6, 2016 at 10:32 PM, Richard Guy Briggs <rgb@redhat.com>
> >> > wrote:
> >> >> >> > > On 2016-12-06 19:17, Paul Moore wrote:
> >> >> >> > >> On Tue, Dec 6, 2016 at 12:13 AM, Richard Guy Briggs <rgb@redhat.com>
> >> >> >> > >> Okay, back up ... this whole mess about atomic_xchg() was always
> >> >> >> > >> unrelated to my original suggestion, let's focus on my original
> >> >> >> > >> comment ... don't reset the counter on a AUDIT_GET, reset it on a
> >> >> >> > >> AUDIT_SET with an AUDIT_STATUS_LOST, does that make sense?
> >> >> >> > >
> >> >> >> > > I understood that.  It sounds like a nice simple and straightforward
> >> >> >> > > method to do it but for the question of accuracy.  Please rewind to
> >> >> >> > > my
> >> >> >> > > fundamental point: How do we get an accurate reading of the last
> >> >> >> > > value
> >> >> >> > > of audit_lost before resetting it?
> >> >> >> >
> >> >> >> > Okay, I thought you were worried about a different race, which is why
> >> >> >> > this discussion wasn't making much sense to me.  I understand your
> >> >> >> > point, but I really dislike the API; although that's not your fault,
> >> >> >> > it's really the only way to do it via AUDIT_GET.
> >> >> >> >
> >> >> >> > I'd much prefer we go with the cleaner AUDIT_SET approach and just not
> >> >> >> > worry about the small race window.  It would only be an issue if you
> >> >> >> > reset the count under heavy audit load, and why would you reset the
> >> >> >> > lost value if you were under a heavy audit load?  That just doesn't
> >> >> >> > make sense.
> >> >> >> >
> >> >> >> > I suppose we should hear from Steve on this since he was the one who
> >> >> >> > has been asking for this feature, although I'm pretty sure I know what
> >> >> >> > he is going to say.
> >> >> >>
> >> >> >> To start with, this request comes from users of the audit system. I just
> >> >> >> passed along the request. The issue is that when you do auditctl -s, you
> >> >> >> get the number of records lost. If you do it the next day, you have to
> >> >> >> do math to see what the one day delta is. So, to make reporting easy,
> >> >> >> they want it to be reset whenever they do audictl -s.
> >> >> >>
> >> >> >> You could also make a AUDIT_GET_RESET that gets the status and resets the
> >> >> >> number atomically. Then I can add another commandline option to auditctl
> >> >> >> that allows an admin to say also reset the counters. If that command
> >> >> >> line option is passed, I call AUDIT_GET_RESET otherwise I call
> >> >> >> AUDIT_GET. Thought?>
> >> >> > This would be slightly simpler in kernel implementation than the method
> >> >> > I proposed and would work fine, off the top of my head.
> >> >>
> >> >> I'd prefer not to introduce another command message type for something
> >> >> small like this.
> >> >>
> >> >> Steve, do you have any objection to the AUDIT_SET based approach?
> >> >
> >> > Either way, we'd need a feature flag so that I can tell if the kernel supports
> >> > this or not.
> >>
> >> I think we are okay without a specific feature flag as sending a
> >> AUDIT_SET/reset on an old kernel will be harmless; it won't do
> >> anything, but it shouldn't return an error either.
> >
> > Ok, so userspace is still left wondering if it worked until the next
> > time it reads that value, and even then it can't be certain if that
> > value is the same or higher than it was when userspace thought it reset
> > that value.  At this point, an old kernel will not return an error and
> > simply ignore any new AUDIT_STATUS_* flag since each flag is treated
> > independently and extra flags are ignored and not blocked.  This seems
> > sloppy since we have two ways of fixing this uncertainty pretty easily.
> 
> Comments like the above aren't helpful, they are just annoying.  The
> drawbacks to the AUDIT_SET/reset approach have already been discussed
> on this thread, if you want to do something constructive think about
> how you can resolve these limitations within the context of others
> comments/feedback.

I'm sorry to offend.  That wasn't my intent.  I was trying to bring new
information and observations into the discussion.  Is there anything
factually wrong in my observations other than the opinion of it being
sloppy?

> I've already mentioned that I didn't like the AUDIT_GET/reset approach
> because I thought the interface was bad.  As I'm sure you know, the
> audit kernel/userspace interface is a bit of a hot-button topic with
> me; I think it has a lot of problems and I'm very intent on not making
> it worse (in my opinion, I will admit that API design is not entirely
> objective).  Continuing to argue for a interface design that I've
> already expressed a dislike for is not likely to win me over to your
> side; regardless of the outcome you will end up frustrating both
> yourself and the maintainer, neither are good things.

I've re-read the thread from the beginning.  I guess I must have missed
what was the fundamental problem with the AUDIT_GET/reset method other
than taste.  What don't you like about the API that precludes
using/abusing it this way?  Is it the issue that a GET would
surprisingly change a value rather than just reading it?  I didn't like
it either, but it was the next obvious way to tackle the issue without
losing information.

> We all agree that there is a potential race window with respect to
> reading/resetting the lost counter, where we disagree is the
> likelihood of that happening in practice.

We've had other races that looked pretty unlikely in practice that were
deemed to be unacceptable.  Granted the risks were higher if they were
exploited, but they were mitigated to the best of our ability.

None of this should matter now since there are ideas below that should
work.

> You feel very strongly that
> the window is of grave concern, Steve and myself much less so.  If you
> still feel strongly about this, think about some different ways in
> which you can avoid losing a lost message counter bump.  Off the top
> of my head, there are really only two ways for the kernel's audit
> subsystem to send information back to userspace in this case, via a
> netlink return/error message or an audit record.  We could possibly do
> something with the netlink error message by returning the lost counter
> as a positive integer (negative integer is a failure code, zero is
> success), but that might get tricky in the future, although we could
> mitigate that risk by forcing the AUDIT_SET/reset to happen by itself
> (in other words, don't simply check to see if the bit is set in the
> bitmask, e.g. (s.mask & AUDIT_STATUS_LOST), check to see it is equal,
> e.g. (s.mask == AUDIT_STATUS_LOST)).

This could work.  What risk do you see in doing it with other flags?
That another set failure could usurp the return code?  If so, yes, I
agree with requiring it to be a lone flag.

> We could also mitigate the race
> via an audit record by emitting a record indicating that the lost
> counter was reset and record the lost counter (before the reset) in
> that record; honestly, now that I'm writing this, it seems like
> something we should be doing regardless, as tampering with the lost
> counter seems like a security relevant event.

Agreed.  This should be added regardless of reset method.

> There you go, two possible solutions for eliminating/mitigating the
> potential race while sticking with the simpler AUDIT_SET/reset
> interface.  I suppose you could even implement both of the solutions
> above, they aren't mutually exclusive; that would depend on what
> Steve/userspace would prefer.  Finally, as for the feature bitmap to
> signal to userspace that we support this new feature: if you can't
> live without it, go ahead and add it in.  As I said before, I'm a
> little concerned at the rate we are consuming this bitmap, but I'll
> admit we still have plenty of room before we have to start worrying
> about alternatives.

I would suggest that the return value (presuming it was reset when
non-zero) or the audit record generated reporting the lost value
reset would be sufficient confirmation that the feature exists on the
running kernel and the addition to the feature bitmap is not strictly
necessary, but you only find this out upon attempting that lost reset.

Well, we haven't used much of that bitmap space and if it isn't to be
used when needed, why is it there?  If there is a relatively simple
alternate non-destructive way to discover the presence of a feature use
of the bitmap isn't necessary.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Paul Moore]

On Fri, Dec 9, 2016 at 2:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2016-12-08 09:05, Paul Moore wrote:
>> On Wed, Dec 7, 2016 at 10:53 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > On 2016-12-07 18:45, Paul Moore wrote:
>> >> On Wed, Dec 7, 2016 at 6:30 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>> >> > On Wednesday, December 7, 2016 6:10:49 PM EST Paul Moore wrote:
>> >> >> On Wed, Dec 7, 2016 at 10:58 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> >> >> > On 2016-12-07 10:53, Steve Grubb wrote:
>> >> >> >> On Wednesday, December 7, 2016 10:05:30 AM EST Paul Moore wrote:
>> >> >> >> > On Tue, Dec 6, 2016 at 10:32 PM, Richard Guy Briggs <rgb@redhat.com>
>> >> > wrote:
>> >> >> >> > > On 2016-12-06 19:17, Paul Moore wrote:
>> >> >> >> > >> On Tue, Dec 6, 2016 at 12:13 AM, Richard Guy Briggs <rgb@redhat.com>
>> >> >> >> > >> Okay, back up ... this whole mess about atomic_xchg() was always
>> >> >> >> > >> unrelated to my original suggestion, let's focus on my original
>> >> >> >> > >> comment ... don't reset the counter on a AUDIT_GET, reset it on a
>> >> >> >> > >> AUDIT_SET with an AUDIT_STATUS_LOST, does that make sense?
>> >> >> >> > >
>> >> >> >> > > I understood that.  It sounds like a nice simple and straightforward
>> >> >> >> > > method to do it but for the question of accuracy.  Please rewind to
>> >> >> >> > > my
>> >> >> >> > > fundamental point: How do we get an accurate reading of the last
>> >> >> >> > > value
>> >> >> >> > > of audit_lost before resetting it?
>> >> >> >> >
>> >> >> >> > Okay, I thought you were worried about a different race, which is why
>> >> >> >> > this discussion wasn't making much sense to me.  I understand your
>> >> >> >> > point, but I really dislike the API; although that's not your fault,
>> >> >> >> > it's really the only way to do it via AUDIT_GET.
>> >> >> >> >
>> >> >> >> > I'd much prefer we go with the cleaner AUDIT_SET approach and just not
>> >> >> >> > worry about the small race window.  It would only be an issue if you
>> >> >> >> > reset the count under heavy audit load, and why would you reset the
>> >> >> >> > lost value if you were under a heavy audit load?  That just doesn't
>> >> >> >> > make sense.
>> >> >> >> >
>> >> >> >> > I suppose we should hear from Steve on this since he was the one who
>> >> >> >> > has been asking for this feature, although I'm pretty sure I know what
>> >> >> >> > he is going to say.
>> >> >> >>
>> >> >> >> To start with, this request comes from users of the audit system. I just
>> >> >> >> passed along the request. The issue is that when you do auditctl -s, you
>> >> >> >> get the number of records lost. If you do it the next day, you have to
>> >> >> >> do math to see what the one day delta is. So, to make reporting easy,
>> >> >> >> they want it to be reset whenever they do audictl -s.
>> >> >> >>
>> >> >> >> You could also make a AUDIT_GET_RESET that gets the status and resets the
>> >> >> >> number atomically. Then I can add another commandline option to auditctl
>> >> >> >> that allows an admin to say also reset the counters. If that command
>> >> >> >> line option is passed, I call AUDIT_GET_RESET otherwise I call
>> >> >> >> AUDIT_GET. Thought?>
>> >> >> > This would be slightly simpler in kernel implementation than the method
>> >> >> > I proposed and would work fine, off the top of my head.
>> >> >>
>> >> >> I'd prefer not to introduce another command message type for something
>> >> >> small like this.
>> >> >>
>> >> >> Steve, do you have any objection to the AUDIT_SET based approach?
>> >> >
>> >> > Either way, we'd need a feature flag so that I can tell if the kernel supports
>> >> > this or not.
>> >>
>> >> I think we are okay without a specific feature flag as sending a
>> >> AUDIT_SET/reset on an old kernel will be harmless; it won't do
>> >> anything, but it shouldn't return an error either.
>> >
>> > Ok, so userspace is still left wondering if it worked until the next
>> > time it reads that value, and even then it can't be certain if that
>> > value is the same or higher than it was when userspace thought it reset
>> > that value.  At this point, an old kernel will not return an error and
>> > simply ignore any new AUDIT_STATUS_* flag since each flag is treated
>> > independently and extra flags are ignored and not blocked.  This seems
>> > sloppy since we have two ways of fixing this uncertainty pretty easily.
>>
>> Comments like the above aren't helpful, they are just annoying.  The
>> drawbacks to the AUDIT_SET/reset approach have already been discussed
>> on this thread, if you want to do something constructive think about
>> how you can resolve these limitations within the context of others
>> comments/feedback.
>
> I'm sorry to offend.  That wasn't my intent.  I was trying to bring new
> information and observations into the discussion.  Is there anything
> factually wrong in my observations other than the opinion of it being
> sloppy?

Your email wasn't offensive, maybe a little snarky at times, but this
is an open mailing list and developers are nothing if not occasionally
snarky (at least I know I am); that wasn't my objection.

My objection to that first paragraph was that you weren't bringing
anything new to this thread (in my opinion), you were simply
regurgitating the same arguments without any thought of how to solve
these concerns within the context of our ongoing discussion.  I tried
to point you in a more constructive direction with my last email, but
perhaps I did a poor job, let me try again ... Opposing viewpoints,
especially when technical matters are concerned, is a good thing, but
ultimately a decision needs to be made if we are to solve our problems
and move forward; it is nice if that decision is the result of
unanimous vote, but it is foolish to expect that, or delay a fix,
waiting for consensus.  When you present a possible solution, and the
solution is rejected, in whole or in part, look at the other proposed
solutions: do they resolve all your concerns?  If yes, wonderful, work
to embrace that solution.  If not, try to resolve your concerns within
the context of this other proposal; don't continue to push something
which has already been rejected.  The ultimate goal should be to solve
the problem, not to merge a specific solution; the "best" solution for
the problem is always the one that gets merged.

Hopefully this helps, because I don't know how else to explain this,
and to be quite frank, I'm growing tired of talking about this.

>> I've already mentioned that I didn't like the AUDIT_GET/reset approach
>> because I thought the interface was bad.  As I'm sure you know, the
>> audit kernel/userspace interface is a bit of a hot-button topic with
>> me; I think it has a lot of problems and I'm very intent on not making
>> it worse (in my opinion, I will admit that API design is not entirely
>> objective).  Continuing to argue for a interface design that I've
>> already expressed a dislike for is not likely to win me over to your
>> side; regardless of the outcome you will end up frustrating both
>> yourself and the maintainer, neither are good things.
>
> I've re-read the thread from the beginning.  I guess I must have missed
> what was the fundamental problem with the AUDIT_GET/reset method other
> than taste.  What don't you like about the API that precludes
> using/abusing it this way?

A few things come to mind immediately: abusing GET this way
effectively makes it a "write" operation which can make access control
policy difficult, having to bracket the GET with a FEATURE operation
is ugly and potentially racy in its own way, complexity compared to
other solutions.

>> You feel very strongly that
>> the window is of grave concern, Steve and myself much less so.  If you
>> still feel strongly about this, think about some different ways in
>> which you can avoid losing a lost message counter bump.  Off the top
>> of my head, there are really only two ways for the kernel's audit
>> subsystem to send information back to userspace in this case, via a
>> netlink return/error message or an audit record.  We could possibly do
>> something with the netlink error message by returning the lost counter
>> as a positive integer (negative integer is a failure code, zero is
>> success), but that might get tricky in the future, although we could
>> mitigate that risk by forcing the AUDIT_SET/reset to happen by itself
>> (in other words, don't simply check to see if the bit is set in the
>> bitmask, e.g. (s.mask & AUDIT_STATUS_LOST), check to see it is equal,
>> e.g. (s.mask == AUDIT_STATUS_LOST)).
>
> This could work.  What risk do you see in doing it with other flags?
> That another set failure could usurp the return code?  If so, yes, I
> agree with requiring it to be a lone flag.

Possible conflicts with other SET operations failing as well as
potential future use.  We can always relax the restriction in the
future, we can't make it more restrictive.

>> There you go, two possible solutions for eliminating/mitigating the
>> potential race while sticking with the simpler AUDIT_SET/reset
>> interface.  I suppose you could even implement both of the solutions
>> above, they aren't mutually exclusive; that would depend on what
>> Steve/userspace would prefer.  Finally, as for the feature bitmap to
>> signal to userspace that we support this new feature: if you can't
>> live without it, go ahead and add it in.  As I said before, I'm a
>> little concerned at the rate we are consuming this bitmap, but I'll
>> admit we still have plenty of room before we have to start worrying
>> about alternatives.
>
> I would suggest that the return value (presuming it was reset when
> non-zero) or the audit record generated reporting the lost value
> reset would be sufficient confirmation that the feature exists on the
> running kernel and the addition to the feature bitmap is not strictly
> necessary, but you only find this out upon attempting that lost reset.
>
> Well, we haven't used much of that bitmap space and if it isn't to be
> used when needed, why is it there?  If there is a relatively simple
> alternate non-destructive way to discover the presence of a feature use
> of the bitmap isn't necessary.

My concern isn't the absolute consumption of the bitmap, but rather
the rate of the consumption.

-- 
paul moore
www.paul-moore.com

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Steve Grubb]

On Friday, December 9, 2016 6:46:43 PM EST Paul Moore wrote:
> > I would suggest that the return value (presuming it was reset when
> > non-zero) or the audit record generated reporting the lost value
> > reset would be sufficient confirmation that the feature exists on the
> > running kernel and the addition to the feature bitmap is not strictly
> > necessary, but you only find this out upon attempting that lost reset.
> > 
> > Well, we haven't used much of that bitmap space and if it isn't to be
> > used when needed, why is it there?  If there is a relatively simple
> > alternate non-destructive way to discover the presence of a feature use
> > of the bitmap isn't necessary.
> 
> My concern isn't the absolute consumption of the bitmap, but rather
> the rate of the consumption.

I'm not concerned much about it. There are very few more RFE's that are either 
in the pipeline or something I can think of that we need.

-Steve

Re: [RFC][PATCH] audit: add feature audit_lost reset

[Paul Moore]

On Sat, Dec 10, 2016 at 3:40 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Friday, December 9, 2016 6:46:43 PM EST Paul Moore wrote:
>> > I would suggest that the return value (presuming it was reset when
>> > non-zero) or the audit record generated reporting the lost value
>> > reset would be sufficient confirmation that the feature exists on the
>> > running kernel and the addition to the feature bitmap is not strictly
>> > necessary, but you only find this out upon attempting that lost reset.
>> >
>> > Well, we haven't used much of that bitmap space and if it isn't to be
>> > used when needed, why is it there?  If there is a relatively simple
>> > alternate non-destructive way to discover the presence of a feature use
>> > of the bitmap isn't necessary.
>>
>> My concern isn't the absolute consumption of the bitmap, but rather
>> the rate of the consumption.
>
> I'm not concerned much about it. There are very few more RFE's that are either
> in the pipeline or something I can think of that we need.

We always need to plan on more features, regardless of what you know
about today, something (many somethings actually) is almost certain to
come up in the future.

-- 
paul moore
www.paul-moore.com