[PATCH] audit: clear thread flag for new children

[PATCH] audit: clear thread flag for new children

[Tony Jones]

From: Tony Jones <tonyj@suse.de>

Minor performance enhancement.

Thread flag TIF_SYSCALL_AUDIT is not cleared for new children when audit 
context creation has been disabled (auditctl -e0). This can cause new children 
forked from a parent created when audit was enabled to not take the fastest 
syscall path thru entry.S

Signed-off-by: Tony Jones <tonyj@suse.de>
---
 kernel/auditsc.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -814,8 +814,10 @@ int audit_alloc(struct task_struct *tsk)
 	struct audit_context *context;
 	enum audit_state     state;
 
-	if (likely(!audit_enabled))
+	if (likely(!audit_enabled)) {
+		clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT);
 		return 0; /* Return if not auditing. */
+	}
 
 	state = audit_filter_task(tsk);
 	if (likely(state == AUDIT_DISABLED))

Re: [PATCH] audit: clear thread flag for new children

[Chris Wright]

* Tony Jones (tonyj@suse.de) wrote:
> From: Tony Jones <tonyj@suse.de>
> Minor performance enhancement.
> 
> Thread flag TIF_SYSCALL_AUDIT is not cleared for new children when audit 
> context creation has been disabled (auditctl -e0). This can cause new children 
> forked from a parent created when audit was enabled to not take the fastest 
> syscall path thru entry.S
> 
> Signed-off-by: Tony Jones <tonyj@suse.de>

Yeah, I think that's the right thing to do.  Doesn't have an
audit_context anyway.

Acked-by: Chris Wright <chrisw@sous-sol.org>

Re: [PATCH] audit: clear thread flag for new children

[Steve Grubb]

On Friday 26 October 2007 04:42:28 pm Tony Jones wrote:
> Thread flag TIF_SYSCALL_AUDIT is not cleared for new children when audit
> context creation has been disabled (auditctl -e0). This can cause new
> children forked from a parent created when audit was enabled to not take
> the fastest syscall path thru entry.S

This came up almost 2 years ago:

https://www.redhat.com/archives/linux-audit/2005-September/msg00048.html

The problem is that removing that flag makes the children unauditable in the 
future. The only place that flag gets set is during fork. Unless I'm missing 
something, to make all children auditable again would mean stopping all 
processes and or'ing that flag into all thread info areas. I do not want to 
propose that patch to LKML.  :)

-Steve

Re: [PATCH] audit: clear thread flag for new children

[Tony Jones]

On Sat, Oct 27, 2007 at 10:21:39AM -0400, Steve Grubb wrote:
> On Friday 26 October 2007 04:42:28 pm Tony Jones wrote:
> > Thread flag TIF_SYSCALL_AUDIT is not cleared for new children when audit
> > context creation has been disabled (auditctl -e0). This can cause new
> > children forked from a parent created when audit was enabled to not take
> > the fastest syscall path thru entry.S
> 
> This came up almost 2 years ago:
> 
> https://www.redhat.com/archives/linux-audit/2005-September/msg00048.html

I was not aware of this, thanks.

> The problem is that removing that flag makes the children unauditable in the 
> future. The only place that flag gets set is during fork. 

I don't see this.  The case that would be undesirable would be for a task
to have an audit context but to not have the thread flag enabled.  That isn't
the case.  This was the point Chris made in his Ack, although perhaps somewhat
tersely.

> Unless I'm missing something, to make all children auditable again would 
> mean stopping all processes and or'ing that flag into all thread info areas. 

I think you are.  Or maybe the code was different two years ago so that the
above made sense.   

In the above scenario, audit is disabled, a new child is forked, we bail
early so there is no audit context (and now there is no flag in the thread
area).   Currently there is no way this task is ever going to be audited as 
there is no audit context.    If this task forks a new child, at this point
the value of audit enabled will determine if there should be a context 
allocated and it will allocate the TIF flag also.   I don't see your stopping
all processes scenario.

> I do not want to propose that patch to LKML.  :)

;-)

Tony

Re: [PATCH] audit: clear thread flag for new children

[Steve Grubb]

On Monday 29 October 2007 01:20:58 pm Tony Jones wrote:
> > The problem is that removing that flag makes the children unauditable in
> > the future. The only place that flag gets set is during fork.
>
> I don't see this.

If the child does not have the TIF_SYSCALL_AUDIT flag, it never goes into 
audit_syscall_entry. It becomes unauditable.


> The case that would be undesirable would be for a task to have an audit
> context but to not have the thread flag enabled.

That would just be a small allocation of memory that will be returned when the 
process exits. From an auditing PoV, something that is undesirable is the 
inability to audit a process that you want to audit.


> > Unless I'm missing something, to make all children auditable again would
> > mean stopping all processes and or'ing that flag into all thread info
> > areas.
>
> I think you are.  Or maybe the code was different two years ago so that the
> above made sense.  
>
> In the above scenario, audit is disabled, a new child is forked, we bail
> early so there is no audit context (and now there is no flag in the thread
> area).   Currently there is no way this task is ever going to be audited as
> there is no audit context. 

So when audit is re-enabled, how do you make that task auditable?


> If this task forks a new child, at this point the value of audit enabled
> will determine if there should be a context allocated and it will allocate
> the TIF flag also.

In the new child, but not the parent.


> I don't see your stopping all processes scenario. 

That is so you can walk the process table and "or" the bit in unconditionally. 
All processes need to be auditable or you've got a security hole.

-Steve

Re: [PATCH] audit: clear thread flag for new children

[Tony Jones]

On Mon, Oct 29, 2007 at 06:04:31PM -0400, Steve Grubb wrote:

> If the child does not have the TIF_SYSCALL_AUDIT flag, it never goes into
> audit_syscall_entry. It becomes unauditable.

True but a task where current->audit_context == NULL is going to immediately
BUG out in audit_syscall_entry.  This is why the invocations of 
audit_syscall_entry() are conditional on current->audit_context.

> So when audit is re-enabled, how do you make that task auditable?

No idea. How do you do it currently? HINT: current->audit_context == NULL
for these tasks.  If !audit_enabled, then audit_alloc() is not going to 
allocate an audit_context for the task.

I'm very curious how you think one of these tasks becomes auditable later
on once audit is re-enabled,  regardless of the value of TIF_SYSCALL_AUDIT.

Tony

Re: [PATCH] audit: clear thread flag for new children

[Steve Grubb]

On Monday 29 October 2007 07:15:30 pm Tony Jones wrote:
> On Mon, Oct 29, 2007 at 06:04:31PM -0400, Steve Grubb wrote:
> > So when audit is re-enabled, how do you make that task auditable?
>
> No idea. How do you do it currently? HINT: current->audit_context == NULL
> for these tasks.  If !audit_enabled, then audit_alloc() is not going to
> allocate an audit_context for the task.

We are looking into this - at one time it did. Someone should follow up with a 
path correcting this soon. But I doubt the audit system will work correctly 
if the flag gets removed as there is no good way to add it again later.

-Steve

Re: [PATCH] audit: clear thread flag for new children

[Tony Jones]

On Thu, Nov 01, 2007 at 10:33:52AM -0400, Steve Grubb wrote:
> On Monday 29 October 2007 07:15:30 pm Tony Jones wrote:
> > On Mon, Oct 29, 2007 at 06:04:31PM -0400, Steve Grubb wrote:
> > > So when audit is re-enabled, how do you make that task auditable?
> >
> > No idea. How do you do it currently? HINT: current->audit_context == NULL
> > for these tasks.  If !audit_enabled, then audit_alloc() is not going to
> > allocate an audit_context for the task.
> 
> We are looking into this - at one time it did. Someone should follow up with 
> a path correcting this soon. But I doubt the audit system will work correctly 
> if the flag gets removed as there is no good way to add it again later.

So on the syscall path you're now going to something like (pseudocode):
if (unlikely(current->audit_context)) {audit_syscall_entry()} 
else if (audit_enabled) {audit_alloc(); audit_syscall_entry()}

I agree, if this is what you want to do, then clearing the thread flag would
be a bad idea.  I'd assumed the current behaviour was by design as allocating
contexts at syscall time doesn't seem a great idea but if you need the 
functionality, you need the functionality.

Tony

Re: [PATCH] audit: clear thread flag for new children

[Steve Grubb]

On Thursday 01 November 2007 01:23:24 pm Tony Jones wrote:
> > We are looking into this - at one time it did. Someone should follow up
> > with a path correcting this soon. But I doubt the audit system will work
> > correctly if the flag gets removed as there is no good way to add it
> > again later.
>
> So on the syscall path you're now going to something like (pseudocode):
> if (unlikely(current->audit_context)) {audit_syscall_entry()}
> else if (audit_enabled) {audit_alloc(); audit_syscall_entry()}

Something like that. The TIF flag is to pick off processes that we are 
interested in auditing, for performance reasons if audit is disabled the 
context is not created. But if auditing is re-enabled, processes can 
be "repaired" so that the are auditable again by allocating the context on 
the fly the first time we see a process. Without the flag, they never get 
steered into the audit system where this can happen.

> I agree, if this is what you want to do, then clearing the thread flag
> would be a bad idea. 

If any other performance improvements jumps out at you, please bring them up.

> I'd assumed the current behaviour was by design as allocating contexts at
> syscall time doesn't seem a great idea but if you need the functionality,
> you need the functionality. 

Yep

-Steve