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
On Wed, May 02, 2012 at 07:23:00PM +0200, Sasha Levin wrote:
> 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
On 05/02/2012 09:23 PM, Sasha Levin wrote:
> 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.
> 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:
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 <[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(-)
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:
Hi Cyrill,
On Thu, May 03, 2012 at 21:31 +0400, Cyrill Gorcunov wrote:
> 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).
...
> ---
> 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))
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,
--
Vasily Kulikov
http://www.openwall.com - bringing security into open computing environments
On Sat, May 05, 2012 at 10:20:51PM +0400, Vasiliy Kulikov wrote:
...
> > ---
> > 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))
>
> 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
On Sat, May 05, 2012 at 10:53:06PM +0400, Cyrill Gorcunov wrote:
> On Sat, May 05, 2012 at 10:20:51PM +0400, Vasiliy Kulikov wrote:
> ...
> > > ---
> > > 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))
> >
> > 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.
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
On Sat, May 05, 2012 at 23:32 +0400, Cyrill Gorcunov wrote:
> On Sat, May 05, 2012 at 10:53:06PM +0400, Cyrill Gorcunov wrote:
> > On Sat, May 05, 2012 at 10:20:51PM +0400, Vasiliy Kulikov wrote:
> > > > - 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.
>
> 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.
> | [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.
--
Vasily Kulikov
http://www.openwall.com - bringing security into open computing environments
On Mon, May 07, 2012 at 12:21:32AM +0400, Vasiliy Kulikov wrote:
> On Sat, May 05, 2012 at 23:32 +0400, Cyrill Gorcunov wrote:
> > On Sat, May 05, 2012 at 10:53:06PM +0400, Cyrill Gorcunov wrote:
> > > On Sat, May 05, 2012 at 10:20:51PM +0400, Vasiliy Kulikov wrote:
> > > > > - 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.
> >
> > 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.
>
> > | [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
On Mon, May 07, 2012 at 00:49 +0400, Cyrill Gorcunov wrote:
> On Mon, May 07, 2012 at 12:21:32AM +0400, Vasiliy Kulikov wrote:
> > On Sat, May 05, 2012 at 23:32 +0400, Cyrill Gorcunov wrote:
> > > 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?
> >
> > 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.
--
Vasily Kulikov
http://www.openwall.com - bringing security into open computing environments
On Fri, May 11, 2012 at 09:56:40PM +0400, Vasiliy Kulikov wrote:
> >
> > 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