linux-kernel - [PATCH] VFS: Add back check for !inode in walk

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150507125241.4da739ac@gandalf.local.home>
Date:	Thu, 7 May 2015 12:52:41 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Al Viro <viro@...IV.linux.org.uk>
Cc:	David Howells <dhowells@...hat.com>,
	LKML <linux-kernel@...r.kernel.org>,
	linux-fsdevel@...r.kernel.org
Subject: [PATCH] VFS: Add back check for !inode in walk_component()


Commit 698934df8b45 "VFS: Combine inode checks with d_is_negative() and
d_is_positive() in pathwalk" removed a check for inode being NULL in
walk_component() where the type is tested. Stressing my tracefs create
and remove instances while reading the files now triggers this:

BUG: unable to handle kernel NULL pointer dereference at 0000001c
IP: [<c05383a6>] inode_permission+0x2d/0x6c
*pdpt = 0000000030d9a001 *pde = 0000000000000000
Oops: 0000 [#1] SMP
Modules linked in: ip6t_REJECT nf_reject_ipv6 nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_filter ip6_tables ipv6 microcode ppdev parport_pc parport r8169
CPU: 0 PID: 3201 Comm: ftrace-test-mki Not tainted 4.1.0-rc2-test+ #94
Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6 02/22/2014
task: efa2ac20 ti: ed7a8000 task.ti: ed7a8000
EIP: 0060:[<c05383a6>] EFLAGS: 00010282 CPU: 0
EIP is at inode_permission+0x2d/0x6c
EAX: 00000001 EBX: 00000000 ECX: 00000006 EDX: 00000081
ESI: 00000000 EDI: ef92201b EBP: ed7a9e0c ESP: ed7a9df8
 DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
CR0: 80050033 CR2: 0000001c CR3: 31cb4c20 CR4: 001407f0
Stack:
 c0538379 c101c228 00000000 00000081 ed7a9ef8 ed7a9e64 c0538c7e c0538c33
 c1012e98 00000000 ed7a9ef8 00000000 00000006 efa2ac20 efa2ac20 e1919d01
 00000000 c04767e1 ed7a9e64 c05374fb f1392210 ee3c0240 00000000 c05391b5
Call Trace:
 [<c0538379>] ? __inode_permission+0x91/0x91
 [<c0538c7e>] link_path_walk+0x7a/0x3db
 [<c0538c33>] ? link_path_walk+0x2f/0x3db
 [<c04767e1>] ? trace_hardirqs_on+0xb/0xd
 [<c05374fb>] ? read_seqcount_begin+0x6a/0x77
 [<c05391b5>] ? path_init+0x1d6/0x326
 [<c05392f9>] path_init+0x31a/0x326
 [<c0538fdf>] ? link_path_walk+0x3db/0x3db
 [<c05312a3>] ? get_empty_filp+0x128/0x190
 [<c053ad56>] path_openat+0x1a3/0x3da
 [<c0408c1a>] ? native_sched_clock+0x46/0x4b
 [<c053bcdf>] do_filp_open+0x2e/0x6f
 [<c052f591>] do_sys_open+0x7c/0x108
 [<c052f558>] ? do_sys_open+0x43/0x108
 [<c0cc4f87>] ? sysenter_exit+0xf/0x16
 [<c052f63d>] SyS_open+0x20/0x22
 [<c0cc4f58>] sysenter_do_call+0x12/0x12
Code: e5 53 83 ec 10 3e 8d 74 26 00 89 c3 89 44 24 08 a1 d8 7b 25 c1 89 55 f8 c7 04 24 79 83 53 c0 89 44 24 04 e8 0e b0 f9 ff 8b 55 f8 <8b> 4b 1c f6 c2 02 74 2e f6 41 30 01 66 8b 03 74 25 25 00 f0 00
EIP: [<c05383a6>] inode_permission+0x2d/0x6c SS:ESP 0068:ed7a9df8
CR2: 000000000000001c
---[ end trace 54b6a3ccfbef84c6 ]---

Adding a bunch of debug I found that the race is the following:

  CPU1				CPU2
  ----				----
			    mkdir(foo)
			      d_instantiate(dentry, inode);
				spin_lock(inode->i_lock);
				  spin_lock(dentry->d_lock);
				    __d_set_inode_and_type();
link_path_walk()
  walk_component()
    lookup_fast(nd, path, &inode);
      *inode = path->d_entry->d_inode; (inode == NULL)

				      dentry->d_inode = inode;
				      smp_wmb();
				      dentry->d_flags = flags;

    if (d_is_negative(path->d_entry))
      [ fails ]

    [...]

    nd->inode = inode; (where inode is NULL);

Then in the next loop of link_path_walk()

  err = may_lookup(nd);
    inode_permission(nd->inode...)
      reference to nd->inode->i_sb (BOOM!)

Without this patch I can easily cause the bug, with this patch, I have
yet to trigger it.

Cc: David Howells <dhowells@...hat.com>
Signed-off-by: Steven Rostedt <rostedt@...dmis.org>
---
 fs/namei.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/namei.c b/fs/namei.c
index 4a8d998b7274..cdd066680de9 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1585,7 +1585,7 @@ static inline int walk_component(struct nameidata *nd, struct path *path,
 		inode = path->dentry->d_inode;
 	}
 	err = -ENOENT;
-	if (d_is_negative(path->dentry))
+	if (!inode || d_is_negative(path->dentry))
 		goto out_path_put;
 
 	if (should_follow_link(path->dentry, follow)) {
-- 
1.8.3.1

--
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/