* [PATCH] audit: add feature audit_lost reset
@ 2016-12-16 6:59 Richard Guy Briggs
2016-12-16 22:58 ` Paul Moore
0 siblings, 1 reply; 8+ messages in thread
From: Richard Guy Briggs @ 2016-12-16 6:59 UTC (permalink / raw)
To: linux-audit; +Cc: Richard Guy Briggs
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@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 */
#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;
+ 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");
+ audit_log_end(ab);
+ return lost;
+ }
break;
}
case AUDIT_GET_FEATURE:
--
1.7.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] audit: add feature audit_lost reset
2016-12-16 6:59 [PATCH] audit: add feature audit_lost reset Richard Guy Briggs
@ 2016-12-16 22:58 ` Paul Moore
2017-01-11 18:56 ` Steve Grubb
0 siblings, 1 reply; 8+ messages in thread
From: Paul Moore @ 2016-12-16 22:58 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit
On Fri, Dec 16, 2016 at 1:59 AM, Richard Guy Briggs <rgb@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@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 */
>
> #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.
> + audit_log_end(ab);
> + return lost;
> + }
> break;
> }
> case AUDIT_GET_FEATURE:
> --
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] audit: add feature audit_lost reset
2016-12-16 22:58 ` Paul Moore
@ 2017-01-11 18:56 ` Steve Grubb
2017-01-11 23:35 ` Steve Grubb
2017-01-12 4:12 ` Richard Guy Briggs
0 siblings, 2 replies; 8+ messages in thread
From: Steve Grubb @ 2017-01-11 18:56 UTC (permalink / raw)
To: linux-audit; +Cc: Richard Guy Briggs
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@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@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@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] audit: add feature audit_lost reset
2017-01-11 18:56 ` Steve Grubb
@ 2017-01-11 23:35 ` Steve Grubb
2017-01-12 4:19 ` Richard Guy Briggs
2017-01-12 4:12 ` Richard Guy Briggs
1 sibling, 1 reply; 8+ messages in thread
From: Steve Grubb @ 2017-01-11 23:35 UTC (permalink / raw)
To: linux-audit; +Cc: Richard Guy Briggs
On Wednesday, January 11, 2017 1:56:46 PM EST Steve Grubb wrote:
> On Friday, December 16, 2016 5:58:39 PM EST Paul Moore wrote:
> > On Fri, Dec 16, 2016 at 1:59 AM, Richard Guy Briggs <rgb@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@redhat.com>
> > > ---
> > > v3: Switch, from returing a message to the initiating process, to
> > > queueing the message for the audit log.
> > >
> > > v2: Switch from AUDIT_GET to AUDIT_SET, adding a +ve return code and
> > > sending a dedicated AUDIT_LOST_RESET message.
> > > ---
> > >
> > > include/uapi/linux/audit.h | 2 ++
> > > kernel/audit.c | 16 +++++++++++++++-
> > > 2 files changed, 17 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > index 208df7b..6d38bff 100644
> > > --- a/include/uapi/linux/audit.h
> > > +++ b/include/uapi/linux/audit.h
> > > @@ -70,6 +70,7 @@
> > >
> > > #define AUDIT_TTY_SET 1017 /* Set TTY auditing status */
> > > #define AUDIT_SET_FEATURE 1018 /* Turn an audit feature on or
> > > off
> > > */ #define AUDIT_GET_FEATURE 1019 /* Get which features are
> > > enabled */>
> > >
> > > +#define AUDIT_LOST_RESET 1020 /* Reset the audit_lost value */
>
> The 1000 block is for command and control, not logging. If this is used in
> logging, it should be in the 1300 block. But see below, this probably is not
> needed.
>
> > > #define AUDIT_FIRST_USER_MSG 1100 /* Userspace messages mostly
> > > uninteresting to kernel */ #define AUDIT_USER_AVC 1107 /* We
> > > filter this differently */>
> > >
> > > @@ -325,6 +326,7 @@ enum {
> > >
> > > #define AUDIT_STATUS_RATE_LIMIT 0x0008
> > > #define AUDIT_STATUS_BACKLOG_LIMIT 0x0010
> > > #define AUDIT_STATUS_BACKLOG_WAIT_TIME 0x0020
> > >
> > > +#define AUDIT_STATUS_LOST 0x0040
> > >
> > > #define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT 0x00000001
> > > #define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002
> > >
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index f1ca116..441e8c0 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -122,7 +122,7 @@
> > >
> > > 3) suppressed due to audit_rate_limit
> > > 4) suppressed due to audit_backlog_limit
> > >
> > > */
> > >
> > > -static atomic_t audit_lost = ATOMIC_INIT(0);
> > > +static atomic_t audit_lost = ATOMIC_INIT(0);
> > >
> > > /* The netlink socket. */
> > > static struct sock *audit_sock;
> > >
> > > @@ -920,6 +920,20 @@ static int audit_receive_msg(struct sk_buff *skb,
> > > struct nlmsghdr *nlh)>
> > >
> > > if (err < 0)
> > >
> > > return err;
> > >
> > > }
> > >
> > > + if (s.mask == AUDIT_STATUS_LOST) {
> > > + struct audit_buffer *ab;
> > > + u32 lost = atomic_xchg(&audit_lost, 0);
> > > +
> > > + ab = audit_log_start(NULL, GFP_KERNEL,
> > > AUDIT_LOST_RESET); + if (unlikely(!ab))
> > > + return lost;
> >
> > I'm generally not a fan of adding the likely/unlikely optimizations in
> > non-critial/control path code like this one, but don't respin the
> > patch just for this. However, if you do have to respin the patch
> > please fix this.
> >
> > > + audit_log_format(ab, "lost=0 old=%u", lost);
> > > + audit_log_session_info(ab);
> > > + audit_log_task_context(ab);
> > > + audit_log_format(ab, " res=1");
> >
> > We're still need to userspace code, so no rush on this, but we should
> > get Steve's opinion on the format; he'll surely have something to say.
>
> So, I'm looking at this and wondering a few things. The config option right
> above this is audit_set_backlog_wait_time. Wouldn't you want to pattern this
> after it? IOW, make an audit_reset_lost function which calls
> audit_do_config_change() which calls audit_log_config_change()? This would
> make the event type CONFIG_CHANGE instead of LOST_RESET. Since no one is
> counting on this event at the moment, no one has software that would try to
> interpret LOST_RESET events so we can change it.
>
> I'm thinking its probably more important to be consistent than creating a
> new event type. I admit, I didn't follow this whole thread in detail and
> maybe there was a good reason to separate out LOST_RESET. By using
> audit_do_config_change() you also become consistent with other rules like
> if config changes are disallowed because we are immutable.
>
> These changes are on the logging side. This won't affect integration with
> auditctl. If you do want to keep LOST_RESET, then it affects all searching
> and reporting utilities.
OK. the code to support this is in svn. However, since we didn't use a feature
bit like we normally do, there is absolutely no way to report that the
underlying kernel does not support this. It quietly fails and pretends
everything is fine. I'd prefer that we had a feature bit to output a proper
error message.
-Steve
> > > + audit_log_end(ab);
> > > + return lost;
> > > + }
> > >
> > > break;
> > >
> > > }
> > >
> > > case AUDIT_GET_FEATURE:
> > > --
> > > 1.7.1
> > >
> > > --
> > > Linux-audit mailing list
> > > Linux-audit@redhat.com
> > > https://www.redhat.com/mailman/listinfo/linux-audit
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] audit: add feature audit_lost reset
2017-01-11 18:56 ` Steve Grubb
2017-01-11 23:35 ` Steve Grubb
@ 2017-01-12 4:12 ` Richard Guy Briggs
2017-01-12 14:56 ` Steve Grubb
1 sibling, 1 reply; 8+ messages in thread
From: Richard Guy Briggs @ 2017-01-12 4:12 UTC (permalink / raw)
To: Steve Grubb; +Cc: linux-audit
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@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@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@redhat.com
> > > https://www.redhat.com/mailman/listinfo/linux-audit
>
>
- 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] audit: add feature audit_lost reset
2017-01-11 23:35 ` Steve Grubb
@ 2017-01-12 4:19 ` Richard Guy Briggs
2017-01-12 14:58 ` Steve Grubb
0 siblings, 1 reply; 8+ messages in thread
From: Richard Guy Briggs @ 2017-01-12 4:19 UTC (permalink / raw)
To: Steve Grubb; +Cc: linux-audit
On 2017-01-11 18:35, Steve Grubb wrote:
> On Wednesday, January 11, 2017 1:56:46 PM EST Steve Grubb wrote:
> > On Friday, December 16, 2016 5:58:39 PM EST Paul Moore wrote:
> > > On Fri, Dec 16, 2016 at 1:59 AM, Richard Guy Briggs <rgb@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@redhat.com>
> > > > ---
> > > > v3: Switch, from returing a message to the initiating process, to
> > > > queueing the message for the audit log.
> > > >
> > > > v2: Switch from AUDIT_GET to AUDIT_SET, adding a +ve return code and
> > > > sending a dedicated AUDIT_LOST_RESET message.
> > > > ---
> > > >
> > > > include/uapi/linux/audit.h | 2 ++
> > > > kernel/audit.c | 16 +++++++++++++++-
> > > > 2 files changed, 17 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > > index 208df7b..6d38bff 100644
> > > > --- a/include/uapi/linux/audit.h
> > > > +++ b/include/uapi/linux/audit.h
> > > > @@ -70,6 +70,7 @@
> > > >
> > > > #define AUDIT_TTY_SET 1017 /* Set TTY auditing status */
> > > > #define AUDIT_SET_FEATURE 1018 /* Turn an audit feature on or
> > > > off
> > > > */ #define AUDIT_GET_FEATURE 1019 /* Get which features are
> > > > enabled */>
> > > >
> > > > +#define AUDIT_LOST_RESET 1020 /* Reset the audit_lost value */
> >
> > The 1000 block is for command and control, not logging. If this is used in
> > logging, it should be in the 1300 block. But see below, this probably is not
> > needed.
> >
> > > > #define AUDIT_FIRST_USER_MSG 1100 /* Userspace messages mostly
> > > > uninteresting to kernel */ #define AUDIT_USER_AVC 1107 /* We
> > > > filter this differently */>
> > > >
> > > > @@ -325,6 +326,7 @@ enum {
> > > >
> > > > #define AUDIT_STATUS_RATE_LIMIT 0x0008
> > > > #define AUDIT_STATUS_BACKLOG_LIMIT 0x0010
> > > > #define AUDIT_STATUS_BACKLOG_WAIT_TIME 0x0020
> > > >
> > > > +#define AUDIT_STATUS_LOST 0x0040
> > > >
> > > > #define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT 0x00000001
> > > > #define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002
> > > >
> > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > index f1ca116..441e8c0 100644
> > > > --- a/kernel/audit.c
> > > > +++ b/kernel/audit.c
> > > > @@ -122,7 +122,7 @@
> > > >
> > > > 3) suppressed due to audit_rate_limit
> > > > 4) suppressed due to audit_backlog_limit
> > > >
> > > > */
> > > >
> > > > -static atomic_t audit_lost = ATOMIC_INIT(0);
> > > > +static atomic_t audit_lost = ATOMIC_INIT(0);
> > > >
> > > > /* The netlink socket. */
> > > > static struct sock *audit_sock;
> > > >
> > > > @@ -920,6 +920,20 @@ static int audit_receive_msg(struct sk_buff *skb,
> > > > struct nlmsghdr *nlh)>
> > > >
> > > > if (err < 0)
> > > >
> > > > return err;
> > > >
> > > > }
> > > >
> > > > + if (s.mask == AUDIT_STATUS_LOST) {
> > > > + struct audit_buffer *ab;
> > > > + u32 lost = atomic_xchg(&audit_lost, 0);
> > > > +
> > > > + ab = audit_log_start(NULL, GFP_KERNEL,
> > > > AUDIT_LOST_RESET); + if (unlikely(!ab))
> > > > + return lost;
> > >
> > > I'm generally not a fan of adding the likely/unlikely optimizations in
> > > non-critial/control path code like this one, but don't respin the
> > > patch just for this. However, if you do have to respin the patch
> > > please fix this.
> > >
> > > > + audit_log_format(ab, "lost=0 old=%u", lost);
> > > > + audit_log_session_info(ab);
> > > > + audit_log_task_context(ab);
> > > > + audit_log_format(ab, " res=1");
> > >
> > > We're still need to userspace code, so no rush on this, but we should
> > > get Steve's opinion on the format; he'll surely have something to say.
> >
> > So, I'm looking at this and wondering a few things. The config option right
> > above this is audit_set_backlog_wait_time. Wouldn't you want to pattern this
> > after it? IOW, make an audit_reset_lost function which calls
> > audit_do_config_change() which calls audit_log_config_change()? This would
> > make the event type CONFIG_CHANGE instead of LOST_RESET. Since no one is
> > counting on this event at the moment, no one has software that would try to
> > interpret LOST_RESET events so we can change it.
> >
> > I'm thinking its probably more important to be consistent than creating a
> > new event type. I admit, I didn't follow this whole thread in detail and
> > maybe there was a good reason to separate out LOST_RESET. By using
> > audit_do_config_change() you also become consistent with other rules like
> > if config changes are disallowed because we are immutable.
> >
> > These changes are on the logging side. This won't affect integration with
> > auditctl. If you do want to keep LOST_RESET, then it affects all searching
> > and reporting utilities.
>
> OK. the code to support this is in svn. However, since we didn't use a feature
> bit like we normally do, there is absolutely no way to report that the
> underlying kernel does not support this. It quietly fails and pretends
> everything is fine. I'd prefer that we had a feature bit to output a proper
> error message.
Do you still want to switch to CONFIG_CHANGE? (I think that is a good
idea.)
I agree detecting this feature is a destructive operation requiring an
existing lost count and checking the positive return code, but not
impossible, and would prefer a feature bit.
As for audit being immutable, I could see an argument to have this
feature usable even though the config is locked. What's your take?
> -Steve
>
> > > > + audit_log_end(ab);
> > > > + return lost;
> > > > + }
> > > >
> > > > break;
> > > >
> > > > }
> > > >
> > > > case AUDIT_GET_FEATURE:
> > > > --
> > > > 1.7.1
> > > >
> > > > --
> > > > Linux-audit mailing list
> > > > Linux-audit@redhat.com
> > > > https://www.redhat.com/mailman/listinfo/linux-audit
> >
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
>
>
- 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] audit: add feature audit_lost reset
2017-01-12 4:12 ` Richard Guy Briggs
@ 2017-01-12 14:56 ` Steve Grubb
0 siblings, 0 replies; 8+ messages in thread
From: Steve Grubb @ 2017-01-12 14:56 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit
On Wednesday, January 11, 2017 11:12:47 PM EST Richard Guy Briggs wrote:
> On 2017-01-11 13:56, Steve Grubb wrote:
> 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?
There's 2 parts to this. Resolving the set command and resetting the count and
logging that this was done. What I'm saying is that the AUDIT_STATUS_LOST gets
me into that block of code so auditctl is all set - except for not being able
to tell if it should even try because the underlying kernel doesn't support
this.
> Do you not want to be able to trigger this from auditctl?
I can. Svn code already does this. The only issue is reporting failure and
logging what happened.
> 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.
Just like it is is just fine. Auditctl does not send multiple commands because
there's no way to express that from the rules or command line.
> 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.
Just call audit_log_config_change() from the AUDIT_STATUS_LOST section.
-Steve
> > > > + audit_log_end(ab);
> > > > + return lost;
> > > > + }
> > > >
> > > > break;
> > > >
> > > > }
> > > >
> > > > case AUDIT_GET_FEATURE:
> > > > --
> > > > 1.7.1
> > > >
> > > > --
> > > > Linux-audit mailing list
> > > > Linux-audit@redhat.com
> > > > https://www.redhat.com/mailman/listinfo/linux-audit
>
> - 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] audit: add feature audit_lost reset
2017-01-12 4:19 ` Richard Guy Briggs
@ 2017-01-12 14:58 ` Steve Grubb
0 siblings, 0 replies; 8+ messages in thread
From: Steve Grubb @ 2017-01-12 14:58 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit
On Wednesday, January 11, 2017 11:19:42 PM EST Richard Guy Briggs wrote:
> > OK. the code to support this is in svn. However, since we didn't use a
> > feature bit like we normally do, there is absolutely no way to report
> > that the underlying kernel does not support this. It quietly fails and
> > pretends everything is fine. I'd prefer that we had a feature bit to
> > output a proper error message.
>
> Do you still want to switch to CONFIG_CHANGE? (I think that is a good
> idea.)
Sure.
> I agree detecting this feature is a destructive operation requiring an
> existing lost count and checking the positive return code, but not
> impossible, and would prefer a feature bit.
I'd prefer a feature bit so that I can tell people your kernel doesn't support
this. Audit runs on a large variety of kernels.
> As for audit being immutable, I could see an argument to have this
> feature usable even though the config is locked. What's your take?
I can see value in resetting the count even when immutable. Perhaps just use
its logging function. So we don't have a new record type.
-Steve
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-01-12 14:58 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-16 6:59 [PATCH] audit: add feature audit_lost reset Richard Guy Briggs
2016-12-16 22:58 ` Paul Moore
2017-01-11 18:56 ` Steve Grubb
2017-01-11 23:35 ` Steve Grubb
2017-01-12 4:19 ` Richard Guy Briggs
2017-01-12 14:58 ` Steve Grubb
2017-01-12 4:12 ` Richard Guy Briggs
2017-01-12 14:56 ` Steve Grubb
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).