unionfs: Ensure SAVENAME is set for unionfs vnode operations

"rm-style" system calls such as kern_frmdirat() and kern_funlinkat()
don't supply SAVENAME to preserve the pathname buffer for subsequent
vnode ops.  For unionfs this poses an issue because the pathname may
be needed for a relookup operation in unionfs_remove()/unionfs_rmdir().
Currently unionfs doesn't check for this case, leading to a panic on
DIAGNOSTIC kernels and use-after-free of cn_nameptr otherwise.

The unionfs node's stored buffer would suffice as a replacement for
cnp->cn_nameptr in some (but not all) cases, but it's cleaner to just
ensure that unionfs vnode ops always have a valid cn_nameptr by setting
SAVENAME in unionfs_lookup().

While here, do some light cleanup in unionfs_lookup() and assert that
HASBUF is always present in the relevant relookup calls.

Reported by:	pho
Reviewed by:	markj
Differential Revision: https://reviews.freebsd.org/D32148

2 files changed, 24 insertions, 17 deletions

diff --git a/sys/fs/unionfs/union_subr.c b/sys/fs/unionfs/union_subr.c
index eb7681df645b..380a61819f1a 100644
--- a/sys/fs/unionfs/union_subr.c
+++ b/sys/fs/unionfs/union_subr.c
@@ -678,6 +678,8 @@ unionfs_relookup_for_create(struct vnode *dvp, struct componentname *cnp,
 	udvp = UNIONFSVPTOUPPERVP(dvp);
 	vp = NULLVP;
 
+	KASSERT((cnp->cn_flags & HASBUF) != 0,
+	    ("%s called without HASBUF", __func__));
 	error = unionfs_relookup(udvp, &vp, cnp, &cn, td, cnp->cn_nameptr,
 	    cnp->cn_namelen, CREATE);
 	if (error)
@@ -712,6 +714,8 @@ unionfs_relookup_for_delete(struct vnode *dvp, struct componentname *cnp,
 	udvp = UNIONFSVPTOUPPERVP(dvp);
 	vp = NULLVP;
 
+	KASSERT((cnp->cn_flags & HASBUF) != 0,
+	    ("%s called without HASBUF", __func__));
 	error = unionfs_relookup(udvp, &vp, cnp, &cn, td, cnp->cn_nameptr,
 	    cnp->cn_namelen, DELETE);
 	if (error)
@@ -746,6 +750,8 @@ unionfs_relookup_for_rename(struct vnode *dvp, struct componentname *cnp,
 	udvp = UNIONFSVPTOUPPERVP(dvp);
 	vp = NULLVP;
 
+	KASSERT((cnp->cn_flags & HASBUF) != 0,
+	    ("%s called without HASBUF", __func__));
 	error = unionfs_relookup(udvp, &vp, cnp, &cn, td, cnp->cn_nameptr,
 	    cnp->cn_namelen, RENAME);
 	if (error)
diff --git a/sys/fs/unionfs/union_vnops.c b/sys/fs/unionfs/union_vnops.c
index eb74192fd504..b6c088fb3804 100644
--- a/sys/fs/unionfs/union_vnops.c
+++ b/sys/fs/unionfs/union_vnops.c
@@ -167,9 +167,7 @@ unionfs_lookup(struct vop_cachedlookup_args *ap)
 		} else if (error == ENOENT && (cnflags & MAKEENTRY) != 0)
 			cache_enter(dvp, NULLVP, cnp);
 
-		UNIONFS_INTERNAL_DEBUG("unionfs_lookup: leave (%d)\n", error);
-
-		return (error);
+		goto unionfs_lookup_return;
 	}
 
 	/*
@@ -184,10 +182,8 @@ unionfs_lookup(struct vop_cachedlookup_args *ap)
 				*(ap->a_vpp) = dvp;
 				vref(dvp);
 
-				UNIONFS_INTERNAL_DEBUG(
-				    "unionfs_lookup: leave (%d)\n", uerror);
-
-				return (uerror);
+				error = uerror;
+				goto unionfs_lookup_return;
 			}
 			if (nameiop == DELETE || nameiop == RENAME ||
 			    (cnp->cn_lkflags & LK_TYPE_MASK))
@@ -246,9 +242,8 @@ unionfs_lookup(struct vop_cachedlookup_args *ap)
 	 * check lookup result
 	 */
 	if (uvp == NULLVP && lvp == NULLVP) {
-		UNIONFS_INTERNAL_DEBUG("unionfs_lookup: leave (%d)\n",
-		    (udvp != NULLVP ? uerror : lerror));
-		return (udvp != NULLVP ? uerror : lerror);
+		error = (udvp != NULLVP ? uerror : lerror);
+		goto unionfs_lookup_return;
 	}
 
 	/*
@@ -270,7 +265,7 @@ unionfs_lookup(struct vop_cachedlookup_args *ap)
 		error = unionfs_nodeget(dvp->v_mount, NULLVP, lvp, dvp, &vp,
 		    cnp, td);
 		if (error != 0)
-			goto unionfs_lookup_out;
+			goto unionfs_lookup_cleanup;
 
 		if (LK_SHARED == (cnp->cn_lkflags & LK_TYPE_MASK))
 			VOP_UNLOCK(vp);
@@ -289,7 +284,7 @@ unionfs_lookup(struct vop_cachedlookup_args *ap)
 				vput(vp);
 			else
 				vrele(vp);
-			goto unionfs_lookup_out;
+			goto unionfs_lookup_cleanup;
 		}
 		if ((cnp->cn_lkflags & LK_TYPE_MASK) == LK_SHARED)
 			vn_lock(vp, LK_SHARED | LK_RETRY);
@@ -303,7 +298,7 @@ unionfs_lookup(struct vop_cachedlookup_args *ap)
 		else
 			error = lerror;
 		if (error != 0)
-			goto unionfs_lookup_out;
+			goto unionfs_lookup_cleanup;
 		/*
 		 * get socket vnode.
 		 */
@@ -328,7 +323,7 @@ unionfs_lookup(struct vop_cachedlookup_args *ap)
 		if (error != 0) {
 			UNIONFSDEBUG(
 			    "unionfs_lookup: Unable to create unionfs vnode.");
-			goto unionfs_lookup_out;
+			goto unionfs_lookup_cleanup;
 		}
 		if ((nameiop == DELETE || nameiop == RENAME) &&
 		    (cnp->cn_lkflags & LK_TYPE_MASK) == 0)
@@ -340,7 +335,7 @@ unionfs_lookup(struct vop_cachedlookup_args *ap)
 	if ((cnflags & MAKEENTRY) && vp->v_type != VSOCK)
 		cache_enter(dvp, vp, cnp);
 
-unionfs_lookup_out:
+unionfs_lookup_cleanup:
 	if (uvp != NULLVP)
 		vrele(uvp);
 	if (lvp != NULLVP)
@@ -349,6 +344,12 @@ unionfs_lookup_out:
 	if (error == ENOENT && (cnflags & MAKEENTRY) != 0)
 		cache_enter(dvp, NULLVP, cnp);
 
+unionfs_lookup_return:
+
+	/* Ensure subsequent vnops will get a valid pathname buffer. */
+	if (nameiop != LOOKUP && (error == 0 || error == EJUSTRETURN))
+		cnp->cn_flags |= SAVENAME;
+
 	UNIONFS_INTERNAL_DEBUG("unionfs_lookup: leave (%d)\n", error);
 
 	return (error);
@@ -1372,7 +1373,7 @@ unionfs_mkdir(struct vop_mkdir_args *ap)
 			error = VOP_GETATTR(udvp, &va, cnp->cn_cred);
 			if (error != 0)
 				return (error);
-			if (va.va_flags & OPAQUE) 
+			if ((va.va_flags & OPAQUE) != 0)
 				cnp->cn_flags |= ISWHITEOUT;
 		}
 
@@ -1434,7 +1435,7 @@ unionfs_rmdir(struct vop_rmdir_args *ap)
 		if (ump->um_whitemode == UNIONFS_WHITE_ALWAYS || lvp != NULLVP)
 			cnp->cn_flags |= DOWHITEOUT;
 		error = unionfs_relookup_for_delete(ap->a_dvp, cnp, td);
-		if (!error)
+		if (error == 0)
 			error = VOP_RMDIR(udvp, uvp, cnp);
 	}
 	else if (lvp != NULLVP)