CVE-2016-6197: Improper Input Validation (CWE-20)
value: fs/overlayfs/dir.c in the OverlayFS filesystem implementation in the Linux kernel before 4.6 does not properly verify the upper dentry before proceeding with unlink and rename system-call processing, which allows local users to cause a denial of service (system crash) via a rename system call that specifies a self-hardlink.
CWE description: The product receives input or data, but it does
        not validate or incorrectly validates that the input has the
        properties that are required to process the data safely and
        correctly.
CVSS v3 base score: 5.5
Published: 2016-08-06T20:59Z
Repository: https://github.com/torvalds/linux
Language: C
Files changed: fs/overlayfs/dir.c
Commit date: 2016-03-21 17:31:44 +0100
Commit message: ovl: verify upper dentry before unlink and rename

Unlink and rename in overlayfs checked the upper dentry for staleness by
verifying upper->d_parent against upperdir.  However the dentry can go
stale also by being unhashed, for example.

Expand the verification to actually look up the name again (under parent
lock) and check if it matches the upper dentry.  This matches what the VFS
does before passing the dentry to filesytem's unlink/rename methods, which
excludes any inconsistency caused by overlayfs.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>

Diff: diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 52f6de5d40a92..bc1758a160f6d 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -594,34 +594,39 @@ static int ovl_remove_and_whiteout(struct dentry *dentry, bool is_dir)
 
 static int ovl_remove_upper(struct dentry *dentry, bool is_dir)
 {
 	struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
 	struct inode *dir = upperdir->d_inode;
-	struct dentry *upper = ovl_dentry_upper(dentry);
+	struct dentry *upper;
 	int err;
 
 	inode_lock_nested(dir, I_MUTEX_PARENT);
+	upper = lookup_one_len(dentry->d_name.name, upperdir,
+			       dentry->d_name.len);
+	err = PTR_ERR(upper);
+	if (IS_ERR(upper))
+		goto out_unlock;
+
 	err = -ESTALE;
-	if (upper->d_parent == upperdir) {
-		/* Don't let d_delete() think it can reset d_inode */
-		dget(upper);
+	if (upper == ovl_dentry_upper(dentry)) {
 		if (is_dir)
 			err = vfs_rmdir(dir, upper);
 		else
 			err = vfs_unlink(dir, upper, NULL);
-		dput(upper);
 		ovl_dentry_version_inc(dentry->d_parent);
 	}
+	dput(upper);
 
 	/*
 	 * Keeping this dentry hashed would mean having to release
 	 * upperpath/lowerpath, which could only be done if we are the
 	 * sole user of this dentry.  Too tricky...  Just unhash for
 	 * now.
 	 */
 	if (!err)
 		d_drop(dentry);
+out_unlock:
 	inode_unlock(dir);
 
 	return err;
 }
 
@@ -838,33 +843,43 @@ static int ovl_rename2(struct inode *olddir, struct dentry *old,
 	old_upperdir = ovl_dentry_upper(old->d_parent);
 	new_upperdir = ovl_dentry_upper(new->d_parent);
 
 	trap = lock_rename(new_upperdir, old_upperdir);
 
-	olddentry = ovl_dentry_upper(old);
-	newdentry = ovl_dentry_upper(new);
-	if (newdentry) {
+
+	olddentry = lookup_one_len(old->d_name.name, old_upperdir,
+				   old->d_name.len);
+	err = PTR_ERR(olddentry);
+	if (IS_ERR(olddentry))
+		goto out_unlock;
+
+	err = -ESTALE;
+	if (olddentry != ovl_dentry_upper(old))
+		goto out_dput_old;
+
+	newdentry = lookup_one_len(new->d_name.name, new_upperdir,
+				   new->d_name.len);
+	err = PTR_ERR(newdentry);
+	if (IS_ERR(newdentry))
+		goto out_dput_old;
+
+	err = -ESTALE;
+	if (ovl_dentry_upper(new)) {
 		if (opaquedir) {
-			newdentry = opaquedir;
-			opaquedir = NULL;
+			if (newdentry != opaquedir)
+				goto out_dput;
 		} else {
-			dget(newdentry);
+			if (newdentry != ovl_dentry_upper(new))
+				goto out_dput;
 		}
 	} else {
 		new_create = true;
-		newdentry = lookup_one_len(new->d_name.name, new_upperdir,
-					   new->d_name.len);
-		err = PTR_ERR(newdentry);
-		if (IS_ERR(newdentry))
-			goto out_unlock;
+		if (!d_is_negative(newdentry) &&
+		    (!new_opaque || !ovl_is_whiteout(newdentry)))
+			goto out_dput;
 	}
 
-	err = -ESTALE;
-	if (olddentry->d_parent != old_upperdir)
-		goto out_dput;
-	if (newdentry->d_parent != new_upperdir)
-		goto out_dput;
 	if (olddentry == trap)
 		goto out_dput;
 	if (newdentry == trap)
 		goto out_dput;
 
@@ -923,10 +938,12 @@ static int ovl_rename2(struct inode *olddir, struct dentry *old,
 	ovl_dentry_version_inc(old->d_parent);
 	ovl_dentry_version_inc(new->d_parent);
 
 out_dput:
 	dput(newdentry);
+out_dput_old:
+	dput(olddentry);
 out_unlock:
 	unlock_rename(new_upperdir, old_upperdir);
 out_revert_creds:
 	if (old_opaque || new_opaque) {
 		revert_creds(old_cred);

Vulnerable code: 	struct dentry *upper = ovl_dentry_upper(dentry);
	if (upper->d_parent == upperdir) {
		/* Don't let d_delete() think it can reset d_inode */
		dget(upper);
		dput(upper);
	olddentry = ovl_dentry_upper(old);
	newdentry = ovl_dentry_upper(new);
	if (newdentry) {
			newdentry = opaquedir;
			opaquedir = NULL;
			dget(newdentry);
		newdentry = lookup_one_len(new->d_name.name, new_upperdir,
					   new->d_name.len);
		err = PTR_ERR(newdentry);
		if (IS_ERR(newdentry))
			goto out_unlock;
	err = -ESTALE;
	if (olddentry->d_parent != old_upperdir)
		goto out_dput;
	if (newdentry->d_parent != new_upperdir)
		goto out_dput;
Fixed code: 	struct dentry *upper;
	upper = lookup_one_len(dentry->d_name.name, upperdir,
			       dentry->d_name.len);
	err = PTR_ERR(upper);
	if (IS_ERR(upper))
		goto out_unlock;

	if (upper == ovl_dentry_upper(dentry)) {
	dput(upper);
out_unlock:

	olddentry = lookup_one_len(old->d_name.name, old_upperdir,
				   old->d_name.len);
	err = PTR_ERR(olddentry);
	if (IS_ERR(olddentry))
		goto out_unlock;

	err = -ESTALE;
	if (olddentry != ovl_dentry_upper(old))
		goto out_dput_old;

	newdentry = lookup_one_len(new->d_name.name, new_upperdir,
				   new->d_name.len);
	err = PTR_ERR(newdentry);
	if (IS_ERR(newdentry))
		goto out_dput_old;

	err = -ESTALE;
	if (ovl_dentry_upper(new)) {
			if (newdentry != opaquedir)
				goto out_dput;
			if (newdentry != ovl_dentry_upper(new))
				goto out_dput;
		if (!d_is_negative(newdentry) &&
		    (!new_opaque || !ovl_is_whiteout(newdentry)))
			goto out_dput;
out_dput_old:
	dput(olddentry);