[PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

[PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

[Jiri Kosina]

From: Jiri Kosina <jkosina@suse.cz>

There is no point going through all the audit slow path syscall entry/exit 
in case the audit daemon is running, but hasn't populated the audit filter 
with any rules whatsoever.

Only set TIF_AUDIT_SYSCALL in case the number of populated audit rules is 
non-zero.

Originally-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---

This is basically resurrection / rebase of patch Andi Lutomirski sent some
time back in 2014 or so.
 
Andi, is there any reason this hasn't been pursued further? I think we 
still want to get some of the slow path performance back.

Thanks.

 include/linux/audit.h | 15 +++++++++++++--
 kernel/auditfilter.c  |  4 ++--
 kernel/auditsc.c      | 45 ++++++++++++++++++++++++++++++++++++++++-----
 kernel/fork.c         |  2 +-
 4 files changed, 56 insertions(+), 10 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index af410d9fbf2d..3d5e96f96be5 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -451,8 +451,10 @@ static inline void audit_fanotify(unsigned int response)
 		__audit_fanotify(response);
 }
 
-extern int audit_n_rules;
 extern int audit_signals;
+extern void audit_populate(struct task_struct *tsk);
+extern void audit_inc_n_rules(void);
+extern void audit_dec_n_rules(void);
 #else /* CONFIG_AUDITSYSCALL */
 static inline int audit_alloc(struct task_struct *task)
 {
@@ -572,7 +574,16 @@ static inline void audit_fanotify(unsigned int response)
 
 static inline void audit_ptrace(struct task_struct *t)
 { }
-#define audit_n_rules 0
+
+static inline void audit_populate(struct task_struct *tsk)
+{ }
+
+static inline void audit_inc_n_rules(void)
+{ }
+
+static inline void audit_dec_n_rules(void)
+{ }
+
 #define audit_signals 0
 #endif /* CONFIG_AUDITSYSCALL */
 
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 4a1758adb222..46ad138d8ba2 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -991,7 +991,7 @@ static inline int audit_add_rule(struct audit_entry *entry)
 	}
 #ifdef CONFIG_AUDITSYSCALL
 	if (!dont_count)
-		audit_n_rules++;
+		audit_inc_n_rules();
 
 	if (!audit_match_signal(entry))
 		audit_signals++;
@@ -1038,7 +1038,7 @@ int audit_del_rule(struct audit_entry *entry)
 
 #ifdef CONFIG_AUDITSYSCALL
 	if (!dont_count)
-		audit_n_rules--;
+		audit_dec_n_rules();
 
 	if (!audit_match_signal(entry))
 		audit_signals--;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index e80459f7e132..642dd856e716 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -91,7 +91,7 @@
 #define MAX_PROCTITLE_AUDIT_LEN 128
 
 /* number of audit rules */
-int audit_n_rules;
+static int audit_n_rules;
 
 /* determines whether we collect data for signals sent */
 int audit_signals;
@@ -919,6 +919,36 @@ static inline struct audit_context *audit_alloc_context(enum audit_state state)
 	return context;
 }
 
+void audit_inc_n_rules()
+{
+	struct task_struct *p, *g;
+	unsigned long flags;
+
+	read_lock_irqsave(&tasklist_lock, flags);
+	if (!audit_n_rules++) {
+		do_each_thread(g, p) {
+			if (p->audit_context)
+				set_tsk_thread_flag(p, TIF_SYSCALL_AUDIT);
+		} while_each_thread(g, p);
+	}
+	read_unlock_irqrestore(&tasklist_lock, flags);
+}
+
+void audit_dec_n_rules()
+{
+	struct task_struct *p, *g;
+	unsigned long flags;
+
+	read_lock_irqsave(&tasklist_lock, flags);
+	audit_n_rules--;
+	if (!audit_n_rules) {
+		do_each_thread(g, p) {
+			clear_tsk_thread_flag(p, TIF_SYSCALL_AUDIT);
+		} while_each_thread(g, p);
+	}
+	read_unlock_irqrestore(&tasklist_lock, flags);
+}
+
 /**
  * audit_alloc - allocate an audit context block for a task
  * @tsk: task
@@ -938,10 +968,8 @@ int audit_alloc(struct task_struct *tsk)
 		return 0; /* Return if not auditing. */
 
 	state = audit_filter_task(tsk, &key);
-	if (state == AUDIT_DISABLED) {
-		clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT);
+	if (state == AUDIT_DISABLED)
 		return 0;
-	}
 
 	if (!(context = audit_alloc_context(state))) {
 		kfree(key);
@@ -951,7 +979,6 @@ int audit_alloc(struct task_struct *tsk)
 	context->filterkey = key;
 
 	tsk->audit_context  = context;
-	set_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT);
 	return 0;
 }
 
@@ -967,6 +994,14 @@ static inline void audit_free_context(struct audit_context *context)
 	kfree(context);
 }
 
+void audit_populate(struct task_struct *tsk)
+{
+	if (tsk->audit_context && audit_n_rules)
+		set_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT);
+	else
+		clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT);
+}
+
 static int audit_log_pid_context(struct audit_context *context, pid_t pid,
 				 kuid_t auid, kuid_t uid, unsigned int sessionid,
 				 u32 sid, char *comm)
diff --git a/kernel/fork.c b/kernel/fork.c
index e5d9d405ae4e..79c828746d24 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1955,7 +1955,7 @@ static __latent_entropy struct task_struct *copy_process(
 		attach_pid(p, PIDTYPE_PID);
 		nr_threads++;
 	}
-
+	audit_populate(p);
 	total_forks++;
 	spin_unlock(&current->sighand->siglock);
 	syscall_tracepoint_update(p);
-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

[Andy Lutomirski]

On Wed, Mar 7, 2018 at 10:32 AM, Jiri Kosina <jikos@kernel.org> wrote:
> From: Jiri Kosina <jkosina@suse.cz>
>
> There is no point going through all the audit slow path syscall entry/exit
> in case the audit daemon is running, but hasn't populated the audit filter
> with any rules whatsoever.
>
> Only set TIF_AUDIT_SYSCALL in case the number of populated audit rules is
> non-zero.
>
> Originally-by: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> ---
>
> This is basically resurrection / rebase of patch Andi Lutomirski sent some
> time back in 2014 or so.
>
> Andi, is there any reason this hasn't been pursued further? I think we
> still want to get some of the slow path performance back.
>

Wow, this was a long time ago.  From memory and a bit of email diving,
there are two reasons.

1. The probably was partially solved (by Oleg, IIRC) by making
auditctl -a task,never cause newly spawned tasks to not suck.  Yes,
it's a very partial solution.  After considerable nagging, I got Fedora
to default to -a task,never.

2. This patch, as is, may be a bit problematic.  In particular, if one
task changes the audit rules while another task is in the middle of
the syscall, then it's too late to audit that syscall correctly.  This
could be seen as a bug or it could be seen as being just fine.

--Andy

Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

[Jiri Kosina]

On Wed, 7 Mar 2018, Andy Lutomirski wrote:

> Wow, this was a long time ago.  

Oh yeah; but it now resurfaced on our side, as we are of course receiving 
a lot of requests with respect to making syscall performance great again 
:)

> From memory and a bit of email diving, there are two reasons.
> 
> 1. The probably was partially solved (by Oleg, IIRC) by making auditctl 
>    -a task,never cause newly spawned tasks to not suck.  Yes, it's a 
>    very partial solution.  After considerable nagging, I got Fedora to 
>    default to -a task,never.

Hm, right; that's a bit inconvenient, because it takes each and every 
vendor having to realize this option, and put it in. Making kernel do the 
right thing automatically sounds like a better option to me.

> 2. This patch, as is, may be a bit problematic.  In particular, if one 
>    task changes the audit rules while another task is in the middle of 
>    the syscall, then it's too late to audit that syscall correctly.  
>    This could be seen as a bug or it could be seen as being just fine.

I don't think this should be a problem, given the fact that the whole 
timing/ordering is not predictable anyway due to scheduling.

Paul, what do you think?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

[Paul Moore]

On Wed, Mar 7, 2018 at 11:48 AM, Jiri Kosina <jikos@kernel.org> wrote:
> On Wed, 7 Mar 2018, Andy Lutomirski wrote:
>> Wow, this was a long time ago.
>
> Oh yeah; but it now resurfaced on our side, as we are of course receiving
> a lot of requests with respect to making syscall performance great again
> :)

Ooof.  I'm not sure I can handle making more things "great again" ;)

>> From memory and a bit of email diving, there are two reasons.
>>
>> 1. The probably was partially solved (by Oleg, IIRC) by making auditctl
>>    -a task,never cause newly spawned tasks to not suck.  Yes, it's a
>>    very partial solution.  After considerable nagging, I got Fedora to
>>    default to -a task,never.
>
> Hm, right; that's a bit inconvenient, because it takes each and every
> vendor having to realize this option, and put it in. Making kernel do the
> right thing automatically sounds like a better option to me.

This predates audit falling into my lap, but speaking generally I
think it would be good if the kernel did The Right Thing, so long as
it isn't too painful.

>> 2. This patch, as is, may be a bit problematic.  In particular, if one
>>    task changes the audit rules while another task is in the middle of
>>    the syscall, then it's too late to audit that syscall correctly.
>>    This could be seen as a bug or it could be seen as being just fine.
>
> I don't think this should be a problem, given the fact that the whole
> timing/ordering is not predictable anyway due to scheduling.
>
> Paul, what do you think?

I'm not overly concerned about the race condition between configuring
the audit filters and syscalls that are currently in-flight; after all
we have that now and "fixing" it would be pretty much impractical
(impossible maybe?).  Most serious audit users configure it during
boot and let it run, frequent runtime changes are not common as far as
I can tell.

I just looked quickly at the patch and decided it isn't something I'm
going to be able to carefully review in the time I've got left today,
so it's going to have to wait until tomorrow and Friday ... however,
speaking on general principle I don't have an objection to the ideas
put forth here.

Andy, if you've got any Reviewed-by/Tested-by/NACK/etc. you want to
add, that would be good to have.

-- 
paul moore
www.paul-moore.com

Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

[Paul Moore]

On Wed, Mar 7, 2018 at 6:41 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Mar 7, 2018 at 11:48 AM, Jiri Kosina <jikos@kernel.org> wrote:
>> On Wed, 7 Mar 2018, Andy Lutomirski wrote:
>>> Wow, this was a long time ago.
>>
>> Oh yeah; but it now resurfaced on our side, as we are of course receiving
>> a lot of requests with respect to making syscall performance great again
>> :)
>
> Ooof.  I'm not sure I can handle making more things "great again" ;)
>
>>> From memory and a bit of email diving, there are two reasons.
>>>
>>> 1. The probably was partially solved (by Oleg, IIRC) by making auditctl
>>>    -a task,never cause newly spawned tasks to not suck.  Yes, it's a
>>>    very partial solution.  After considerable nagging, I got Fedora to
>>>    default to -a task,never.
>>
>> Hm, right; that's a bit inconvenient, because it takes each and every
>> vendor having to realize this option, and put it in. Making kernel do the
>> right thing automatically sounds like a better option to me.
>
> This predates audit falling into my lap, but speaking generally I
> think it would be good if the kernel did The Right Thing, so long as
> it isn't too painful.
>
>>> 2. This patch, as is, may be a bit problematic.  In particular, if one
>>>    task changes the audit rules while another task is in the middle of
>>>    the syscall, then it's too late to audit that syscall correctly.
>>>    This could be seen as a bug or it could be seen as being just fine.
>>
>> I don't think this should be a problem, given the fact that the whole
>> timing/ordering is not predictable anyway due to scheduling.
>>
>> Paul, what do you think?
>
> I'm not overly concerned about the race condition between configuring
> the audit filters and syscalls that are currently in-flight; after all
> we have that now and "fixing" it would be pretty much impractical
> (impossible maybe?).  Most serious audit users configure it during
> boot and let it run, frequent runtime changes are not common as far as
> I can tell.
>
> I just looked quickly at the patch and decided it isn't something I'm
> going to be able to carefully review in the time I've got left today,
> so it's going to have to wait until tomorrow and Friday ... however,
> speaking on general principle I don't have an objection to the ideas
> put forth here.
>
> Andy, if you've got any Reviewed-by/Tested-by/NACK/etc. you want to
> add, that would be good to have.

... and I just realized that linux-audit isn't on the To/CC line,
adding them now.

Link to the patch is below.

* https://marc.info/?t=152041887600003&r=1&w=2

-- 
paul moore
www.paul-moore.com

Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

[Andy Lutomirski]

On Wed, Mar 7, 2018 at 11:41 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Mar 7, 2018 at 11:48 AM, Jiri Kosina <jikos@kernel.org> wrote:
>> On Wed, 7 Mar 2018, Andy Lutomirski wrote:
>>> Wow, this was a long time ago.
>>
>> Oh yeah; but it now resurfaced on our side, as we are of course receiving
>> a lot of requests with respect to making syscall performance great again
>> :)
>
> Ooof.  I'm not sure I can handle making more things "great again" ;)
>
>>> From memory and a bit of email diving, there are two reasons.
>>>
>>> 1. The probably was partially solved (by Oleg, IIRC) by making auditctl
>>>    -a task,never cause newly spawned tasks to not suck.  Yes, it's a
>>>    very partial solution.  After considerable nagging, I got Fedora to
>>>    default to -a task,never.
>>
>> Hm, right; that's a bit inconvenient, because it takes each and every
>> vendor having to realize this option, and put it in. Making kernel do the
>> right thing automatically sounds like a better option to me.
>
> This predates audit falling into my lap, but speaking generally I
> think it would be good if the kernel did The Right Thing, so long as
> it isn't too painful.
>
>>> 2. This patch, as is, may be a bit problematic.  In particular, if one
>>>    task changes the audit rules while another task is in the middle of
>>>    the syscall, then it's too late to audit that syscall correctly.
>>>    This could be seen as a bug or it could be seen as being just fine.
>>
>> I don't think this should be a problem, given the fact that the whole
>> timing/ordering is not predictable anyway due to scheduling.
>>
>> Paul, what do you think?
>
> I'm not overly concerned about the race condition between configuring
> the audit filters and syscalls that are currently in-flight; after all
> we have that now and "fixing" it would be pretty much impractical
> (impossible maybe?).  Most serious audit users configure it during
> boot and let it run, frequent runtime changes are not common as far as
> I can tell.
>
> I just looked quickly at the patch and decided it isn't something I'm
> going to be able to carefully review in the time I've got left today,
> so it's going to have to wait until tomorrow and Friday ... however,
> speaking on general principle I don't have an objection to the ideas
> put forth here.
>
> Andy, if you've got any Reviewed-by/Tested-by/NACK/etc. you want to
> add, that would be good to have.
>

It's sort of my patch, and I was reasonably happy with it, so
definitely no NACK from me.  The only caveat I have is that I wrote
the original version so long ago that we need to re-audit the code.
In particular, I want to make sure that the following two cases don't
result in warnings, oopses, or other misbehavior:

1. Do a syscall with TIF_AUDIT_SYSCALL clear.  Return with
TIF_AUDIT_SYSCALL set and that syscall enabled for auditing.

2.. Do a VFS syscall with TIF_AUDIT_CLEAR and have TIF_AUDIT_SYSCALL
set before we execute any VFS code.  The VFS code will call into the
audit code to log the paths it touches (IIRC).  Again, this shouldn't
warn or otherwise misbehave.

Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

[Richard Guy Briggs]

On 2018-03-07 18:43, Paul Moore wrote:
> On Wed, Mar 7, 2018 at 6:41 PM, Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, Mar 7, 2018 at 11:48 AM, Jiri Kosina <jikos@kernel.org> wrote:
> >> On Wed, 7 Mar 2018, Andy Lutomirski wrote:
> >>> Wow, this was a long time ago.
> >>
> >> Oh yeah; but it now resurfaced on our side, as we are of course receiving
> >> a lot of requests with respect to making syscall performance great again
> >> :)
> >
> > Ooof.  I'm not sure I can handle making more things "great again" ;)
> >
> >>> From memory and a bit of email diving, there are two reasons.
> >>>
> >>> 1. The probably was partially solved (by Oleg, IIRC) by making auditctl
> >>>    -a task,never cause newly spawned tasks to not suck.  Yes, it's a
> >>>    very partial solution.  After considerable nagging, I got Fedora to
> >>>    default to -a task,never.
> >>
> >> Hm, right; that's a bit inconvenient, because it takes each and every
> >> vendor having to realize this option, and put it in. Making kernel do the
> >> right thing automatically sounds like a better option to me.
> >
> > This predates audit falling into my lap, but speaking generally I
> > think it would be good if the kernel did The Right Thing, so long as
> > it isn't too painful.
> >
> >>> 2. This patch, as is, may be a bit problematic.  In particular, if one
> >>>    task changes the audit rules while another task is in the middle of
> >>>    the syscall, then it's too late to audit that syscall correctly.
> >>>    This could be seen as a bug or it could be seen as being just fine.
> >>
> >> I don't think this should be a problem, given the fact that the whole
> >> timing/ordering is not predictable anyway due to scheduling.
> >>
> >> Paul, what do you think?
> >
> > I'm not overly concerned about the race condition between configuring
> > the audit filters and syscalls that are currently in-flight; after all
> > we have that now and "fixing" it would be pretty much impractical
> > (impossible maybe?).  Most serious audit users configure it during
> > boot and let it run, frequent runtime changes are not common as far as
> > I can tell.

I'd agree the race condition here can't easily be fixed and isn't worth
fixing for the reasons already stated (rules don't change often and may
even be locked once in place relatively early, scheduling uncertainties).

> > I just looked quickly at the patch and decided it isn't something I'm
> > going to be able to carefully review in the time I've got left today,
> > so it's going to have to wait until tomorrow and Friday ... however,
> > speaking on general principle I don't have an objection to the ideas
> > put forth here.

The approach seems a bit draconian since it touches all tasks but only
when adding the first rule or deleting the last.

What we lose is the ability to set or clear individual task auditing and
have it stick to speed up non-audited tasks when there are rules
present, though this isn't currently used, in favour of audit_context
presence.

> > Andy, if you've got any Reviewed-by/Tested-by/NACK/etc. you want to
> > add, that would be good to have.
> 
> ... and I just realized that linux-audit isn't on the To/CC line,
> adding them now.

(and Andy's non-NACK missed too...)  The mailing list *is* in MAINTAINERS.

> Link to the patch is below.
> 
> * https://marc.info/?t=152041887600003&r=1&w=2
> 
> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

[Andy Lutomirski]

> On Mar 8, 2018, at 1:12 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> 
>> On 2018-03-07 18:43, Paul Moore wrote:
>>> On Wed, Mar 7, 2018 at 6:41 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>> On Wed, Mar 7, 2018 at 11:48 AM, Jiri Kosina <jikos@kernel.org> wrote:
>>>>> On Wed, 7 Mar 2018, Andy Lutomirski wrote:
>>>>> Wow, this was a long time ago.
>>>> 
>>>> Oh yeah; but it now resurfaced on our side, as we are of course receiving
>>>> a lot of requests with respect to making syscall performance great again
>>>> :)
>>> 
>>> Ooof.  I'm not sure I can handle making more things "great again" ;)
>>> 
>>>>> From memory and a bit of email diving, there are two reasons.
>>>>> 
>>>>> 1. The probably was partially solved (by Oleg, IIRC) by making auditctl
>>>>>   -a task,never cause newly spawned tasks to not suck.  Yes, it's a
>>>>>   very partial solution.  After considerable nagging, I got Fedora to
>>>>>   default to -a task,never.
>>>> 
>>>> Hm, right; that's a bit inconvenient, because it takes each and every
>>>> vendor having to realize this option, and put it in. Making kernel do the
>>>> right thing automatically sounds like a better option to me.
>>> 
>>> This predates audit falling into my lap, but speaking generally I
>>> think it would be good if the kernel did The Right Thing, so long as
>>> it isn't too painful.
>>> 
>>>>> 2. This patch, as is, may be a bit problematic.  In particular, if one
>>>>>   task changes the audit rules while another task is in the middle of
>>>>>   the syscall, then it's too late to audit that syscall correctly.
>>>>>   This could be seen as a bug or it could be seen as being just fine.
>>>> 
>>>> I don't think this should be a problem, given the fact that the whole
>>>> timing/ordering is not predictable anyway due to scheduling.
>>>> 
>>>> Paul, what do you think?
>>> 
>>> I'm not overly concerned about the race condition between configuring
>>> the audit filters and syscalls that are currently in-flight; after all
>>> we have that now and "fixing" it would be pretty much impractical
>>> (impossible maybe?).  Most serious audit users configure it during
>>> boot and let it run, frequent runtime changes are not common as far as
>>> I can tell.
> 
> I'd agree the race condition here can't easily be fixed and isn't worth
> fixing for the reasons already stated (rules don't change often and may
> even be locked once in place relatively early, scheduling uncertainties).
> 
>>> I just looked quickly at the patch and decided it isn't something I'm
>>> going to be able to carefully review in the time I've got left today,
>>> so it's going to have to wait until tomorrow and Friday ... however,
>>> speaking on general principle I don't have an objection to the ideas
>>> put forth here.
> 
> The approach seems a bit draconian since it touches all tasks but only
> when adding the first rule or deleting the last.
> 
> What we lose is the ability to set or clear individual task auditing and
> have it stick to speed up non-audited tasks when there are rules
> present, though this isn't currently used, in favour of audit_context
> presence.
> 
>>> Andy, if you've got any Reviewed-by/Tested-by/NACK/etc. you want to
>>> add, that would be good to have.
>> 
>> ... and I just realized that linux-audit isn't on the To/CC line,
>> adding them now.
> 
> (and Andy's non-NACK missed too...)  The mailing list *is* in MAINTAINERS.
> 

The mailing list bounces my emails. 

>> Link to the patch is below.
>> 
>> * https://marc.info/?t=152041887600003&r=1&w=2
>> 
>> paul moore
> 
> - RGB
> 
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

[Richard Guy Briggs]

On 2018-03-08 06:30, Andy Lutomirski wrote:
> 
> 
> > On Mar 8, 2018, at 1:12 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > 
> >> On 2018-03-07 18:43, Paul Moore wrote:
> >>> On Wed, Mar 7, 2018 at 6:41 PM, Paul Moore <paul@paul-moore.com> wrote:
> >>>> On Wed, Mar 7, 2018 at 11:48 AM, Jiri Kosina <jikos@kernel.org> wrote:
> >>>>> On Wed, 7 Mar 2018, Andy Lutomirski wrote:
> >>>>> Wow, this was a long time ago.
> >>>> 
> >>>> Oh yeah; but it now resurfaced on our side, as we are of course receiving
> >>>> a lot of requests with respect to making syscall performance great again
> >>>> :)
> >>> 
> >>> Ooof.  I'm not sure I can handle making more things "great again" ;)
> >>> 
> >>>>> From memory and a bit of email diving, there are two reasons.
> >>>>> 
> >>>>> 1. The probably was partially solved (by Oleg, IIRC) by making auditctl
> >>>>>   -a task,never cause newly spawned tasks to not suck.  Yes, it's a
> >>>>>   very partial solution.  After considerable nagging, I got Fedora to
> >>>>>   default to -a task,never.
> >>>> 
> >>>> Hm, right; that's a bit inconvenient, because it takes each and every
> >>>> vendor having to realize this option, and put it in. Making kernel do the
> >>>> right thing automatically sounds like a better option to me.
> >>> 
> >>> This predates audit falling into my lap, but speaking generally I
> >>> think it would be good if the kernel did The Right Thing, so long as
> >>> it isn't too painful.
> >>> 
> >>>>> 2. This patch, as is, may be a bit problematic.  In particular, if one
> >>>>>   task changes the audit rules while another task is in the middle of
> >>>>>   the syscall, then it's too late to audit that syscall correctly.
> >>>>>   This could be seen as a bug or it could be seen as being just fine.
> >>>> 
> >>>> I don't think this should be a problem, given the fact that the whole
> >>>> timing/ordering is not predictable anyway due to scheduling.
> >>>> 
> >>>> Paul, what do you think?
> >>> 
> >>> I'm not overly concerned about the race condition between configuring
> >>> the audit filters and syscalls that are currently in-flight; after all
> >>> we have that now and "fixing" it would be pretty much impractical
> >>> (impossible maybe?).  Most serious audit users configure it during
> >>> boot and let it run, frequent runtime changes are not common as far as
> >>> I can tell.
> > 
> > I'd agree the race condition here can't easily be fixed and isn't worth
> > fixing for the reasons already stated (rules don't change often and may
> > even be locked once in place relatively early, scheduling uncertainties).
> > 
> >>> I just looked quickly at the patch and decided it isn't something I'm
> >>> going to be able to carefully review in the time I've got left today,
> >>> so it's going to have to wait until tomorrow and Friday ... however,
> >>> speaking on general principle I don't have an objection to the ideas
> >>> put forth here.
> > 
> > The approach seems a bit draconian since it touches all tasks but only
> > when adding the first rule or deleting the last.
> > 
> > What we lose is the ability to set or clear individual task auditing and
> > have it stick to speed up non-audited tasks when there are rules
> > present, though this isn't currently used, in favour of audit_context
> > presence.
> > 
> >>> Andy, if you've got any Reviewed-by/Tested-by/NACK/etc. you want to
> >>> add, that would be good to have.
> >> 
> >> ... and I just realized that linux-audit isn't on the To/CC line,
> >> adding them now.
> > 
> > (and Andy's non-NACK missed too...)  The mailing list *is* in MAINTAINERS.
> > 
> 
> The mailing list bounces my emails. 

They'll get approved.

> >> Link to the patch is below.
> >> 
> >> * https://marc.info/?t=152041887600003&r=1&w=2
> >> 
> >> paul moore
> > 
> > - RGB

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

[Steve Grubb]

On Wed, 7 Mar 2018 18:43:42 -0500
Paul Moore <paul@paul-moore.com> wrote:
> ... and I just realized that linux-audit isn't on the To/CC line,
> adding them now.
> 
> Link to the patch is below.
> 
> * https://marc.info/?t=152041887600003&r=1&w=2

Yes...I wished I was in on the beginning of this discussion. Here's the
problem. We need all tasks auditable unless specifically dismissed as
uninteresting. This would be a task,never rule.

The way we look at it, is if it boots with audit=1, then we know auditd
is expected to run at some point. So, we need all tasks to stay
auditable. If they weren't and auditd enabled auditing, then we'd need
to walk the whole proctable and stab TIF_AUDIT_SYSCALL into every
process in the system. It was decided that this is too ugly.

So, we need them all to be auditable if there is any intent to audit.
It doesn't matter if there are rules loaded or not. All processes have
to stay within reach.

What might be acceptable is to add one more state to audit boot variable
to indicate that auditing is never expected. We currently have:
disabled - which means we'll decide later, enabled, and immutable (no
changes allowed). Then have calls to audit_enable or loading rules
fail on that flag state so that user space can log that there is a
conflict (boot vs daemon) that has to be resolved. As long as we can
fail in a discoverable way, I think it would be OK to do something like
this. Also, I don't think we want that to be the default state at the
moment because the current default is keep all processes auditable.

-Steve

Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

[Andy Lutomirski]

On Sat, Mar 10, 2018 at 10:15 AM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Wed, 7 Mar 2018 18:43:42 -0500
> Paul Moore <paul@paul-moore.com> wrote:
>> ... and I just realized that linux-audit isn't on the To/CC line,
>> adding them now.
>>
>> Link to the patch is below.
>>
>> * https://marc.info/?t=152041887600003&r=1&w=2
>
> Yes...I wished I was in on the beginning of this discussion. Here's the
> problem. We need all tasks auditable unless specifically dismissed as
> uninteresting. This would be a task,never rule.
>
> The way we look at it, is if it boots with audit=1, then we know auditd
> is expected to run at some point. So, we need all tasks to stay
> auditable. If they weren't and auditd enabled auditing, then we'd need
> to walk the whole proctable and stab TIF_AUDIT_SYSCALL into every
> process in the system. It was decided that this is too ugly.

When was that decided?  That's what this patch does.

Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

[Jiri Kosina]

On Wed, 14 Mar 2018, Andy Lutomirski wrote:

> > Yes...I wished I was in on the beginning of this discussion. Here's the
> > problem. We need all tasks auditable unless specifically dismissed as
> > uninteresting. This would be a task,never rule.
> >
> > The way we look at it, is if it boots with audit=1, then we know auditd
> > is expected to run at some point. So, we need all tasks to stay
> > auditable. If they weren't and auditd enabled auditing, then we'd need
> > to walk the whole proctable and stab TIF_AUDIT_SYSCALL into every
> > process in the system. It was decided that this is too ugly.
> 
> When was that decided?  That's what this patch does.

I'd like to see some more justification as well.

Namely, if I compare "setting TIF_AUDIT_SYSCALL for every process on a 
need-to-be-so basis" to "we always go through the slow path and 
pessimistically assume that audit is enabled and has reasonable ruleset 
loaded", I have my own (different) opinion of what is too ugly.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

[Andy Lutomirski]

On Wed, Mar 14, 2018 at 12:28 AM, Jiri Kosina <jikos@kernel.org> wrote:
> On Wed, 14 Mar 2018, Andy Lutomirski wrote:
>
>> > Yes...I wished I was in on the beginning of this discussion. Here's the
>> > problem. We need all tasks auditable unless specifically dismissed as
>> > uninteresting. This would be a task,never rule.
>> >
>> > The way we look at it, is if it boots with audit=1, then we know auditd
>> > is expected to run at some point. So, we need all tasks to stay
>> > auditable. If they weren't and auditd enabled auditing, then we'd need
>> > to walk the whole proctable and stab TIF_AUDIT_SYSCALL into every
>> > process in the system. It was decided that this is too ugly.
>>
>> When was that decided?  That's what this patch does.
>
> I'd like to see some more justification as well.
>
> Namely, if I compare "setting TIF_AUDIT_SYSCALL for every process on a
> need-to-be-so basis" to "we always go through the slow path and
> pessimistically assume that audit is enabled and has reasonable ruleset
> loaded", I have my own (different) opinion of what is too ugly.

Me too.

That being said, on re-review of my old code, I think that
audit_dec_n_rules() may be the wrong approach.  It may be better to
leave TIF_AUDIT_SYSCALL set but to make the audit code itself clear
the flag the next time through.  That way we don't end up with a
partially filled in syscall audit record that never gets consumed if
we clear TIF_AUDIT_SYSCALL in the middle of a syscall.

Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

[Steve Grubb]

On Tuesday, March 13, 2018 8:28:57 PM EDT Jiri Kosina wrote:
> On Wed, 14 Mar 2018, Andy Lutomirski wrote:
> > > Yes...I wished I was in on the beginning of this discussion. Here's the
> > > problem. We need all tasks auditable unless specifically dismissed as
> > > uninteresting. This would be a task,never rule.
> > > 
> > > The way we look at it, is if it boots with audit=1, then we know auditd
> > > is expected to run at some point. So, we need all tasks to stay
> > > auditable. If they weren't and auditd enabled auditing, then we'd need
> > > to walk the whole proctable and stab TIF_AUDIT_SYSCALL into every
> > > process in the system. It was decided that this is too ugly.
> > 
> > When was that decided?  That's what this patch does.
> 
> I'd like to see some more justification as well.

There was some discussion about removing the flag here:
https://www.redhat.com/archives/linux-audit/2007-October/msg00053.html

-Steve

> Namely, if I compare "setting TIF_AUDIT_SYSCALL for every process on a
> need-to-be-so basis" to "we always go through the slow path and
> pessimistically assume that audit is enabled and has reasonable ruleset
> loaded", I have my own (different) opinion of what is too ugly.

Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

[Steve Grubb]

On Tuesday, March 13, 2018 8:35:44 PM EDT Andy Lutomirski wrote:
> On Wed, Mar 14, 2018 at 12:28 AM, Jiri Kosina <jikos@kernel.org> wrote:
> > On Wed, 14 Mar 2018, Andy Lutomirski wrote:
> >> > Yes...I wished I was in on the beginning of this discussion. Here's
> >> > the
> >> > problem. We need all tasks auditable unless specifically dismissed as
> >> > uninteresting. This would be a task,never rule.
> >> > 
> >> > The way we look at it, is if it boots with audit=1, then we know
> >> > auditd
> >> > is expected to run at some point. So, we need all tasks to stay
> >> > auditable. If they weren't and auditd enabled auditing, then we'd need
> >> > to walk the whole proctable and stab TIF_AUDIT_SYSCALL into every
> >> > process in the system. It was decided that this is too ugly.
> >> 
> >> When was that decided?  That's what this patch does.
> > 
> > I'd like to see some more justification as well.
> > 
> > Namely, if I compare "setting TIF_AUDIT_SYSCALL for every process on a
> > need-to-be-so basis" to "we always go through the slow path and
> > pessimistically assume that audit is enabled and has reasonable ruleset
> > loaded", I have my own (different) opinion of what is too ugly.
> 
> Me too.
> 
> That being said, on re-review of my old code, I think that
> audit_dec_n_rules() may be the wrong approach.  It may be better to
> leave TIF_AUDIT_SYSCALL set but to make the audit code itself clear
> the flag the next time through.  That way we don't end up with a
> partially filled in syscall audit record that never gets consumed if
> we clear TIF_AUDIT_SYSCALL in the middle of a syscall.

So what happened when auditd is being restarted due to system update? Its 
possible to have no auditd, no rules, and audit_enabled == 0. But its getting 
ready to start a new auditd, enable audit, and load rules. In this case, we 
expect tasks already dismissed by task filter to stay dismissed because the 
task filter only runs at fork time.

For example, suppose httpd is not desired to be audited. The admin sets up a 
task rule that sets it to never for httpd. The rule triggers at fork and 
marks it inauditable. Would this patch cause httpd to suddenly become 
auditable again during an auditd restart?

Rather than play run time games which is ultimately racey and prone to 
missing something, what about the approach of controlling this from a boot 
variable and letting user space see an error should auditing try to do its 
normal thing when its not wanted? This way auditd can exit and there aren't 
any races.

-Steve