From nobody Fri Mar 04 16:16:20 2022 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 9687119E3EB9; Fri, 4 Mar 2022 16:16:20 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4K9Cdw3pnZz3rrQ; Fri, 4 Mar 2022 16:16:20 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1646410580; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=cPv3Sg4Qpnie0WsPwKawhRw+5+pO+To0VXFxXlBr7Qo=; b=giHvWaKIpQhuICJEPNWToS/K+5MvEvTxTfxeEleNmhN12TmxHsjQprZZHBVrNSMk0eHkA1 wcD6X8qIh5ZZkC/yeOreEuBI/osenTzWYhqtOA1wCjnSQygXuRWldh3PD+gOayAVPSM1uH 5Ynr6kGRrTBG5FYpidHY+Nl8SXJxRj4+mryeU5h32+UIYctgxkeXlN+PkrgI1qVOHAiQr1 kZZsFlnsaYWwhE4Pzt5405714L59/ajzE38DmKHjZL8jSYa3rXA3DwFaA1Jut/odaU+Q23 wb80ULLM+EFnqwHGiUsylH9TcBiyHVh4WrAx0WZnfxuQXeG0PImrgSrfAbe6lg== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 62582751E; Fri, 4 Mar 2022 16:16:20 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 224GGKGB084697; Fri, 4 Mar 2022 16:16:20 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 224GGKCr084696; Fri, 4 Mar 2022 16:16:20 GMT (envelope-from git) Date: Fri, 4 Mar 2022 16:16:20 GMT Message-Id: <202203041616.224GGKCr084696@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Rick Macklem Subject: git: 22d6238a0473 - stable/12 - nfscl: Fix a use after free in nfscl_cleanupkext() List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: rmacklem X-Git-Repository: src X-Git-Refname: refs/heads/stable/12 X-Git-Reftype: branch X-Git-Commit: 22d6238a047311edf01df8b1b04bb2af3c99ae15 Auto-Submitted: auto-generated ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1646410580; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=cPv3Sg4Qpnie0WsPwKawhRw+5+pO+To0VXFxXlBr7Qo=; b=lSQfkILSCaotkG9F4wpCZlcaEDNGCBW8cv32UP+S7GDg5XXro79/obMS2T0oayEAq0sj5I f/Gimftfp+msCr2uGBJ3+McmNwVARgByl8dwQW6xuzv2/So1FsUDCZnKNlgcFPPEi9hlcj Ceo8xu9te3iyLvlB7BSYWt+ELy5bY5vSGv1uu2aq27foQ1dOACQU9SHgpSTbd+JNwoB7th SQRGepE3FHo/Id2jMKMVhUH/0guUkbqbccWIH/j6UXAKhPio7rvANTohxIhlHcRynN5WBw SNwgwjAopTKpEKGGZrOIRpiQxohgFykad0i7xdu2fGM26OHPaWQTOq/9HA9R0g== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1646410580; a=rsa-sha256; cv=none; b=NRN8xL2K6HdSJUNycioRxzqYBhvSeEwpIunGQyqZsLsISs5Q4yb6Cxkreer1day+o0lIoq VecWL1En6/HEXrwBEyaUTrvg1vKJGaj+FyanZEev+DY9/gTfa/Opnjx0EzbIcSzzHFlYDk eMAzLvBejC3r+ELDeid30a8bM1JkhgXnFHhJbiPhZcWdy3OaZmEF/0uqIecxWkxyVE6vsG dMOm1bOYebS4EDwM5VqeZC8+7q0Fv5HkBF376YQ5SewGwhWKdpTtBYYs9miW+XABtxIPsY nGvFXjQLc3JTr2HKoEkxKBSJy75Df9Y9KecD2tdIxkDKzSXl0UNgGm3Gbrvauw== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N The branch stable/12 has been updated by rmacklem: URL: https://cgit.FreeBSD.org/src/commit/?id=22d6238a047311edf01df8b1b04bb2af3c99ae15 commit 22d6238a047311edf01df8b1b04bb2af3c99ae15 Author: Rick Macklem AuthorDate: 2022-02-25 15:27:03 +0000 Commit: Rick Macklem CommitDate: 2022-03-04 16:14:56 +0000 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) --- sys/fs/nfsclient/nfs_clstate.c | 48 ++++++++++++++++++++++++++++++++++++------ 1 file 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();