nfscl: Fix a use after free in nfscl_cleanupkext()

ler@, markj@ reported a use after free in nfscl_cleanupkext().
They also provided two possible causes:
- In nfscl_cleanup_common(), "own" is the owner string
  owp->nfsow_owner.  If we free that particular
  owner structure, than in subsequent comparisons
  "own" will point to freed memory.
- nfscl_cleanup_common() can free more than one owner, so the use
  of LIST_FOREACH_SAFE() in nfscl_cleanupkext() is not sufficient.

I also believe there is a 3rd:
- If nfscl_freeopenowner() or nfscl_freelockowner() is called
  without the NFSCLSTATE mutex held, this could race with
  nfscl_cleanupkext().
  This could happen when the exclusive lock is held
  on the client, such as when delegations are being returned
  or when recovering from NFSERR_EXPIRED.

This patch fixes them as follows:
1 - Copy the owner string to a local variable before the
    nfscl_cleanup_common() call.
2 - Modify nfscl_cleanup_common() so that it will never free more
    than the first matching element.  Normally there should only
    be one element in each list with a matching open/lock owner
    anyhow (but there might be a bug that results in a duplicate).
    This should guarantee that the FOREACH_SAFE loops in
    nfscl_cleanupkext() are adequate.
3 - Acquire the NFSCLSTATE mutex in nfscl_freeopenowner()
    and nfscl_freelockowner(), if it is not already held.
    This serializes all of these calls with the ones done in
    nfscl_cleanup_common().

(cherry picked from commit 1cedb4ea1a790f976ec6211c938dfaa23874b497)

1 files changed, 42 insertions, 6 deletions

diff --git a/sys/fs/nfsclient/nfs_clstate.c b/sys/fs/nfsclient/nfs_clstate.c
index d6ed7970e805..da43c883cbe5 100644
--- a/sys/fs/nfsclient/nfs_clstate.c
+++ b/sys/fs/nfsclient/nfs_clstate.c
@@ -1559,8 +1559,22 @@ nfscl_expireopen(struct nfsclclient *clp, struct nfsclopen *op,
 static void
 nfscl_freeopenowner(struct nfsclowner *owp, int local)
 {
+	int owned;
 
+	/*
+	 * Make sure the NFSCLSTATE mutex is held, to avoid races with
+	 * calls in nfscl_renewthread() that do not hold a reference
+	 * count on the nfsclclient and just the mutex.
+	 * The mutex will not be held for calls done with the exclusive
+	 * nfsclclient lock held, in particular, nfscl_hasexpired()
+	 * and nfscl_recalldeleg() might do this.
+	 */
+	owned = mtx_owned(NFSCLSTATEMUTEXPTR);
+	if (owned == 0)
+		NFSLOCKCLSTATE();
 	LIST_REMOVE(owp, nfsow_list);
+	if (owned == 0)
+		NFSUNLOCKCLSTATE();
 	free(owp, M_NFSCLOWNER);
 	if (local)
 		nfsstatsv1.cllocalopenowners--;
@@ -1575,8 +1589,22 @@ void
 nfscl_freelockowner(struct nfscllockowner *lp, int local)
 {
 	struct nfscllock *lop, *nlop;
+	int owned;
 
+	/*
+	 * Make sure the NFSCLSTATE mutex is held, to avoid races with
+	 * calls in nfscl_renewthread() that do not hold a reference
+	 * count on the nfsclclient and just the mutex.
+	 * The mutex will not be held for calls done with the exclusive
+	 * nfsclclient lock held, in particular, nfscl_hasexpired()
+	 * and nfscl_recalldeleg() might do this.
+	 */
+	owned = mtx_owned(NFSCLSTATEMUTEXPTR);
+	if (owned == 0)
+		NFSLOCKCLSTATE();
 	LIST_REMOVE(lp, nfsl_list);
+	if (owned == 0)
+		NFSUNLOCKCLSTATE();
 	LIST_FOREACH_SAFE(lop, &lp->nfsl_lock, nfslo_list, nlop) {
 		nfscl_freelock(lop, local);
 	}
@@ -1762,16 +1790,17 @@ static void
 nfscl_cleanup_common(struct nfsclclient *clp, u_int8_t *own)
 {
 	struct nfsclowner *owp, *nowp;
-	struct nfscllockowner *lp, *nlp;
+	struct nfscllockowner *lp;
 	struct nfscldeleg *dp;
 
 	/* First, get rid of local locks on delegations. */
 	TAILQ_FOREACH(dp, &clp->nfsc_deleg, nfsdl_list) {
-		LIST_FOREACH_SAFE(lp, &dp->nfsdl_lock, nfsl_list, nlp) {
+		LIST_FOREACH(lp, &dp->nfsdl_lock, nfsl_list) {
 		    if (!NFSBCMP(lp->nfsl_owner, own, NFSV4CL_LOCKNAMELEN)) {
 			if ((lp->nfsl_rwlock.nfslock_lock & NFSV4LOCK_WANTED))
 			    panic("nfscllckw");
 			nfscl_freelockowner(lp, 1);
+			break;
 		    }
 		}
 	}
@@ -1790,6 +1819,7 @@ nfscl_cleanup_common(struct nfsclclient *clp, u_int8_t *own)
 				nfscl_freeopenowner(owp, 0);
 			else
 				owp->nfsow_defunct = 1;
+			break;
 		}
 		owp = nowp;
 	}
@@ -1805,6 +1835,7 @@ nfscl_cleanupkext(struct nfsclclient *clp, struct nfscllockownerfhhead *lhp)
 	struct nfsclopen *op;
 	struct nfscllockowner *lp, *nlp;
 	struct nfscldeleg *dp;
+	uint8_t own[NFSV4CL_LOCKNAMELEN];
 
 	NFSPROCLISTLOCK();
 	NFSLOCKCLSTATE();
@@ -1815,8 +1846,10 @@ nfscl_cleanupkext(struct nfsclclient *clp, struct nfscllockownerfhhead *lhp)
 					nfscl_emptylockowner(lp, lhp);
 			}
 		}
-		if (nfscl_procdoesntexist(owp->nfsow_owner))
-			nfscl_cleanup_common(clp, owp->nfsow_owner);
+		if (nfscl_procdoesntexist(owp->nfsow_owner)) {
+			memcpy(own, owp->nfsow_owner, NFSV4CL_LOCKNAMELEN);
+			nfscl_cleanup_common(clp, own);
+		}
 	}
 
 	/*
@@ -1828,8 +1861,11 @@ nfscl_cleanupkext(struct nfsclclient *clp, struct nfscllockownerfhhead *lhp)
 	 */
 	TAILQ_FOREACH(dp, &clp->nfsc_deleg, nfsdl_list) {
 		LIST_FOREACH_SAFE(lp, &dp->nfsdl_lock, nfsl_list, nlp) {
-			if (nfscl_procdoesntexist(lp->nfsl_owner))
-				nfscl_cleanup_common(clp, lp->nfsl_owner);
+			if (nfscl_procdoesntexist(lp->nfsl_owner)) {
+				memcpy(own, lp->nfsl_owner,
+				    NFSV4CL_LOCKNAMELEN);
+				nfscl_cleanup_common(clp, own);
+			}
 		}
 	}
 	NFSUNLOCKCLSTATE();