Re: c/r: broken locking when executing map_files

[Cyrill Gorcunov <gorcunov@openvz.org>]

On Wed, May 02, 2012 at 09:27:56PM +0400, Cyrill Gorcunov wrote:
> > 
> > My theory is that files under map_files shouldn't be executable,
> > but since I'm not sure what the usermode code for c/r does exactly,
> > I should probably confirm that first. If it's indeed the case, you
> > can probably skip the rest of this mail.
> 
> Thanks Sasha, I'll take a look!

Sasha, the patch below should fix the lock problem (still I would
prefer to obtain confirm on patch from Vasiliy, CC'ed).

	Cyrill
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: fs, proc: Fix ABBA deadlock in case of execution attempt of map_files/ entries

map_files/ entries are never supposed to be executed, still curious
minds might try to run them, which leads to the following deadlock

[  270.895009] ======================================================
[  270.895054] [ INFO: possible circular locking dependency detected ]
[  270.895100] 3.4.0-rc4-24406-g841e6a6 #121 Not tainted
[  270.895144] -------------------------------------------------------
[  270.895189] bash/1556 is trying to acquire lock:
[  270.895235]  (&sb->s_type->i_mutex_key#8){+.+.+.}, at: [<ffffffff8116917f>] do_lookup+0x267/0x2b1
[  270.895612] 
[  270.895613] but task is already holding lock:
[  270.895731]  (&sig->cred_guard_mutex){+.+.+.}, at: [<ffffffff81165244>] prepare_bprm_creds+0x2d/0x69
[  270.896081] 
[  270.896082] which lock already depends on the new lock.
[  270.896083] 
[  270.896220] 
[  270.896221] the existing dependency chain (in reverse order) is:
[  270.896340] 
[  270.896341] -> #1 (&sig->cred_guard_mutex){+.+.+.}:
[  270.896637]        [<ffffffff810b346c>] validate_chain+0x444/0x4f4
[  270.896734]        [<ffffffff810b409f>] __lock_acquire+0x387/0x3f8
[  270.896847]        [<ffffffff810b423b>] lock_acquire+0x12b/0x158
[  270.896950]        [<ffffffff81bd322c>] __mutex_lock_common+0x56/0x3a9
[  270.897056]        [<ffffffff81bd3604>] mutex_lock_killable_nested+0x40/0x45
[  270.897155]        [<ffffffff811b48e2>] lock_trace+0x24/0x59
[  270.897253]        [<ffffffff811b6903>] proc_map_files_lookup+0x5a/0x165
[  270.897365]        [<ffffffff81168ef7>] __lookup_hash+0x52/0x73
[  270.897463]        [<ffffffff8116918e>] do_lookup+0x276/0x2b1
[  270.897560]        [<ffffffff8116ab9a>] walk_component+0x3d/0x114
[  270.897657]        [<ffffffff8116ad6d>] do_last+0xfc/0x540
[  270.897753]        [<ffffffff8116b7a0>] path_openat+0xd3/0x306
[  270.897864]        [<ffffffff8116bacc>] do_filp_open+0x3d/0x89
[  270.897967]        [<ffffffff8115d7a8>] do_sys_open+0x74/0x106
[  270.898068]        [<ffffffff8115d871>] sys_open+0x21/0x23
[  270.898164]        [<ffffffff81bd6c50>] tracesys+0xdd/0xe2
[  270.898262] 
[  270.898263] -> #0 (&sb->s_type->i_mutex_key#8){+.+.+.}:
[  270.898598]        [<ffffffff810b22eb>] check_prev_add+0x6a/0x1ef
[  270.898696]        [<ffffffff810b346c>] validate_chain+0x444/0x4f4
[  270.898810]        [<ffffffff810b409f>] __lock_acquire+0x387/0x3f8
[  270.898908]        [<ffffffff810b423b>] lock_acquire+0x12b/0x158
[  270.899005]        [<ffffffff81bd322c>] __mutex_lock_common+0x56/0x3a9
[  270.899103]        [<ffffffff81bd368e>] mutex_lock_nested+0x40/0x45
[  270.899200]        [<ffffffff8116917f>] do_lookup+0x267/0x2b1
[  270.899309]        [<ffffffff8116ab9a>] walk_component+0x3d/0x114
[  270.899408]        [<ffffffff8116b3aa>] link_path_walk+0x1f9/0x48f
[  270.899505]        [<ffffffff8116b783>] path_openat+0xb6/0x306
[  270.899602]        [<ffffffff8116bacc>] do_filp_open+0x3d/0x89
[  270.899699]        [<ffffffff81165bcb>] open_exec+0x25/0xa0
[  270.899809]        [<ffffffff811664af>] do_execve_common+0xea/0x2f9
[  270.899907]        [<ffffffff81166752>] do_execve+0x43/0x45
[  270.900004]        [<ffffffff81018ac0>] sys_execve+0x43/0x5a
[  270.900102]        [<ffffffff81bd6eec>] stub_execve+0x6c/0xc0

This is because prepare_bprm_creds grabs task->signal->cred_guard_mutex
and when do_lookup happens we try to grab task->signal->cred_guard_mutex
again in lock_trace.

Fix it using plain ptrace_may_access() helper, proc_map_files_lookup is
guarded by CAP_SYS_ADMIN.

Reported-by: Sasha Levin <levinsasha928@gmail.com>
Cc: Konstantin Khlebnikov <khlebnikov@openvz.org>
Cc: Pavel Emelyanov <xemul@openvz.org>
Cc: Dave Jones <davej@redhat.com>
Cc: Vasiliy Kulikov <segoon@openwall.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 fs/proc/base.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Index: linux-2.6.git/fs/proc/base.c
===================================================================
--- linux-2.6.git.orig/fs/proc/base.c
+++ linux-2.6.git/fs/proc/base.c
@@ -2226,16 +2226,16 @@ static struct dentry *proc_map_files_loo
 		goto out;
 
 	result = ERR_PTR(-EACCES);
-	if (lock_trace(task))
+	if (!ptrace_may_access(task, PTRACE_MODE_READ))
 		goto out_put_task;
 
 	result = ERR_PTR(-ENOENT);
 	if (dname_to_vma_addr(dentry, &vm_start, &vm_end))
-		goto out_unlock;
+		goto out_put_task;
 
 	mm = get_task_mm(task);
 	if (!mm)
-		goto out_unlock;
+		goto out_put_task;
 
 	down_read(&mm->mmap_sem);
 	vma = find_exact_vma(mm, vm_start, vm_end);
@@ -2247,8 +2247,6 @@ static struct dentry *proc_map_files_loo
 out_no_vma:
 	up_read(&mm->mmap_sem);
 	mmput(mm);
-out_unlock:
-	unlock_trace(task);
 out_put_task:
 	put_task_struct(task);
 out: