2008-08-19 15:18:50

by David Howells

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

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 <[email protected]>
---

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:

2008-08-19 23:30:32

by James Morris

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

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 <[email protected]>

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
<[email protected]>

2008-08-20 12:00:43

by Alexander Beregalov

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

> 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