* Re: [PATCH 0/22] fsnotify: Avoid SRCU stalls with fanotify permission events
From: Paul Moore @ 2016-12-22 20:58 UTC (permalink / raw)
To: Jan Kara
Cc: linux-fsdevel, Amir Goldstein, Lino Sanfilippo, Miklos Szeredi,
linux-audit
In-Reply-To: <20161222091538.28702-1-jack@suse.cz>
On Thu, Dec 22, 2016 at 4:15 AM, Jan Kara <jack@suse.cz> wrote:
> Hello,
>
> currently, fanotify waits for response to a permission even from userspace
> process while holding fsnotify_mark_srcu lock. That has a consequence that
> when userspace process takes long to respond or does not respond at all,
> fsnotify_mark_srcu period cannot ever complete blocking reclaim of any
> notification marks and also blocking any process that did synchronize_srcu()
> on fsnotify_mark_srcu. Effectively, this eventually blocks anybody interacting
> with the notification subsystem. Miklos has some real world reports of this
> happening. Although this in principle a problem of broken userspace
> application (which futhermore has to have CAP_SYS_ADMIN in init_user_ns, so
> it is not a security problem), it is still nasty that a simple error can
> block the kernel like this.
>
> This patch set solves this problem ...
>
> Patches have survived testing with inotify/fanotify tests in LTP. I didn't test
> audit - Paul can you give these patches some testing? Since some of the
> changes are really non-trivial, I'd welcome if someone reviewed the patch set.
I'm going to take a look at the audit related patches right now,
expect some feedback shortly.
In the meantime, if you wanted to play a bit with some simple audit
regression tests, check out the testsuite below:
* https://github.com/linux-audit/audit-testsuite
... it is still rather simplistic, but the tests in tests/file_* and
tests/exec_name should do some basic exercises of the audit code that
leverages fsnotify. If nothing else, it should give you some ideas
about how you might stress this a bit more with audit.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 1/2] selinux: log errors when loading new policy
From: Gary Tierney @ 2016-12-19 16:00 UTC (permalink / raw)
To: sds; +Cc: selinux, linux-audit
In-Reply-To: <1482161529.28570.25.camel@tycho.nsa.gov>
On Mon, Dec 19, 2016 at 10:32:09AM -0500, Stephen Smalley wrote:
> On Mon, 2016-12-19 at 15:19 +0000, Gary Tierney wrote:
> > On Mon, Dec 19, 2016 at 09:43:06AM -0500, Stephen Smalley wrote:
> > >
> > > On Sat, 2016-12-17 at 20:48 +0000, Gary Tierney wrote:
> > > >
> > > > Adds error and warning messages to the codepaths which can fail
> > > > when
> > > > loading a new policy. If a policy fails to load, an error
> > > > message
> > > > will
> > > > be printed to dmesg with a description of what
> > > > failed. Previously if
> > > > there was an error during policy loading there would be no
> > > > indication
> > > > that it failed.
> > > >
> > > > Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
> > > > ---
> > > > security/selinux/selinuxfs.c | 26 +++++++++++++++++++++-----
> > > > 1 file changed, 21 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/security/selinux/selinuxfs.c
> > > > b/security/selinux/selinuxfs.c
> > > > index 0aac402..2139cc7 100644
> > > > --- a/security/selinux/selinuxfs.c
> > > > +++ b/security/selinux/selinuxfs.c
> > > > @@ -522,20 +522,32 @@ static ssize_t sel_write_load(struct file
> > > > *file, const char __user *buf,
> > > > goto out;
> > > >
> > > > length = security_load_policy(data, count);
> > > > - if (length)
> > > > + if (length) {
> > > > + pr_err("SELinux: %s: failed to load policy\n",
> > > > + __func__);
> > >
> > > Not sure about your usage of pr_err() vs pr_warn();
> > > security_load_policy() may simply fail due to invalid policy from
> > > userspace, not a kernel-internal error per se.
> > >
> >
> > The intention was to make a distinction between failures on or after
> > security_load_policy(). If security_load_policy() fails then no
> > audit message
> > will be logged about loading a new policy, so it seemed more
> > appropriate to
> > treat that case as KERN_ERROR. Though with what you said in mind, it
> > is
> > probably better to change this to pr_warn() as security_load_policy()
> > is
> > unlikely to cause an actual kernel-internal error.
>
> Yes, I tend to view them in the reverse; a failure on
> security_load_policy() is just a typical userspace-induced (or OOM)
> failure, whereas failure on any of the later calls will leave the
> kernel in an inconsistent internal state, so if anything, those should
> be the pr_err() cases instead, while security_load_policy() failure
> might even need/want a pr_warn_ratelimited() since it can be induced by
> userspace (albeit only root with :security load_policy permission).
>
Noted.
> >
> > >
> > > I would tend to omit the function name; I don't think it is
> > > especially
> > > helpful.
> > >
> >
> > Agreed. It seems to be used as a convention throughout
> > security/selinux,
> > though am happy to drop it from the patch.
> >
> > I was planning to send a v2 with pr_err() swapped for pr_warn() and
> > __func__
> > dropped from the log message, though keeping in mind that Steve has
> > prepared a
> > patch for this (also, logging to the audit subsystem might be more
> > appropriate) would it be better to drop #1 and keep #2?
>
> Not sure - I'd have to see Steve's patch or at least hear more details
> from him to know whether his patch would obsolete yours or just
> complement it.
>
Right, I'll spin up a v2 with the recommended changes and CC in Steve for his
feedback.
> >
> > >
> > > There was an earlier discussion about augmenting the audit logging
> > > from
> > > this function, so this might overlap with that. I don't know where
> > > that stands.
> > >
> > > >
> > > > goto out;
> > > > + }
> > > >
> > > > length = sel_make_bools();
> > > > - if (length)
> > > > + if (length) {
> > > > + pr_warn("SELinux: %s: failed to load policy
> > > > booleans\n",
> > > > + __func__);
> > > > goto out1;
> > > > + }
> > > >
> > > > length = sel_make_classes();
> > > > - if (length)
> > > > + if (length) {
> > > > + pr_warn("SELinux: %s: failed to load policy
> > > > classes\n",
> > > > + __func__);
> > > > goto out1;
> > > > + }
> > > >
> > > > length = sel_make_policycap();
> > > > - if (length)
> > > > + if (length) {
> > > > + pr_warn("SELinux: %s: failed to load policy
> > > > capabilities\n",
> > > > + __func__);
> > > > goto out1;
> > > > + }
> > > >
> > > > length = count;
> > > >
> > > > @@ -1299,9 +1311,13 @@ static int sel_make_bools(void)
> > > >
> > > > isec = (struct inode_security_struct *)inode-
> > > > >
> > > > > i_security;
> > > > ret = security_genfs_sid("selinuxfs", page,
> > > > SECCLASS_FILE, &sid);
> > > > - if (ret)
> > > > + if (ret) {
> > > > + pr_warn_ratelimited("SELinux: %s: failed
> > > > to
> > > > lookup sid for %s\n",
> > > > + __func__, page);
> > > > goto out;
> > > >
> > > > + }
> > > > +
> > > > isec->sid = sid;
> > > > isec->initialized = LABEL_INITIALIZED;
> > > > inode->i_fop = &sel_bool_ops;
> >
--
Gary Tierney
GPG fingerprint: 412C 0EF9 C305 68E6 B660 BDAF 706E D765 85AA 79D8
https://sks-keyservers.net/pks/lookup?op=get&search=0x706ED76585AA79D8
^ permalink raw reply
* Re: [PATCH 1/2] selinux: log errors when loading new policy
From: Stephen Smalley @ 2016-12-19 15:32 UTC (permalink / raw)
To: Gary Tierney; +Cc: selinux, linux-audit
In-Reply-To: <20161219151946.GA5359@workstation>
On Mon, 2016-12-19 at 15:19 +0000, Gary Tierney wrote:
> On Mon, Dec 19, 2016 at 09:43:06AM -0500, Stephen Smalley wrote:
> >
> > On Sat, 2016-12-17 at 20:48 +0000, Gary Tierney wrote:
> > >
> > > Adds error and warning messages to the codepaths which can fail
> > > when
> > > loading a new policy. If a policy fails to load, an error
> > > message
> > > will
> > > be printed to dmesg with a description of what
> > > failed. Previously if
> > > there was an error during policy loading there would be no
> > > indication
> > > that it failed.
> > >
> > > Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
> > > ---
> > > security/selinux/selinuxfs.c | 26 +++++++++++++++++++++-----
> > > 1 file changed, 21 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/security/selinux/selinuxfs.c
> > > b/security/selinux/selinuxfs.c
> > > index 0aac402..2139cc7 100644
> > > --- a/security/selinux/selinuxfs.c
> > > +++ b/security/selinux/selinuxfs.c
> > > @@ -522,20 +522,32 @@ static ssize_t sel_write_load(struct file
> > > *file, const char __user *buf,
> > > goto out;
> > >
> > > length = security_load_policy(data, count);
> > > - if (length)
> > > + if (length) {
> > > + pr_err("SELinux: %s: failed to load policy\n",
> > > + __func__);
> >
> > Not sure about your usage of pr_err() vs pr_warn();
> > security_load_policy() may simply fail due to invalid policy from
> > userspace, not a kernel-internal error per se.
> >
>
> The intention was to make a distinction between failures on or after
> security_load_policy(). If security_load_policy() fails then no
> audit message
> will be logged about loading a new policy, so it seemed more
> appropriate to
> treat that case as KERN_ERROR. Though with what you said in mind, it
> is
> probably better to change this to pr_warn() as security_load_policy()
> is
> unlikely to cause an actual kernel-internal error.
Yes, I tend to view them in the reverse; a failure on
security_load_policy() is just a typical userspace-induced (or OOM)
failure, whereas failure on any of the later calls will leave the
kernel in an inconsistent internal state, so if anything, those should
be the pr_err() cases instead, while security_load_policy() failure
might even need/want a pr_warn_ratelimited() since it can be induced by
userspace (albeit only root with :security load_policy permission).
>
> >
> > I would tend to omit the function name; I don't think it is
> > especially
> > helpful.
> >
>
> Agreed. It seems to be used as a convention throughout
> security/selinux,
> though am happy to drop it from the patch.
>
> I was planning to send a v2 with pr_err() swapped for pr_warn() and
> __func__
> dropped from the log message, though keeping in mind that Steve has
> prepared a
> patch for this (also, logging to the audit subsystem might be more
> appropriate) would it be better to drop #1 and keep #2?
Not sure - I'd have to see Steve's patch or at least hear more details
from him to know whether his patch would obsolete yours or just
complement it.
>
> >
> > There was an earlier discussion about augmenting the audit logging
> > from
> > this function, so this might overlap with that. I don't know where
> > that stands.
> >
> > >
> > > goto out;
> > > + }
> > >
> > > length = sel_make_bools();
> > > - if (length)
> > > + if (length) {
> > > + pr_warn("SELinux: %s: failed to load policy
> > > booleans\n",
> > > + __func__);
> > > goto out1;
> > > + }
> > >
> > > length = sel_make_classes();
> > > - if (length)
> > > + if (length) {
> > > + pr_warn("SELinux: %s: failed to load policy
> > > classes\n",
> > > + __func__);
> > > goto out1;
> > > + }
> > >
> > > length = sel_make_policycap();
> > > - if (length)
> > > + if (length) {
> > > + pr_warn("SELinux: %s: failed to load policy
> > > capabilities\n",
> > > + __func__);
> > > goto out1;
> > > + }
> > >
> > > length = count;
> > >
> > > @@ -1299,9 +1311,13 @@ static int sel_make_bools(void)
> > >
> > > isec = (struct inode_security_struct *)inode-
> > > >
> > > > i_security;
> > > ret = security_genfs_sid("selinuxfs", page,
> > > SECCLASS_FILE, &sid);
> > > - if (ret)
> > > + if (ret) {
> > > + pr_warn_ratelimited("SELinux: %s: failed
> > > to
> > > lookup sid for %s\n",
> > > + __func__, page);
> > > goto out;
> > >
> > > + }
> > > +
> > > isec->sid = sid;
> > > isec->initialized = LABEL_INITIALIZED;
> > > inode->i_fop = &sel_bool_ops;
>
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply
* Re: [PATCH 1/2] selinux: log errors when loading new policy
From: Gary Tierney @ 2016-12-19 15:19 UTC (permalink / raw)
To: sds; +Cc: selinux, linux-audit
In-Reply-To: <1482158586.28570.17.camel@tycho.nsa.gov>
On Mon, Dec 19, 2016 at 09:43:06AM -0500, Stephen Smalley wrote:
> On Sat, 2016-12-17 at 20:48 +0000, Gary Tierney wrote:
> > Adds error and warning messages to the codepaths which can fail when
> > loading a new policy. If a policy fails to load, an error message
> > will
> > be printed to dmesg with a description of what failed. Previously if
> > there was an error during policy loading there would be no indication
> > that it failed.
> >
> > Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
> > ---
> > security/selinux/selinuxfs.c | 26 +++++++++++++++++++++-----
> > 1 file changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/security/selinux/selinuxfs.c
> > b/security/selinux/selinuxfs.c
> > index 0aac402..2139cc7 100644
> > --- a/security/selinux/selinuxfs.c
> > +++ b/security/selinux/selinuxfs.c
> > @@ -522,20 +522,32 @@ static ssize_t sel_write_load(struct file
> > *file, const char __user *buf,
> > goto out;
> >
> > length = security_load_policy(data, count);
> > - if (length)
> > + if (length) {
> > + pr_err("SELinux: %s: failed to load policy\n",
> > + __func__);
>
> Not sure about your usage of pr_err() vs pr_warn();
> security_load_policy() may simply fail due to invalid policy from
> userspace, not a kernel-internal error per se.
>
The intention was to make a distinction between failures on or after
security_load_policy(). If security_load_policy() fails then no audit message
will be logged about loading a new policy, so it seemed more appropriate to
treat that case as KERN_ERROR. Though with what you said in mind, it is
probably better to change this to pr_warn() as security_load_policy() is
unlikely to cause an actual kernel-internal error.
> I would tend to omit the function name; I don't think it is especially
> helpful.
>
Agreed. It seems to be used as a convention throughout security/selinux,
though am happy to drop it from the patch.
I was planning to send a v2 with pr_err() swapped for pr_warn() and __func__
dropped from the log message, though keeping in mind that Steve has prepared a
patch for this (also, logging to the audit subsystem might be more
appropriate) would it be better to drop #1 and keep #2?
> There was an earlier discussion about augmenting the audit logging from
> this function, so this might overlap with that. I don't know where
> that stands.
>
> > goto out;
> > + }
> >
> > length = sel_make_bools();
> > - if (length)
> > + if (length) {
> > + pr_warn("SELinux: %s: failed to load policy
> > booleans\n",
> > + __func__);
> > goto out1;
> > + }
> >
> > length = sel_make_classes();
> > - if (length)
> > + if (length) {
> > + pr_warn("SELinux: %s: failed to load policy
> > classes\n",
> > + __func__);
> > goto out1;
> > + }
> >
> > length = sel_make_policycap();
> > - if (length)
> > + if (length) {
> > + pr_warn("SELinux: %s: failed to load policy
> > capabilities\n",
> > + __func__);
> > goto out1;
> > + }
> >
> > length = count;
> >
> > @@ -1299,9 +1311,13 @@ static int sel_make_bools(void)
> >
> > isec = (struct inode_security_struct *)inode-
> > >i_security;
> > ret = security_genfs_sid("selinuxfs", page,
> > SECCLASS_FILE, &sid);
> > - if (ret)
> > + if (ret) {
> > + pr_warn_ratelimited("SELinux: %s: failed to
> > lookup sid for %s\n",
> > + __func__, page);
> > goto out;
> >
> > + }
> > +
> > isec->sid = sid;
> > isec->initialized = LABEL_INITIALIZED;
> > inode->i_fop = &sel_bool_ops;
--
Gary Tierney
GPG fingerprint: 412C 0EF9 C305 68E6 B660 BDAF 706E D765 85AA 79D8
https://sks-keyservers.net/pks/lookup?op=get&search=0x706ED76585AA79D8
^ permalink raw reply
* Re: [PATCH 1/2] selinux: log errors when loading new policy
From: Steve Grubb @ 2016-12-19 15:08 UTC (permalink / raw)
To: Stephen Smalley; +Cc: linux-audit, selinux
In-Reply-To: <1482158586.28570.17.camel@tycho.nsa.gov>
On Monday, December 19, 2016 9:43:06 AM EST Stephen Smalley wrote:
> On Sat, 2016-12-17 at 20:48 +0000, Gary Tierney wrote:
> > Adds error and warning messages to the codepaths which can fail when
> > loading a new policy. If a policy fails to load, an error message
> > will
> > be printed to dmesg with a description of what failed. Previously if
> > there was an error during policy loading there would be no indication
> > that it failed.
> >
> > Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
> > ---
> > security/selinux/selinuxfs.c | 26 +++++++++++++++++++++-----
> > 1 file changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/security/selinux/selinuxfs.c
> > b/security/selinux/selinuxfs.c
> > index 0aac402..2139cc7 100644
> > --- a/security/selinux/selinuxfs.c
> > +++ b/security/selinux/selinuxfs.c
> > @@ -522,20 +522,32 @@ static ssize_t sel_write_load(struct file
> > *file, const char __user *buf,
> > goto out;
> >
> > length = security_load_policy(data, count);
> > - if (length)
> > + if (length) {
> > + pr_err("SELinux: %s: failed to load policy\n",
> > + __func__);
>
> Not sure about your usage of pr_err() vs pr_warn();
> security_load_policy() may simply fail due to invalid policy from
> userspace, not a kernel-internal error per se.
>
> I would tend to omit the function name; I don't think it is especially
> helpful.
>
> There was an earlier discussion about augmenting the audit logging from
> this function, so this might overlap with that. I don't know where
> that stands.
I have a new patch that I'm going to send soon that addresses this. But I also
have a second patch that fixes the setboolean auditing as well, but it
deadlocks the system. I talked about it with Paul and I have an idea on how to
fix the deadlock but I haven't sent the updated patches yet. I plan to get to
them later this week.
-Steve
> > goto out;
> > + }
> >
> > length = sel_make_bools();
> > - if (length)
> > + if (length) {
> > + pr_warn("SELinux: %s: failed to load policy
> > booleans\n",
> > + __func__);
> > goto out1;
> > + }
> >
> > length = sel_make_classes();
> > - if (length)
> > + if (length) {
> > + pr_warn("SELinux: %s: failed to load policy
> > classes\n",
> > + __func__);
> > goto out1;
> > + }
> >
> > length = sel_make_policycap();
> > - if (length)
> > + if (length) {
> > + pr_warn("SELinux: %s: failed to load policy
> > capabilities\n",
> > + __func__);
> > goto out1;
> > + }
> >
> > length = count;
> >
> > @@ -1299,9 +1311,13 @@ static int sel_make_bools(void)
> >
> > isec = (struct inode_security_struct *)inode-
> >
> > >i_security;
> >
> > ret = security_genfs_sid("selinuxfs", page,
> > SECCLASS_FILE, &sid);
> > - if (ret)
> > + if (ret) {
> > + pr_warn_ratelimited("SELinux: %s: failed to
> > lookup sid for %s\n",
> > + __func__, page);
> > goto out;
> >
> > + }
> > +
> > isec->sid = sid;
> > isec->initialized = LABEL_INITIALIZED;
> > inode->i_fop = &sel_bool_ops;
^ permalink raw reply
* Re: [PATCH 1/2] selinux: log errors when loading new policy
From: Stephen Smalley @ 2016-12-19 14:43 UTC (permalink / raw)
To: Gary Tierney, selinux; +Cc: linux-audit
In-Reply-To: <1482007719-14313-2-git-send-email-gary.tierney@gmx.com>
On Sat, 2016-12-17 at 20:48 +0000, Gary Tierney wrote:
> Adds error and warning messages to the codepaths which can fail when
> loading a new policy. If a policy fails to load, an error message
> will
> be printed to dmesg with a description of what failed. Previously if
> there was an error during policy loading there would be no indication
> that it failed.
>
> Signed-off-by: Gary Tierney <gary.tierney@gmx.com>
> ---
> security/selinux/selinuxfs.c | 26 +++++++++++++++++++++-----
> 1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/security/selinux/selinuxfs.c
> b/security/selinux/selinuxfs.c
> index 0aac402..2139cc7 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -522,20 +522,32 @@ static ssize_t sel_write_load(struct file
> *file, const char __user *buf,
> goto out;
>
> length = security_load_policy(data, count);
> - if (length)
> + if (length) {
> + pr_err("SELinux: %s: failed to load policy\n",
> + __func__);
Not sure about your usage of pr_err() vs pr_warn();
security_load_policy() may simply fail due to invalid policy from
userspace, not a kernel-internal error per se.
I would tend to omit the function name; I don't think it is especially
helpful.
There was an earlier discussion about augmenting the audit logging from
this function, so this might overlap with that. I don't know where
that stands.
> goto out;
> + }
>
> length = sel_make_bools();
> - if (length)
> + if (length) {
> + pr_warn("SELinux: %s: failed to load policy
> booleans\n",
> + __func__);
> goto out1;
> + }
>
> length = sel_make_classes();
> - if (length)
> + if (length) {
> + pr_warn("SELinux: %s: failed to load policy
> classes\n",
> + __func__);
> goto out1;
> + }
>
> length = sel_make_policycap();
> - if (length)
> + if (length) {
> + pr_warn("SELinux: %s: failed to load policy
> capabilities\n",
> + __func__);
> goto out1;
> + }
>
> length = count;
>
> @@ -1299,9 +1311,13 @@ static int sel_make_bools(void)
>
> isec = (struct inode_security_struct *)inode-
> >i_security;
> ret = security_genfs_sid("selinuxfs", page,
> SECCLASS_FILE, &sid);
> - if (ret)
> + if (ret) {
> + pr_warn_ratelimited("SELinux: %s: failed to
> lookup sid for %s\n",
> + __func__, page);
> goto out;
>
> + }
> +
> isec->sid = sid;
> isec->initialized = LABEL_INITIALIZED;
> inode->i_fop = &sel_bool_ops;
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply
* Re: [PATCH] audit: add feature audit_lost reset
From: Paul Moore @ 2016-12-16 22:58 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit
In-Reply-To: <8c740656caee622c9a9f8642ac48f22e1bf6933c.1481869063.git.rgb@redhat.com>
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
* Re: [PATCH v2] audit: add feature audit_lost reset
From: Paul Moore @ 2016-12-16 22:47 UTC (permalink / raw)
To: Steve Grubb, Richard Guy Briggs; +Cc: linux-audit
In-Reply-To: <20161216033942.GI1707@madcap2.tricolour.ca>
On Thu, Dec 15, 2016 at 10:39 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2016-12-15 22:12, Steve Grubb wrote:
>> On Thursday, December 15, 2016 7:50:48 PM EST Paul Moore wrote:
>> > On Thu, Dec 15, 2016 at 7:22 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>> > > I'm planning to replace all the config change logging with the
>> > > audit_log_task_simple function I sent so that we have everything. Can we
>> > > go ahead and pull that in so that we can start using it?
>> >
>> > There needs to be more than one user of the function to make it
>> > worthwhile; so far that function has only been proposed with a single
>> > user. Propose it with multiple users and we can look at it seriously.
>>
>> That's because I have several unrelated patches that use it. Do you want me to
>> send all of them at once? There's going to be at least 5 users of the
>> function. Possibly more. I want it to be the default for all future events
>> added because it concisely gives the necessary information for well-formed
>> events.
>
> I'd send the audit_log_task_simple() patch alone, then send each feature
> that uses it in a separate patch set. Failing that, send it as a
> separate patch in the first patch set to make it available for all, then
> follow it with more separate patchsets for other events.
>
> There is a chicken and egg problem here.
The extremely safe way to do this would be to submit the unrelated
patches first, each written to *not* make use of your new
consolidation function, then submit a single patch which adds the
function and integrates it with the others.
This approach also has the advantage that you are able to submit fixes
as you run across then and not have to wait for everything. I know
you already have at least one patch ready to, you just need to remove
the references to audit_log_task_simple.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* [PATCH] audit: add feature audit_lost reset
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
* Re: [PATCH v2] audit: add feature audit_lost reset
From: Richard Guy Briggs @ 2016-12-16 3:59 UTC (permalink / raw)
To: Steve Grubb; +Cc: linux-audit
In-Reply-To: <1803050.hqkP3u55ii@x2>
On 2016-12-15 19:22, Steve Grubb wrote:
> On Thursday, December 15, 2016 3:39:16 PM EST Paul Moore wrote:
> > On Sat, Dec 10, 2016 at 6:52 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 sent to the listening audit daemon.
> > > The data field will contain a u32 with the positive value of the
> > > audit_lost value when it was reset.
> > >
> > > See: https://github.com/linux-audit/audit-kernel/issues/3
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > >
> > > include/uapi/linux/audit.h | 2 ++
> > > kernel/audit.c | 8 +++++++-
> > > 2 files changed, 9 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..19cfee0 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,12 @@ static int audit_receive_msg(struct sk_buff *skb,
> > > struct nlmsghdr *nlh)>
> > > if (err < 0)
> > >
> > > return err;
> > >
> > > }
> > >
> > > + if (s.mask == AUDIT_STATUS_LOST) {
> > > + u32 lost = atomic_xchg(&audit_lost, 0);
> > > +
> > > + audit_send_reply(skb, seq, AUDIT_LOST_RESET, 0, 0,
> > > &lost, sizeof(lost));
> > I'm not sure it makes much sense to both return the lost value as a
> > netlink return code as well as send a separate netlink message back to
> > the controlling task with the same information. What I meant earlier
> > was that we would emit an audit record, similar to
> > audit_log_config_change(), so that the audit log would not only have
> > information that the status count was reset, but also the subject
> > information necessary to associate the action with an individual.
> >
> > Does that make sense?
>
> I'm planning to replace all the config change logging with the
> audit_log_task_simple function I sent so that we have everything. Can we go
> ahead and pull that in so that we can start using it?
I was too late for this kernel release even with the first version of
this patch, so now we have plenty of time to get this right and have it
sit in next for a while.
> Thanks,
> -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
^ permalink raw reply
* Re: [PATCH v2] audit: add feature audit_lost reset
From: Richard Guy Briggs @ 2016-12-16 3:54 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit
In-Reply-To: <CAHC9VhQwVKTmFnYUmOAHSv5RfPs_BYaHcpzxcMyMfWm-kKuHXA@mail.gmail.com>
On 2016-12-15 15:39, Paul Moore wrote:
> On Sat, Dec 10, 2016 at 6:52 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 sent to the listening audit daemon.
> > The data field will contain a u32 with the positive value of the
> > audit_lost value when it was reset.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/3
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > include/uapi/linux/audit.h | 2 ++
> > kernel/audit.c | 8 +++++++-
> > 2 files changed, 9 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..19cfee0 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,12 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > if (err < 0)
> > return err;
> > }
> > + if (s.mask == AUDIT_STATUS_LOST) {
> > + u32 lost = atomic_xchg(&audit_lost, 0);
> > +
> > + audit_send_reply(skb, seq, AUDIT_LOST_RESET, 0, 0, &lost, sizeof(lost));
>
> I'm not sure it makes much sense to both return the lost value as a
> netlink return code as well as send a separate netlink message back to
> the controlling task with the same information. What I meant earlier
> was that we would emit an audit record, similar to
> audit_log_config_change(), so that the audit log would not only have
> information that the status count was reset, but also the subject
> information necessary to associate the action with an individual.
I would argue that both are useful, but I missed your point about having
the record sent to the regular queue rather than sent to the process
that is doing the reset. The process doing the reset will see that
message, but auditd won't, so it won't end up in the log itself, which
was the whole point of sending the notice of an action that requires
trackng.
I'll respin. I still see value in returing a +ve value to the caller to
detect that feature.
> Does that make sense?
It does. (The discussion of audit_log_task_simple was a hijack of this
thread that really belongs in a new thread or at least a new subject
name).
> > + return lost;
> > + }
> > break;
> > }
> > case AUDIT_GET_FEATURE:
>
> 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
^ permalink raw reply
* Re: [PATCH v2] audit: add feature audit_lost reset
From: Richard Guy Briggs @ 2016-12-16 3:39 UTC (permalink / raw)
To: Steve Grubb; +Cc: linux-audit
In-Reply-To: <14416625.8shW6piCVY@x2>
On 2016-12-15 22:12, Steve Grubb wrote:
> On Thursday, December 15, 2016 7:50:48 PM EST Paul Moore wrote:
> > On Thu, Dec 15, 2016 at 7:22 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > > I'm planning to replace all the config change logging with the
> > > audit_log_task_simple function I sent so that we have everything. Can we
> > > go ahead and pull that in so that we can start using it?
> >
> > There needs to be more than one user of the function to make it
> > worthwhile; so far that function has only been proposed with a single
> > user. Propose it with multiple users and we can look at it seriously.
>
> That's because I have several unrelated patches that use it. Do you want me to
> send all of them at once? There's going to be at least 5 users of the
> function. Possibly more. I want it to be the default for all future events
> added because it concisely gives the necessary information for well-formed
> events.
I'd send the audit_log_task_simple() patch alone, then send each feature
that uses it in a separate patch set. Failing that, send it as a
separate patch in the first patch set to make it available for all, then
follow it with more separate patchsets for other events.
There is a chicken and egg problem here.
> -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
^ permalink raw reply
* audit 2.7 released
From: Steve Grubb @ 2016-12-16 3:22 UTC (permalink / raw)
To: Linux Audit
Hello,
I've just released a new version of the audit daemon. It can be downloaded
from http://people.redhat.com/sgrubb/audit. It will also be in rawhide
soon. The ChangeLog is:
- Remove config file permission checks in auparse
- Audisp-remote should detect normal socket close and mark remote_ended
- Allow auditctl to list rules if no capabilities but root euid
- In libaudit, use the last word of the syscall bit mask
- In auditd, write_logs option was not correctly handled (#1382397)
- In libaudit, allow filtering on new exclude filter fields (Richard Guy Briggs)
- In auditd, fix looping when checking active connections
- In auparse, the auparse_state_t pointer to keep escape_mode information
- In libaudit, add support for rules using sessionid (Richard Guy Briggs)
- Remove entry filter support
- Add auparse_destroy_ext function
- Improve ENRICHED logging format performance in auditd
- Fix regex rule file matching in augenrules (#1396792)
- Add numeric field/record accessors to auparse
- Fix auditd freeing in middle of reply buffer when nolog is used
- Switch auparse uid/gid cache to lru to limit growth
- Prevent ausearch from clobbering type field on loginuid search
- Add audit_get_session function to libaudit
- Add session and uid to most audit events
- Add auparse_classify code interface for subj, obj, action, results
The main goal of this update is to land the auparse_classify interface to
auparse. This will unlock many new capabilities in subsequent releases of the
2.7 series. If you are a programmer and do stuff with R or machine learning,
let me know. This is aimed squarely at transforming data into knowledge.
Aside from that, this fixes remote logging, and logging with the nolog and
write_logs = no option, it allows audit rules on the new exclude filter fields
and rules that use sessionid.
The entry filter support has been dropped. It was deprecated a couple years
ago. There are performance enhancements and correctness fixes.
Please let me know if you run across any problems with this release.
-Steve
^ permalink raw reply
* Re: [PATCH v2] audit: add feature audit_lost reset
From: Steve Grubb @ 2016-12-16 3:12 UTC (permalink / raw)
To: Paul Moore; +Cc: Richard Guy Briggs, linux-audit
In-Reply-To: <CAHC9VhS4yCw6bfYJVLOzy1VOZnF+FZyA8wS3O6UjVT4SZ45KUA@mail.gmail.com>
On Thursday, December 15, 2016 7:50:48 PM EST Paul Moore wrote:
> On Thu, Dec 15, 2016 at 7:22 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > I'm planning to replace all the config change logging with the
> > audit_log_task_simple function I sent so that we have everything. Can we
> > go ahead and pull that in so that we can start using it?
>
> There needs to be more than one user of the function to make it
> worthwhile; so far that function has only been proposed with a single
> user. Propose it with multiple users and we can look at it seriously.
That's because I have several unrelated patches that use it. Do you want me to
send all of them at once? There's going to be at least 5 users of the
function. Possibly more. I want it to be the default for all future events
added because it concisely gives the necessary information for well-formed
events.
-Steve
^ permalink raw reply
* Re: [PATCH v2] audit: add feature audit_lost reset
From: Paul Moore @ 2016-12-16 0:50 UTC (permalink / raw)
To: Steve Grubb; +Cc: Richard Guy Briggs, linux-audit
In-Reply-To: <1803050.hqkP3u55ii@x2>
On Thu, Dec 15, 2016 at 7:22 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> I'm planning to replace all the config change logging with the
> audit_log_task_simple function I sent so that we have everything. Can we go
> ahead and pull that in so that we can start using it?
There needs to be more than one user of the function to make it
worthwhile; so far that function has only been proposed with a single
user. Propose it with multiple users and we can look at it seriously.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v2] audit: add feature audit_lost reset
From: Steve Grubb @ 2016-12-16 0:22 UTC (permalink / raw)
To: linux-audit; +Cc: Richard Guy Briggs
In-Reply-To: <CAHC9VhQwVKTmFnYUmOAHSv5RfPs_BYaHcpzxcMyMfWm-kKuHXA@mail.gmail.com>
On Thursday, December 15, 2016 3:39:16 PM EST Paul Moore wrote:
> On Sat, Dec 10, 2016 at 6:52 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 sent to the listening audit daemon.
> > The data field will contain a u32 with the positive value of the
> > audit_lost value when it was reset.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/3
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >
> > include/uapi/linux/audit.h | 2 ++
> > kernel/audit.c | 8 +++++++-
> > 2 files changed, 9 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..19cfee0 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,12 @@ static int audit_receive_msg(struct sk_buff *skb,
> > struct nlmsghdr *nlh)>
> > if (err < 0)
> >
> > return err;
> >
> > }
> >
> > + if (s.mask == AUDIT_STATUS_LOST) {
> > + u32 lost = atomic_xchg(&audit_lost, 0);
> > +
> > + audit_send_reply(skb, seq, AUDIT_LOST_RESET, 0, 0,
> > &lost, sizeof(lost));
> I'm not sure it makes much sense to both return the lost value as a
> netlink return code as well as send a separate netlink message back to
> the controlling task with the same information. What I meant earlier
> was that we would emit an audit record, similar to
> audit_log_config_change(), so that the audit log would not only have
> information that the status count was reset, but also the subject
> information necessary to associate the action with an individual.
>
> Does that make sense?
I'm planning to replace all the config change logging with the
audit_log_task_simple function I sent so that we have everything. Can we go
ahead and pull that in so that we can start using it?
Thanks,
-Steve
> > + 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
* Re: [PATCH v2] audit: add feature audit_lost reset
From: Paul Moore @ 2016-12-15 20:39 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit
In-Reply-To: <75c9c4dcd0a57ba6afec676ec55155e70ccb6a28.1481370732.git.rgb@redhat.com>
On Sat, Dec 10, 2016 at 6:52 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 sent to the listening audit daemon.
> The data field will contain a u32 with the positive value of the
> audit_lost value when it was reset.
>
> See: https://github.com/linux-audit/audit-kernel/issues/3
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/uapi/linux/audit.h | 2 ++
> kernel/audit.c | 8 +++++++-
> 2 files changed, 9 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..19cfee0 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,12 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> if (err < 0)
> return err;
> }
> + if (s.mask == AUDIT_STATUS_LOST) {
> + u32 lost = atomic_xchg(&audit_lost, 0);
> +
> + audit_send_reply(skb, seq, AUDIT_LOST_RESET, 0, 0, &lost, sizeof(lost));
I'm not sure it makes much sense to both return the lost value as a
netlink return code as well as send a separate netlink message back to
the controlling task with the same information. What I meant earlier
was that we would emit an audit record, similar to
audit_log_config_change(), so that the audit log would not only have
information that the status count was reset, but also the subject
information necessary to associate the action with an individual.
Does that make sense?
> + 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
* Re: [GIT PULL] Audit patches for v4.10
From: Paul Moore @ 2016-12-14 21:14 UTC (permalink / raw)
To: linux-audit
In-Reply-To: <1522672.4tJvQfugPF@sifl>
On Wed, Dec 14, 2016 at 1:27 PM, Paul Moore <pmoore@redhat.com> wrote:
> Hi Linus,
>
> After the small number of patches for v4.9, we've got a much bigger pile for
> v4.10 ...
[NOTE: dropped everyone except for the linux-audit list]
Due to the large number of changes in audit#next for v4.10 I've
decided to not do the usual audit#next rebase following the
pull-request to Linus; the next branch will remain based on v4.8 for
the v4.11 development cycle. I've also merged the two patches in my
next-queue branch into audit#next.
As a reminder, the normal process is documented here:
* http://www.paul-moore.com/blog/d/2016/03/kernel-repo-process.html
--
paul moore
www.paul-moore.com
^ permalink raw reply
* [GIT PULL] Audit patches for v4.10
From: Paul Moore @ 2016-12-14 18:27 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-audit, linux-kernel
Hi Linus,
After the small number of patches for v4.9, we've got a much bigger pile for
v4.10.
The bulk of these patches involve a rework of the audit backlog queue to
enable us to move the netlink multicasting out of the task/thread that
generates the audit record and into the kernel thread that emits the record
(just like we do for the audit unicast to auditd). While we were playing
with the backlog queue(s) we fixed a number of other little problems with
the code, and from all the testing so far things look to be in much better
shape now. Doing this also allowed us to re-enable disabling IRQs for some
netns operations ("netns: avoid disabling irq for netns id"). The remaining
patches fix some small problems that are well documented in the commit
descriptions, as well as adding session ID filtering support.
You will likely hit two merge conflicts, one in net/core/net_namespace.c and
one in include/uapi/linux/audit.h, both are easily resolved so I won't
bother you with that here. If you have questions, you know how to find me.
Thanks,
-Paul
---
The following changes since commit c8d2bc9bc39ebea8437fd974fdbc21847bb897a3:
Linux 4.8 (2016-10-02 16:24:33 -0700)
are available in the git repository at:
git://git.infradead.org/users/pcmoore/audit stable-4.10
for you to fetch changes up to 533c7b69c764ad5febb3e716899f43a75564fcab:
audit: use proper refcount locking on audit_sock
(2016-12-14 13:06:04 -0500)
----------------------------------------------------------------
Alexey Dobriyan (1):
audit: less stack usage for /proc/*/loginuid
Paul Moore (9):
audit: fixup audit_init()
audit: queue netlink multicast sends just like we do for unicast sends
audit: rename the queues and kauditd related functions
audit: rework the audit queue handling
audit: rework audit_log_start()
audit: wake up kauditd_thread after auditd registers
audit: handle a clean auditd shutdown with grace
audit: don't ever sleep on a command record/message
netns: avoid disabling irq for netns id
Richard Guy Briggs (5):
audit: tame initialization warning len_abuf in audit_log_execve_info
audit: skip sessionid sentinel value when auto-incrementing
audit: add support for session ID user filter
audit: move kaudit thread start from auditd registration to
kaudit init (#2)
audit: use proper refcount locking on audit_sock
Steve Grubb (1):
audit: fix formatting of AUDIT_CONFIG_CHANGE events
fs/proc/base.c | 2 +-
include/uapi/linux/audit.h | 5 +-
kernel/audit.c | 532 ++++++++++++++++++++++++---------------
kernel/audit_fsnotify.c | 5 +-
kernel/audit_tree.c | 3 +-
kernel/audit_watch.c | 5 +-
kernel/auditfilter.c | 5 +-
kernel/auditsc.c | 12 +-
net/core/net_namespace.c | 35 ++-
9 files changed, 361 insertions(+), 243 deletions(-)
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [RFC PATCH v3] audit: use proper refcount locking on audit_sock
From: Cong Wang @ 2016-12-14 5:36 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Linux Kernel Network Developers, LKML, Eric Dumazet, linux-audit,
Dmitry Vyukov
In-Reply-To: <20161214040005.GL22660@madcap2.tricolour.ca>
On Tue, Dec 13, 2016 at 8:00 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2016-12-13 16:19, Cong Wang wrote:
>> On Tue, Dec 13, 2016 at 7:03 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > @@ -1283,8 +1299,10 @@ static void __net_exit audit_net_exit(struct net *net)
>> > {
>> > struct audit_net *aunet = net_generic(net, audit_net_id);
>> > struct sock *sock = aunet->nlsk;
>> > + mutex_lock(&audit_cmd_mutex);
>> > if (sock == audit_sock)
>> > auditd_reset();
>> > + mutex_unlock(&audit_cmd_mutex);
>>
>> This still doesn't look correct to me, b/c here we release the audit_sock
>> refcnt twice:
>>
>> 1) inside audit_reset()
>
> The audit_reset() refcount decrement corresponds to a setting of
> audit_sock only if audit_sock is still non-NULL.
>
Hmm, thinking about it again, looks like the sock == audit_sock
and audit_sock != NULL checks can guarantee we are safe. So,
Reviewed-by: Cong Wang <xiyou.wangcong@gmail.com>
^ permalink raw reply
* Re: netlink: GPF in sock_sndtimeo
From: Richard Guy Briggs @ 2016-12-14 4:17 UTC (permalink / raw)
To: Cong Wang
Cc: Herbert Xu, Johannes Berg, netdev, Florian Westphal, LKML,
Eric Dumazet, linux-audit, syzkaller, David Miller, Dmitry Vyukov
In-Reply-To: <CAM_iQpVPEJ2t29ENpT4qcBznwE83w_PEBOxStwyzDH27Si2Ppw@mail.gmail.com>
On 2016-12-13 16:17, Cong Wang wrote:
> On Tue, Dec 13, 2016 at 2:52 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > It is actually the audit_pid and audit_nlk_portid that I care about
> > more. The audit daemon could vanish or close the socket while the
> > kernel sock to which it was attached is still quite valid. Accessing
> > the set of three atomically is the urge. I wonder if it makes more
> > sense to test for the presence of auditd using audit_sock rather than
> > audit_pid, but still keep audit_pid for our reporting and replacement
> > strategy. Another idea would be to put the three in one struct.
>
> Note, the process has audit_pid should hold a refcnt to the netns too,
> so the netns can't be gone until that process is gone.
I noted that. I did wonder if there might be a problem if all the
processes were moved to another netns with the struct sock stuck in the
now process-void netns.
This is alluded-to in 6f285b19d09f ("audit: Send replies in the proper
network namespace.").
> > Can someone explain how they think the original test was able to trigger
> > this GPF? Network namespace shutdown while something pretended to set
> > up a new auditd? That's impressive for a fuzzer if that's the case...
> > Is there an strace? I guess it is all in test().
>
> I am surprised you still don't get the race condition even when you
> are now working on v2...
>
> The race happens in this scenarios :
>
> 1) Create a new netns
>
> 2) In the new netns, communicate with kauditd to set audit_sock
>
> 3) Generate some audit messages, so kauditd will keep sending them
> via audit_sock
>
> 4) exit the netns
>
> 5) the previous audit_sock is now going away, but kaudit_sock could still
> access it in this small window.
Ah ok that fits...
- 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
* Re: [RFC PATCH v3] audit: use proper refcount locking on audit_sock
From: Richard Guy Briggs @ 2016-12-14 4:00 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, LKML, Eric Dumazet, linux-audit,
Dmitry Vyukov
In-Reply-To: <CAM_iQpVFkNdEirvBDi8wV=iExt9BnCm3KU7+Q8oqhrJJtcnu9Q@mail.gmail.com>
On 2016-12-13 16:19, Cong Wang wrote:
> On Tue, Dec 13, 2016 at 7:03 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > @@ -1283,8 +1299,10 @@ static void __net_exit audit_net_exit(struct net *net)
> > {
> > struct audit_net *aunet = net_generic(net, audit_net_id);
> > struct sock *sock = aunet->nlsk;
> > + mutex_lock(&audit_cmd_mutex);
> > if (sock == audit_sock)
> > auditd_reset();
> > + mutex_unlock(&audit_cmd_mutex);
>
> This still doesn't look correct to me, b/c here we release the audit_sock
> refcnt twice:
>
> 1) inside audit_reset()
The audit_reset() refcount decrement corresponds to a setting of
audit_sock only if audit_sock is still non-NULL.
> 2) netlink_kernel_release()
This refcount decrement corresponds to netlink_kernel_create().
- 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
* Re: [RFC PATCH v3] audit: use proper refcount locking on audit_sock
From: Cong Wang @ 2016-12-14 0:19 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Linux Kernel Network Developers, LKML, linux-audit, Dmitry Vyukov,
Eric Dumazet, Eric Paris, Paul Moore, sgrubb
In-Reply-To: <61c37ca790bc11bc023aea8f9b70ab3098aa30f5.1481626466.git.rgb@redhat.com>
On Tue, Dec 13, 2016 at 7:03 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> @@ -1283,8 +1299,10 @@ static void __net_exit audit_net_exit(struct net *net)
> {
> struct audit_net *aunet = net_generic(net, audit_net_id);
> struct sock *sock = aunet->nlsk;
> + mutex_lock(&audit_cmd_mutex);
> if (sock == audit_sock)
> auditd_reset();
> + mutex_unlock(&audit_cmd_mutex);
This still doesn't look correct to me, b/c here we release the audit_sock
refcnt twice:
1) inside audit_reset()
2) netlink_kernel_release()
^ permalink raw reply
* Re: netlink: GPF in sock_sndtimeo
From: Cong Wang @ 2016-12-14 0:17 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Herbert Xu, Johannes Berg, netdev, Florian Westphal, LKML,
Eric Dumazet, linux-audit, syzkaller, David Miller, Dmitry Vyukov
In-Reply-To: <20161213105233.GG1305@madcap2.tricolour.ca>
On Tue, Dec 13, 2016 at 2:52 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> It is actually the audit_pid and audit_nlk_portid that I care about
> more. The audit daemon could vanish or close the socket while the
> kernel sock to which it was attached is still quite valid. Accessing
> the set of three atomically is the urge. I wonder if it makes more
> sense to test for the presence of auditd using audit_sock rather than
> audit_pid, but still keep audit_pid for our reporting and replacement
> strategy. Another idea would be to put the three in one struct.
Note, the process has audit_pid should hold a refcnt to the netns too,
so the netns can't be gone until that process is gone.
>
> Can someone explain how they think the original test was able to trigger
> this GPF? Network namespace shutdown while something pretended to set
> up a new auditd? That's impressive for a fuzzer if that's the case...
> Is there an strace? I guess it is all in test().
>
I am surprised you still don't get the race condition even when you
are now working on v2...
The race happens in this scenarios :
1) Create a new netns
2) In the new netns, communicate with kauditd to set audit_sock
3) Generate some audit messages, so kauditd will keep sending them
via audit_sock
4) exit the netns
5) the previous audit_sock is now going away, but kaudit_sock could still
access it in this small window.
^ permalink raw reply
* Re: [RFC PATCH v3] audit: use proper refcount locking on audit_sock
From: Paul Moore @ 2016-12-13 20:50 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: netdev, linux-kernel, edumazet, linux-audit, xiyou.wangcong,
dvyukov
In-Reply-To: <61c37ca790bc11bc023aea8f9b70ab3098aa30f5.1481626466.git.rgb@redhat.com>
On Tue, Dec 13, 2016 at 10:03 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Resetting audit_sock appears to be racy.
>
> audit_sock was being copied and dereferenced without using a refcount on
> the source sock.
>
> Bump the refcount on the underlying sock when we store a refrence in
> audit_sock and release it when we reset audit_sock. audit_sock
> modification needs the audit_cmd_mutex.
>
> See: https://lkml.org/lkml/2016/11/26/232
>
> Thanks to Eric Dumazet <edumazet@google.com> and Cong Wang
> <xiyou.wangcong@gmail.com> on ideas how to fix it.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> There has been a lot of change in the audit code that is about to go
> upstream to address audit queue issues. This patch is based on the
> source tree: git://git.infradead.org/users/pcmoore/audit#next
> ---
> kernel/audit.c | 28 +++++++++++++++++++++++-----
> 1 files changed, 23 insertions(+), 5 deletions(-)
This looks more reasonable. I still wonder about synchronization
between threads changing the audit_* connection variables and the
kauditd_thread, but I guess we can treat that as another issue; this
patch fixes a bug and is worth merging now.
I'm building a test kernel right now, assuming nothing blows up I'll
push this patch with the rest of the audit patches tomorrow; if
something bad happens, this is going to miss the first audit pull
request.
> diff --git a/kernel/audit.c b/kernel/audit.c
> index f20eee0..3bb4126 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -446,14 +446,19 @@ static void kauditd_retry_skb(struct sk_buff *skb)
> * Description:
> * Break the auditd/kauditd connection and move all the records in the retry
> * queue into the hold queue in case auditd reconnects.
> + * The audit_cmd_mutex must be held when calling this function.
> */
Don't resend, but in the future please start comments like this on the
previous line.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox