[PATCH] CRED: Take cred_exec_mutex in compat_do_execve() and fix error handling in do_execve()

[PATCH] CRED: Take cred_exec_mutex in compat_do_execve() and fix error handling in do_execve()

[David Howells]

Take cred_exec_mutex in compat_do_execve().  This reflects what do_execve()
does.  The mutex protects credentials calculation against PTRACE_ATTACH needing
to alter it mid-exec.

Also fix the error handling in do_execve().  The mutex needs to be unlocked if
an error occurs after it is taken, but before the install_exec_creds()
released it.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/compat.c |   12 ++++++++++--
 fs/exec.c   |   12 ++++++++----
 2 files changed, 18 insertions(+), 6 deletions(-)


diff --git a/fs/compat.c b/fs/compat.c
index 2dde882..af24b8a 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -1360,15 +1360,20 @@ int compat_do_execve(char * filename,
 	if (!bprm)
 		goto out_ret;
 
+	retval = mutex_lock_interruptible(&current->cred_exec_mutex);
+	if (retval < 0)
+		goto out_free;
+
+	retval = -ENOMEM;
 	bprm->cred = prepare_exec_creds();
 	if (!bprm->cred)
-		goto out_free;
+		goto out_unlock;
 	check_unsafe_exec(bprm);
 
 	file = open_exec(filename);
 	retval = PTR_ERR(file);
 	if (IS_ERR(file))
-		goto out_free;
+		goto out_unlock;
 
 	sched_exec();
 
@@ -1423,6 +1428,9 @@ out_file:
 		fput(bprm->file);
 	}
 
+out_unlock:
+	mutex_unlock(&current->cred_exec_mutex);
+
 out_free:
 	free_bprm(bprm);
 
diff --git a/fs/exec.c b/fs/exec.c
index a7633e5..4b31a72 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1308,18 +1308,18 @@ int do_execve(char * filename,
 
 	retval = mutex_lock_interruptible(&current->cred_exec_mutex);
 	if (retval < 0)
-		goto out_kfree;
+		goto out_free;
 
 	retval = -ENOMEM;
 	bprm->cred = prepare_exec_creds();
 	if (!bprm->cred)
-		goto out_kfree;
+		goto out_unlock;
 	check_unsafe_exec(bprm);
 
 	file = open_exec(filename);
 	retval = PTR_ERR(file);
 	if (IS_ERR(file))
-		goto out_kfree;
+		goto out_unlock;
 
 	sched_exec();
 
@@ -1376,7 +1376,11 @@ out_file:
 		allow_write_access(bprm->file);
 		fput(bprm->file);
 	}
-out_kfree:
+
+out_unlock:
+	mutex_unlock(&current->cred_exec_mutex);
+
+out_free:
 	free_bprm(bprm);
 
 out_files:

Re: [PATCH] CRED: Take cred_exec_mutex in compat_do_execve() and fix error handling in do_execve()

[James Morris]

On Tue, 19 Aug 2008, David Howells wrote:

> Take cred_exec_mutex in compat_do_execve().  This reflects what do_execve()
> does.  The mutex protects credentials calculation against PTRACE_ATTACH needing
> to alter it mid-exec.
> 
> Also fix the error handling in do_execve().  The mutex needs to be unlocked if
> an error occurs after it is taken, but before the install_exec_creds()
> released it.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>

Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next-creds

Hopefully this will be in the next linux-next tree for testing.

Alexander, please let us know if it fixes the problem you saw.


- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] CRED: Take cred_exec_mutex in compat_do_execve() and fix error handling in do_execve()

[Alexander Beregalov]

> Applied to
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next-creds
>
> Hopefully this will be in the next linux-next tree for testing.
>
> Alexander, please let us know if it fixes the problem you saw.

Yes, it seems fixed.
David, there is one more problem.

2.6.27-rc3-next-20080820:

[   22.266394] [ BUG: bad unlock balance detected! ]
[   22.266495] -------------------------------------
[   22.266594] khelper/8 is trying to release lock (&p->cred_exec_mutex) at:
[   22.266775] [<00000000006af9f8>] mutex_unlock+0x10/0x20
[   22.266881] but there are no more locks to release!
[   22.266982]
[   22.266987] other info that might help us debug this:
[   22.267136] no locks held by khelper/8.
[   22.267224]
[   22.267229] stack backtrace:
[   22.267343] Call Trace:
[   22.267436]  [000000000047800c] print_unlock_inbalance_bug+0xe8/0xf8
[   22.267569]  [0000000000478338] lock_release+0x98/0x1ac
[   22.267685]  [00000000006af954] __mutex_unlock_slowpath+0xbc/0x150
[   22.267816]  [00000000006af9f8] mutex_unlock+0x10/0x20
[   22.267931]  [00000000004b81fc] free_bprm+0x1c/0x3c
[   22.268043]  [00000000004b93d4] do_execve+0x230/0x254
[   22.268158]  [00000000004273d8] sparc_execve+0x64/0xb0
[   22.268284]  [0000000000406194] linux_sparc_syscall+0x34/0x44
[   22.268407]  [0000000000432cf0] kernel_execve+0x20/0x34
[   22.268535]  [0000000000462fa8] ____call_usermodehelper+0xf4/0x10c
[   22.268661]  [0000000000427260] kernel_thread+0x3c/0x54
[   22.268779]  [0000000000462e7c] __call_usermodehelper+0x7c/0xb4