c/r: broken locking when executing map_files

c/r: broken locking when executing map_files

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

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

Re: c/r: broken locking when executing map_files

[Cyrill Gorcunov]

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

Re: c/r: broken locking when executing map_files

[Pavel Emelyanov]

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:

Re: c/r: broken locking when executing map_files

[Cyrill Gorcunov]

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:

Re: c/r: broken locking when executing map_files

[Vasiliy Kulikov]

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

Re: c/r: broken locking when executing map_files

[Cyrill Gorcunov]

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

Re: c/r: broken locking when executing map_files

[Cyrill Gorcunov]

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

Re: c/r: broken locking when executing map_files

[Vasiliy Kulikov]

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

Re: c/r: broken locking when executing map_files

[Cyrill Gorcunov]

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

Re: c/r: broken locking when executing map_files

[Vasiliy Kulikov]

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

Re: c/r: broken locking when executing map_files

[Cyrill Gorcunov]

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