Skip to contentSkip to conversation

c/r: broken locking when executing map_files

DISCUSSION Sasha Levin 4 participants 11 messages May 2, 2012 – May 11, 2012 Proc filesystem

lore
Share & tools
lore

Sasha Levin

author
#0 lore raw

Hi all,

I've stumbled on several lockdep warnings when playing with the new files created under /proc/[pid], specifically 'map_files'.

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.

First, if I try to simply execute one of the mappings:

sh-4.2# ls -al
total 0

dr-x------ 2 root 0  0 May  2 16:51 .
dr-xr-xr-x 9 root 0  0 May  2 16:51 ..
lr-------- 1 root 0 64 May  2 16:51 400000-4b3000 -> /host/bin/bash

[...]

sh-4.2# ./400000-4b3000

I get the following splat:

[  141.769863] 
[  141.770019] =============================================
[  141.770019] [ INFO: possible recursive locking detected ]
[  141.770019] 3.4.0-rc5-next-20120501-sasha #104 Tainted: G        W   
[  141.770019] ---------------------------------------------
[  141.770019] sh/4464 is trying to acquire lock:
[  141.770019]  (&sig->cred_guard_mutex){+.+.+.}, at: [<ffffffff810b518f>] mm_access+0x2f/0xb0
[  141.770019] 
[  141.770019] but task is already holding lock:
[  141.770019]  (&sig->cred_guard_mutex){+.+.+.}, at: [<ffffffff811f1742>] prepare_bprm_creds+0x32/0x80
[  141.770019] 
[  141.770019] other info that might help us debug this:
[  141.770019]  Possible unsafe locking scenario:
[  141.770019] 
[  141.770019]        CPU0
[  141.770019]        ----
[  141.770019]   lock(&sig->cred_guard_mutex);
[  141.770019]   lock(&sig->cred_guard_mutex);
[  141.770019] 
[  141.770019]  *** DEADLOCK ***
[  141.770019] 
[  141.770019]  May be due to missing lock nesting notation
[  141.770019] 
[  141.770019] 1 lock held by sh/4464:
[  141.770019]  #0:  (&sig->cred_guard_mutex){+.+.+.}, at: [<ffffffff811f1742>] prepare_bprm_creds+0x32/0x80
[  141.770019] 
[  141.770019] stack backtrace:
[  141.770019] Pid: 4464, comm: sh Tainted: G        W    3.4.0-rc5-next-20120501-sasha #104
[  141.770019] Call Trace:
[  141.770019]  [<ffffffff8111b1e9>] print_deadlock_bug+0x119/0x140
[  141.770019]  [<ffffffff8111d3de>] validate_chain+0x5ee/0x790
[  141.770019]  [<ffffffff810f1938>] ? sched_clock_cpu+0x108/0x120
[  141.770019]  [<ffffffff8111d9a3>] __lock_acquire+0x423/0x4c0
[  141.770019]  [<ffffffff8107d4c6>] ? kvm_clock_read+0x46/0x80
[  141.770019]  [<ffffffff8111db1c>] lock_acquire+0xdc/0x120
[  141.770019]  [<ffffffff810b518f>] ? mm_access+0x2f/0xb0
[  141.770019]  [<ffffffff82d8f5d0>] __mutex_lock_common+0x60/0x590
[  141.770019]  [<ffffffff810b518f>] ? mm_access+0x2f/0xb0
[  141.770019]  [<ffffffff810e808e>] ? sub_preempt_count+0xae/0xf0
[  141.770019]  [<ffffffff810b518f>] ? mm_access+0x2f/0xb0
[  141.770019]  [<ffffffff82d8fb90>] mutex_lock_killable_nested+0x40/0x50
[  141.770019]  [<ffffffff810b518f>] mm_access+0x2f/0xb0
[  141.770019]  [<ffffffff810d6f80>] ? alloc_pid+0x210/0x210
[  141.770019]  [<ffffffff81255124>] map_files_d_revalidate+0x74/0x250
[  141.770019]  [<ffffffff811f9175>] do_lookup+0x1d5/0x300
[  141.770019]  [<ffffffff811f95e0>] do_last+0x180/0x850
[  141.770019]  [<ffffffff811faa57>] path_openat+0xd7/0x4a0
[  141.770019]  [<ffffffff810562ad>] ? sched_clock+0x1d/0x30
[  141.770019]  [<ffffffff810f17c5>] ? sched_clock_local+0x25/0x90
[  141.770019]  [<ffffffff810f1938>] ? sched_clock_cpu+0x108/0x120
[  141.770019]  [<ffffffff811faf34>] do_filp_open+0x44/0xa0
[  141.770019]  [<ffffffff810e7951>] ? get_parent_ip+0x11/0x50
[  141.770019]  [<ffffffff810e808e>] ? sub_preempt_count+0xae/0xf0
[  141.770019]  [<ffffffff82d92bb0>] ? _raw_spin_unlock+0x30/0x60
[  141.770019]  [<ffffffff811f2fad>] open_exec+0x2d/0xf0
[  141.770019]  [<ffffffff811f403e>] do_execve_common+0xfe/0x320
[  141.770019]  [<ffffffff811f42e5>] do_execve+0x35/0x40
[  141.770019]  [<ffffffff81057a41>] sys_execve+0x51/0x80
[  141.770019]  [<ffffffff82d9407c>] stub_execve+0x6c/0xc0

And another (different) variant of this, is if you try listing directories combined with execves:

[  183.908022] ======================================================
[  183.908022] [ INFO: possible circular locking dependency detected ]
[  183.908022] 3.4.0-rc5-next-20120501-sasha #104 Tainted: G        W   
[  183.908022] -------------------------------------------------------
[  183.908022] trinity/21028 is trying to acquire lock:
[  183.908022]  (&sb->s_type->i_mutex_key#10){+.+.+.}, at: [<ffffffff811f921d>] do_lookup+0x27d/0x300
[  183.908022] 
[  183.908022] but task is already holding lock:
[  183.908022]  (&sig->cred_guard_mutex){+.+.+.}, at: [<ffffffff811f1742>] prepare_bprm_creds+0x32/0x80
[  183.908022] 
[  183.908022] which lock already depends on the new lock.
[  183.908022] 
[  183.908022] 
[  183.908022] the existing dependency chain (in reverse order) is:
[  183.908022] 
[  183.908022] -> #1 (&sig->cred_guard_mutex){+.+.+.}:
[  183.908022]        [<ffffffff8111d48e>] validate_chain+0x69e/0x790
[  183.908022]        [<ffffffff8111d9a3>] __lock_acquire+0x423/0x4c0
[  183.908022]        [<ffffffff8111db1c>] lock_acquire+0xdc/0x120
[  183.908022]        [<ffffffff82d8f5d0>] __mutex_lock_common+0x60/0x590
[  183.908022]        [<ffffffff82d8fb90>] mutex_lock_killable_nested+0x40/0x50
[  183.908022]        [<ffffffff81252a1f>] task_access_lock+0x2f/0x70
[  183.908022]        [<ffffffff81253592>] proc_map_files_readdir+0x82/0x470
[  183.943078]        [<ffffffff811ff53c>] vfs_readdir+0x7c/0xd0
[  183.943078]        [<ffffffff811ff712>] sys_getdents+0x92/0xf0
[  183.943078]        [<ffffffff82d93bb9>] system_call_fastpath+0x16/0x1b
[  183.943078] 
[  183.943078] -> #0 (&sb->s_type->i_mutex_key#10){+.+.+.}:
[  183.943078]        [<ffffffff8111ca3f>] check_prev_add+0x11f/0x4d0
[  183.943078]        [<ffffffff8111d48e>] validate_chain+0x69e/0x790
[  183.943078]        [<ffffffff8111d9a3>] __lock_acquire+0x423/0x4c0
[  183.943078]        [<ffffffff8111db1c>] lock_acquire+0xdc/0x120
[  183.943078]        [<ffffffff82d8f5d0>] __mutex_lock_common+0x60/0x590
[  183.943078]        [<ffffffff82d8fc30>] mutex_lock_nested+0x40/0x50
[  183.943078]        [<ffffffff811f921d>] do_lookup+0x27d/0x300
[  183.943078]        [<ffffffff811f95e0>] do_last+0x180/0x850
[  183.943078]        [<ffffffff811faa57>] path_openat+0xd7/0x4a0
[  183.943078]        [<ffffffff811faf34>] do_filp_open+0x44/0xa0
[  183.943078]        [<ffffffff811f2fad>] open_exec+0x2d/0xf0
[  183.943078]        [<ffffffff811f403e>] do_execve_common+0xfe/0x320
[  183.943078]        [<ffffffff811f42e5>] do_execve+0x35/0x40
[  183.943078]        [<ffffffff81057a41>] sys_execve+0x51/0x80
[  183.943078]        [<ffffffff82d9407c>] stub_execve+0x6c/0xc0
[  183.943078] 
[  183.943078] other info that might help us debug this:
[  183.943078] 
[  183.943078]  Possible unsafe locking scenario:
[  183.943078] 
[  183.943078]        CPU0                    CPU1
[  183.943078]        ----                    ----
[  183.943078]   lock(&sig->cred_guard_mutex);
[  183.943078]                                lock(&sb->s_type->i_mutex_key#10);
[  183.943078]                                lock(&sig->cred_guard_mutex);
[  183.943078]   lock(&sb->s_type->i_mutex_key#10);
[  183.943078] 
[  183.943078]  *** DEADLOCK ***
[  183.943078] 
[  183.943078] 1 lock held by trinity/21028:
[  183.943078]  #0:  (&sig->cred_guard_mutex){+.+.+.}, at: [<ffffffff811f1742>] prepare_bprm_creds+0x32/0x80
[  183.943078] 
[  183.943078] stack backtrace:
[  183.943078] Pid: 21028, comm: trinity Tainted: G        W    3.4.0-rc5-next-20120501-sasha #104
[  183.943078] Call Trace:
[  183.943078]  [<ffffffff8111b543>] print_circular_bug+0x103/0x120
[  183.943078]  [<ffffffff8111ca3f>] check_prev_add+0x11f/0x4d0
[  183.943078]  [<ffffffff8111d48e>] validate_chain+0x69e/0x790
[  183.943078]  [<ffffffff810f1938>] ? sched_clock_cpu+0x108/0x120
[  183.943078]  [<ffffffff8111d9a3>] __lock_acquire+0x423/0x4c0
[  183.943078]  [<ffffffff8111db1c>] lock_acquire+0xdc/0x120
[  183.943078]  [<ffffffff811f921d>] ? do_lookup+0x27d/0x300
[  183.943078]  [<ffffffff82d8f5d0>] __mutex_lock_common+0x60/0x590
[  183.943078]  [<ffffffff811f921d>] ? do_lookup+0x27d/0x300
[  183.943078]  [<ffffffff810e7951>] ? get_parent_ip+0x11/0x50
[  183.943078]  [<ffffffff810e808e>] ? sub_preempt_count+0xae/0xf0
[  183.943078]  [<ffffffff811f921d>] ? do_lookup+0x27d/0x300
[  183.943078]  [<ffffffff82d8fc30>] mutex_lock_nested+0x40/0x50
[  183.943078]  [<ffffffff811f921d>] do_lookup+0x27d/0x300
[  183.943078]  [<ffffffff811f774b>] ? inode_permission+0xfb/0x110
[  183.943078]  [<ffffffff811f95e0>] do_last+0x180/0x850
[  183.943078]  [<ffffffff811faa57>] path_openat+0xd7/0x4a0
[  183.943078]  [<ffffffff810562ad>] ? sched_clock+0x1d/0x30
[  183.943078]  [<ffffffff810f17c5>] ? sched_clock_local+0x25/0x90
[  183.943078]  [<ffffffff810f1938>] ? sched_clock_cpu+0x108/0x120
[  183.943078]  [<ffffffff811faf34>] do_filp_open+0x44/0xa0
[  183.943078]  [<ffffffff810e7951>] ? get_parent_ip+0x11/0x50
[  183.943078]  [<ffffffff810e808e>] ? sub_preempt_count+0xae/0xf0
[  183.943078]  [<ffffffff82d92bb0>] ? _raw_spin_unlock+0x30/0x60
[  183.943078]  [<ffffffff811f2fad>] open_exec+0x2d/0xf0
[  183.943078]  [<ffffffff811f403e>] do_execve_common+0xfe/0x320
[  183.943078]  [<ffffffff818914fb>] ? strncpy_from_user+0x8b/0x180
[  183.943078]  [<ffffffff811f42e5>] do_execve+0x35/0x40
[  183.943078]  [<ffffffff81057a41>] sys_execve+0x51/0x80
[  183.943078]  [<ffffffff82d9407c>] stub_execve+0x6c/0xc0
#1 lore raw
Quoted 9 lines · Sasha Levin

Hi all,

I've stumbled on several lockdep warnings when playing with the new files created under /proc/[pid], specifically 'map_files'.

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!

Cyrill

#2 lore raw
Quoted 7 lines · Cyrill Gorcunov
Quoted 5 lines

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 <[email protected]>
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 <[email protected]>
Cc Konstantin Khlebnikov <[email protected]>
Cc Pavel Emelyanov <[email protected]>
Cc Dave Jones <[email protected]>
Cc Vasiliy Kulikov <[email protected]>
Signed-off-by Cyrill Gorcunov <[email protected]>
---
 fs/proc/base.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
fs/proc/base.c +3 -5
Index: 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:
#3 lore raw

Hi Cyrill,

Quoted 11 lines · Cyrill Gorcunov
Quoted 7 lines · Cyrill Gorcunov
Quoted 5 lines

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

...

Quoted 9 lines
---
 fs/proc/base.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Index: linux-2.6.git/fs/proc/base.c
===================================================================

fs/proc/base.c static struct dentry *proc_map_files_loo · line 2229

Quoted 5 lines
fs/proc/base.c +1 -1
@@ -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))

Probably it will be better to change mutex_lock_killable() to mutex_lock_killable_nested() inside of lock_trace() instead of this change? It would keep the race-free check.

Thanks,

Signature · 2 lines
Vasily Kulikov
http://www.openwall.com - bringing security into open computing environments
#4 lore raw

On Sat, May 05, 2012 at 10:20:51PM +0400, Vasiliy Kulikov wrote:
...

Quoted 11 lines
Quoted 11 lines
---
 fs/proc/base.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Index: linux-2.6.git/fs/proc/base.c
===================================================================

fs/proc/base.c +0 -0
@@ -2226,16 +2226,16 @@ static struct dentry *proc_map_files_loo
 		goto out;
Quoted 7 lines

fs/proc/base.c static struct dentry *proc_map_files_loo · line 2229

Quoted 3 lines
fs/proc/base.c +1 -1
 	result = ERR_PTR(-EACCES);
-	if (lock_trace(task))
+	if (!ptrace_may_access(task, PTRACE_MODE_READ))

Probably it will be better to change mutex_lock_killable() to mutex_lock_killable_nested() inside of lock_trace() instead of this change? It would keep the race-free check.

Yup, if I'm not missing something SINGLE_DEPTH_NESTING should do the trick for us. I'll test and report.

Cyrill

#5 lore raw
Quoted 21 lines · Cyrill Gorcunov

On Sat, May 05, 2012 at 10:20:51PM +0400, Vasiliy Kulikov wrote:
...

Quoted 11 lines
Quoted 11 lines
---
 fs/proc/base.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Index: linux-2.6.git/fs/proc/base.c
===================================================================

fs/proc/base.c +0 -0
@@ -2226,16 +2226,16 @@ static struct dentry *proc_map_files_loo
 		goto out;
Quoted 7 lines

fs/proc/base.c static struct dentry *proc_map_files_loo · line 2229

Quoted 3 lines
fs/proc/base.c +1 -1
 	result = ERR_PTR(-EACCES);
-	if (lock_trace(task))
+	if (!ptrace_may_access(task, PTRACE_MODE_READ))

Probably it will be better to change mutex_lock_killable() to mutex_lock_killable_nested() inside of lock_trace() instead of this change? It would keep the race-free check.

Quoted 2 lines

Yup, if I'm not missing something SINGLE_DEPTH_NESTING should do the trick for us. I'll test and report.

Hmm, this doesn't work well, the mutex remanins killable so when one does

| [root@neptune ~]# /proc/self/map_files/400000-419000

it sleeps forever until killed, which is not good I think. Vasiliy, could you remind me what exactly is problem if we use unlocked ptrace_may_access here?

Cyrill

#6 lore raw
Quoted 9 lines · Cyrill Gorcunov
Quoted 8 lines · Cyrill Gorcunov
Quoted 6 lines · Vasiliy Kulikov

fs/proc/base.c static struct dentry *proc_map_files_loo · line 2229

Quoted 2 lines
fs/proc/base.c +1 -1
-	if (lock_trace(task))
+	if (!ptrace_may_access(task, PTRACE_MODE_READ))

Probably it will be better to change mutex_lock_killable() to mutex_lock_killable_nested() inside of lock_trace() instead of this change? It would keep the race-free check.

Quoted 4 lines
Quoted 2 lines

Yup, if I'm not missing something SINGLE_DEPTH_NESTING should do the trick for us. I'll test and report.

Hmm, this doesn't work well, the mutex remanins killable so when one does

Does it show circular locking? It shouldn't block if it uses mutex_lock+mutex_lock_nested.

Quoted 5 lines

| [root@neptune ~]# /proc/self/map_files/400000-419000

it sleeps forever until killed, which is not good I think. Vasiliy, could you remind me what exactly is problem if we use unlocked ptrace_may_access here?

There is a race between ptrace_may_access() and dname_to_vma_addr(). The target task may do exec() between these calls and dname_to_vma_addr() will be called against a privileged task. This may lead to a leak of task maps.

Signature · 2 lines
Vasily Kulikov
http://www.openwall.com - bringing security into open computing environments
#7 lore raw
Quoted 10 lines · Vasiliy Kulikov
Quoted 9 lines · Cyrill Gorcunov
Quoted 8 lines · Cyrill Gorcunov
Quoted 6 lines · Vasiliy Kulikov

fs/proc/base.c static struct dentry *proc_map_files_loo · line 2229

Quoted 2 lines
fs/proc/base.c +1 -1
-	if (lock_trace(task))
+	if (!ptrace_may_access(task, PTRACE_MODE_READ))

Probably it will be better to change mutex_lock_killable() to mutex_lock_killable_nested() inside of lock_trace() instead of this change? It would keep the race-free check.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Yup, if I'm not missing something SINGLE_DEPTH_NESTING should do the trick for us. I'll test and report.

Hmm, this doesn't work well, the mutex remanins killable so when one does

Does it show circular locking? It shouldn't block if it uses mutex_lock+mutex_lock_nested.

Nope of course, once mutex_lock_nested called the kernel doesn't complain anymore, but it rather sleep forever, until task killed, and that is wrong i think. moreover, since executing from inside of map_files is special one I think changing the whole lock_trace for this sake is a wrong approach.

Quoted 11 lines
Quoted 6 lines

| [root@neptune ~]# /proc/self/map_files/400000-419000

it sleeps forever until killed, which is not good I think. Vasiliy, could you remind me what exactly is problem if we use unlocked ptrace_may_access here?

There is a race between ptrace_may_access() and dname_to_vma_addr(). The target task may do exec() between these calls and dname_to_vma_addr() will be called against a privileged task. This may lead to a leak of task maps.

Wait, the proc_map_files_lookup requires the caller to be cap-sysadmin granted, would not this be enough?

Cyrill

#8 lore raw
Quoted 9 lines · Cyrill Gorcunov
Quoted 8 lines · Vasiliy Kulikov
Quoted 6 lines · Cyrill Gorcunov

On Sat, May 05, 2012 at 10:53:06PM +0400, Cyrill Gorcunov wrote:
| [root@neptune ~]# /proc/self/map_files/400000-419000

it sleeps forever until killed, which is not good I think. Vasiliy, could you remind me what exactly is problem if we use unlocked ptrace_may_access here?

Quoted 7 lines
Quoted 4 lines

There is a race between ptrace_may_access() and dname_to_vma_addr(). The target task may do exec() between these calls and dname_to_vma_addr() will be called against a privileged task. This may lead to a leak of task maps.

Wait, the proc_map_files_lookup requires the caller to be cap-sysadmin granted, would not this be enough?

Yes :-) You can also remove additional checks after capable() in readdir and revalidate functions.

Signature · 2 lines
Vasily Kulikov
http://www.openwall.com - bringing security into open computing environments
#9 lore raw
Quoted 6 lines · Vasiliy Kulikov
Quoted 3 lines

Wait, the proc_map_files_lookup requires the caller to be cap-sysadmin granted, would not this be enough?

Yes :-) You can also remove additional checks after capable() in readdir and revalidate functions.

Yup, I'll update. Thanks.

Cyrill

#10 lore raw
In reply to Sasha Levin
Quoted 5 lines · Sasha Levin

Hi all,

I've stumbled on several lockdep warnings when playing with the new files created under /proc/[pid], specifically 'map_files'.

My theory is that files under map_files shouldn't be executable,

These are symlinks and x bit on them doesn't mean anything, but anyway, thanks for catching this.

Quoted 5 lines

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.

First, if I try to simply execute one of the mappings: