audit: clear thread flag for new children

From: Tony Jones <[email protected]>

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

---
 kernel/auditsc.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

@@ -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))

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

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

I was not aware of this, thanks.

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.

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.

;-)

Tony

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

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.

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

In the new child, but not the parent.

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

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.

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

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

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

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.

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

Yep

-Steve