| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20120503173125.GF19347@moon>
Date: Thu, 3 May 2012 21:31:25 +0400
From: Cyrill Gorcunov <gorcunov@...nvz.org>
To: Sasha Levin <levinsasha928@...il.com>
Cc: khlebnikov@...nvz.org, xemul@...allels.com,
Dave Jones <davej@...hat.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Vasiliy Kulikov <segoon@...nwall.com>,
Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: c/r: broken locking when executing map_files
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@...nvz.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@...il.com>
Cc: Konstantin Khlebnikov <khlebnikov@...nvz.org>
Cc: Pavel Emelyanov <xemul@...nvz.org>
Cc: Dave Jones <davej@...hat.com>
Cc: Vasiliy Kulikov <segoon@...nwall.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@...nvz.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:
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists