linux-kernel - Re: fs: NULL deref in atime_needs

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-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 28 Feb 2016 20:01:01 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Dmitry Vyukov <dvyukov@...gle.com>
Cc:	Ian Kent <raven@...maw.net>,
	Mickaël Salaün <mic@...ikod.net>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	syzkaller <syzkaller@...glegroups.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Sasha Levin <sasha.levin@...cle.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	David Howells <dhowells@...hat.com>
Subject: Re: fs: NULL deref in atime_needs_update

On Sun, Feb 28, 2016 at 05:01:34PM +0000, Al Viro wrote:

> Erm...  What's to order ->d_inode and ->d_flags fetches there?  David?
> Looks like the barrier in d_is_negative() is on the wrong side of fetch.
> Confused...

OK, as per David's suggestion, let's flip them around, bringing the
barrier in d_is_negative() between them.  Dmitry, could you try this on
top of mainline?  Again, it's until the first warning.

diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index c6d7d3d..86f81e3 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -323,6 +323,7 @@ static struct dentry *autofs4_mountpoint_changed(struct path *path)
 		struct dentry *new = d_lookup(parent, &dentry->d_name);
 		if (!new)
 			return NULL;
+		WARN_ON(d_is_negative(new));
 		ino = autofs4_dentry_ino(new);
 		ino->last_used = jiffies;
 		dput(path->dentry);
diff --git a/fs/namei.c b/fs/namei.c
index 9c590e0..630d222 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1209,6 +1209,7 @@ static int follow_managed(struct path *path, struct nameidata *nd)
 		/* Handle an automount point */
 		if (managed & DCACHE_NEED_AUTOMOUNT) {
 			ret = follow_automount(path, nd, &need_mntput);
+			WARN_ON(d_is_negative(path->dentry));
 			if (ret < 0)
 				break;
 			continue;
@@ -1260,6 +1261,7 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path,
 {
 	for (;;) {
 		struct mount *mounted;
+		void *p;
 		/*
 		 * Don't forget we might have a non-mountpoint managed dentry
 		 * that wants to block transit.
@@ -1289,7 +1291,9 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path,
 		 * dentry sequence number here after this d_inode read,
 		 * because a mount-point is always pinned.
 		 */
-		*inode = path->dentry->d_inode;
+		p = *inode = path->dentry->d_inode;
+		if (unlikely(!p))
+			WARN_ON(!read_seqretry(&mount_lock, nd->m_seq));
 	}
 	return !read_seqretry(&mount_lock, nd->m_seq) &&
 		!(path->dentry->d_flags & DCACHE_NEED_AUTOMOUNT);
@@ -1550,8 +1554,8 @@ static int lookup_fast(struct nameidata *nd,
 		 * This sequence count validates that the inode matches
 		 * the dentry name information from lookup.
 		 */
-		*inode = d_backing_inode(dentry);
 		negative = d_is_negative(dentry);
+		*inode = d_backing_inode(dentry);
 		if (read_seqcount_retry(&dentry->d_seq, seq))
 			return -ECHILD;
 
@@ -1580,6 +1584,7 @@ static int lookup_fast(struct nameidata *nd,
 		 */
 		if (negative)
 			return -ENOENT;
+		WARN_ON(!*inode);		// ->d_seq was fucked somehow
 		path->mnt = mnt;
 		path->dentry = dentry;
 		if (likely(__follow_mount_rcu(nd, path, inode, seqp)))
@@ -1613,8 +1618,10 @@ unlazy:
 	path->mnt = mnt;
 	path->dentry = dentry;
 	err = follow_managed(path, nd);
-	if (likely(!err))
+	if (likely(!err)) {
 		*inode = d_backing_inode(path->dentry);
+		WARN_ON(!*inode);
+	}
 	return err;
 
 need_lookup:
@@ -1717,6 +1724,7 @@ static inline int should_follow_link(struct nameidata *nd, struct path *link,
 		if (read_seqcount_retry(&link->dentry->d_seq, seq))
 			return -ECHILD;
 	}
+	WARN_ON(!inode);		// now, _that_ should not happen.
 	return pick_link(nd, link, inode, seq);
 }
 
@@ -3111,8 +3119,10 @@ static int do_last(struct nameidata *nd,
 			nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
 		/* we _can_ be in RCU mode here */
 		error = lookup_fast(nd, &path, &inode, &seq);
-		if (likely(!error))
+		if (likely(!error)) {
+			WARN_ON(!inode);
 			goto finish_lookup;
+		}
 
 		if (error < 0)
 			return error;
@@ -3203,6 +3213,7 @@ retry_lookup:
 		return -ENOENT;
 	}
 	inode = d_backing_inode(path.dentry);
+	WARN_ON(!inode);
 finish_lookup:
 	if (nd->depth)
 		put_link(nd);
diff --git a/fs/namespace.c b/fs/namespace.c
index 4fb1691..4128a5c 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1060,6 +1060,8 @@ static void cleanup_mnt(struct mount *mnt)
 	 * so mnt_get_writers() below is safe.
 	 */
 	WARN_ON(mnt_get_writers(mnt));
+	WARN_ON(!mnt->mnt.mnt_root->d_inode);	// some joker has managed to
+						// make mnt_root negative on us
 	if (unlikely(mnt->mnt_pins.first))
 		mnt_pin_kill(mnt);
 	fsnotify_vfsmount_delete(&mnt->mnt);