[PATCH] Audit: close race between audit_syscall_exit and proc_loginuid_read

[PATCH] Audit: close race between audit_syscall_exit and proc_loginuid_read

[Eric Paris]

audit_syscall_exit() calls audit_get_context() which returns the
tsk->audit_context but then also sets tsk->audit_context=NULL.  A few
lines later audit_syscall_exit sets the audit_context (either back to
audit_context or to a different context)

During this window when tsk->audit_context is NULL it is possible that
another process will try to read /proc/pid/loginuid and will get a -1.
There does not appear to be a good reason to set audit_context to null
in the get function so this patch merely leaves the audit_context alone
so there is no period of time in which audit_context is not pointing to
a valid context.

Signed-off-by: Eric Paris <eparis@redhat.com>

---

Easily tested, I opened 2 shells.  

shell #1 I read /proc/shell2/loginuid in a very very tight loop.
shell #2 I ran a tight loop adding and removing audit rules

without the patch I often saw -1 as the loginuid but with the patch I
always got the correct behavior.


 kernel/auditsc.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 04f3ffb..62ace9b 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -598,7 +598,6 @@ static inline struct audit_context *audit_get_context(struct task_struct *tsk,
 
 get_context:
 
-	tsk->audit_context = NULL;
 	return context;
 }
 
@@ -1135,6 +1134,8 @@ void audit_free(struct task_struct *tsk)
 	if (likely(!context))
 		return;
 
+	tsk->audit_context = NULL;
+
 	/* Check for system calls that do not go through the exit
 	 * function (e.g., exit_group), then free context block. 
 	 * We use GFP_ATOMIC here because we might be doing this 
@@ -1266,9 +1267,9 @@ void audit_syscall_exit(int valid, long return_code)
 
 	if (context->previous) {
 		struct audit_context *new_context = context->previous;
+		tsk->audit_context = new_context;
 		context->previous  = NULL;
 		audit_free_context(context);
-		tsk->audit_context = new_context;
 	} else {
 		audit_free_names(context);
 		audit_free_aux(context);

Re: [PATCH] Audit: close race between audit_syscall_exit and proc_loginuid_read

[Steve Grubb]

On Wednesday 03 October 2007 14:21:35 Eric Paris wrote:
> audit_syscall_exit() calls audit_get_context() which returns the
> tsk->audit_context but then also sets tsk->audit_context=NULL.

The preferred way to solve this problem is to promote loginuid to be part of 
the task structure. We also have another problem where if the audit system is 
temporarily disabled, loginuid will not be set in any new programs. What we'd 
like to have happen is loginuid be available and usable all the time so that 
we always know the auid when the audit system starts back up.

-Steve

Re: [PATCH] Audit: close race between audit_syscall_exit and proc_loginuid_read

[Eric Paris]

On Wed, 2007-10-03 at 14:29 -0400, Steve Grubb wrote:
> On Wednesday 03 October 2007 14:21:35 Eric Paris wrote:
> > audit_syscall_exit() calls audit_get_context() which returns the
> > tsk->audit_context but then also sets tsk->audit_context=NULL.
> 
> The preferred way to solve this problem is to promote loginuid to be part of 
> the task structure. We also have another problem where if the audit system is 
> temporarily disabled, loginuid will not be set in any new programs. What we'd 
> like to have happen is loginuid be available and usable all the time so that 
> we always know the auid when the audit system starts back up.

While obviously possible I don't see a reason that this patch is bad,
wrong, or shouldn't go in.  If things other than audit want to make use
of the loginuid I wouldn't see a problem putting it in the task struct
but as it stands now this patch fixes an obvious race for a solely audit
problem.

-Eric