From owner-dev-commits-src-all@freebsd.org Fri Aug 13 11:16:19 2021 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 4CDE0654979; Fri, 13 Aug 2021 11:16:19 +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 4GmLbR1QZzz3hLR; Fri, 13 Aug 2021 11:16:19 +0000 (UTC) (envelope-from git@FreeBSD.org) 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 18F535332; Fri, 13 Aug 2021 11:16:19 +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 17DBGJ1I074066; Fri, 13 Aug 2021 11:16:19 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 17DBGI5Q074065; Fri, 13 Aug 2021 11:16:18 GMT (envelope-from git) Date: Fri, 13 Aug 2021 11:16:18 GMT Message-Id: <202108131116.17DBGI5Q074065@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Wojciech Macek Subject: git: 4920e38fecc3 - main - ipsec: fix race condition in key.c MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: wma X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 4920e38fecc3d0274b03ae7151153e9d6b9eb526 Auto-Submitted: auto-generated X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Aug 2021 11:16:19 -0000 The branch main has been updated by wma: URL: https://cgit.FreeBSD.org/src/commit/?id=4920e38fecc3d0274b03ae7151153e9d6b9eb526 commit 4920e38fecc3d0274b03ae7151153e9d6b9eb526 Author: Wojciech Macek AuthorDate: 2021-08-13 10:52:38 +0000 Commit: Wojciech Macek CommitDate: 2021-08-13 10:52:38 +0000 ipsec: fix race condition in key.c Small patch that fixes a race condition in sys/netipsec/key.c Obtained from: Stormshield Differential revision: https://reviews.freebsd.org/D31271 --- sys/netipsec/key.c | 53 +++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 16 deletions(-) diff --git a/sys/netipsec/key.c b/sys/netipsec/key.c index 3ea3a81b90c3..72c598586d8e 100644 --- a/sys/netipsec/key.c +++ b/sys/netipsec/key.c @@ -616,6 +616,7 @@ static struct callout key_timer; #endif static void key_unlink(struct secpolicy *); +static void key_detach(struct secpolicy *); static struct secpolicy *key_do_allocsp(struct secpolicyindex *spidx, u_int dir); static struct secpolicy *key_getsp(struct secpolicyindex *); static struct secpolicy *key_getspbyid(u_int32_t); @@ -1200,18 +1201,26 @@ key_freesp(struct secpolicy **spp) static void key_unlink(struct secpolicy *sp) { + SPTREE_WLOCK(); + key_detach(sp); + SPTREE_WUNLOCK(); + if (SPDCACHE_ENABLED()) + spdcache_clear(); + key_freesp(&sp); +} +static void +key_detach(struct secpolicy *sp) +{ IPSEC_ASSERT(sp->spidx.dir == IPSEC_DIR_INBOUND || sp->spidx.dir == IPSEC_DIR_OUTBOUND, ("invalid direction %u", sp->spidx.dir)); - SPTREE_UNLOCK_ASSERT(); + SPTREE_WLOCK_ASSERT(); KEYDBG(KEY_STAMP, printf("%s: SP(%p)\n", __func__, sp)); - SPTREE_WLOCK(); if (sp->state != IPSEC_SPSTATE_ALIVE) { /* SP is already unlinked */ - SPTREE_WUNLOCK(); return; } sp->state = IPSEC_SPSTATE_DEAD; @@ -1219,10 +1228,6 @@ key_unlink(struct secpolicy *sp) V_spd_size--; LIST_REMOVE(sp, idhash); V_sp_genid++; - SPTREE_WUNLOCK(); - if (SPDCACHE_ENABLED()) - spdcache_clear(); - key_freesp(&sp); } /* @@ -1941,7 +1946,7 @@ key_spdadd(struct socket *so, struct mbuf *m, const struct sadb_msghdr *mhp) struct sadb_address *src0, *dst0; struct sadb_x_policy *xpl0, *xpl; struct sadb_lifetime *lft = NULL; - struct secpolicy *newsp; + struct secpolicy *newsp, *oldsp; int error; IPSEC_ASSERT(so != NULL, ("null socket")); @@ -2019,15 +2024,13 @@ key_spdadd(struct socket *so, struct mbuf *m, const struct sadb_msghdr *mhp) src0->sadb_address_proto, &spidx); /* Checking there is SP already or not. */ - newsp = key_getsp(&spidx); - if (newsp != NULL) { + oldsp = key_getsp(&spidx); + if (oldsp != NULL) { if (mhp->msg->sadb_msg_type == SADB_X_SPDUPDATE) { KEYDBG(KEY_STAMP, printf("%s: unlink SP(%p) for SPDUPDATE\n", - __func__, newsp)); - KEYDBG(KEY_DATA, kdebug_secpolicy(newsp)); - key_unlink(newsp); - key_freesp(&newsp); + __func__, oldsp)); + KEYDBG(KEY_DATA, kdebug_secpolicy(oldsp)); } else { key_freesp(&newsp); ipseclog((LOG_DEBUG, @@ -2038,6 +2041,10 @@ key_spdadd(struct socket *so, struct mbuf *m, const struct sadb_msghdr *mhp) /* allocate new SP entry */ if ((newsp = key_msg2sp(xpl0, PFKEY_EXTLEN(xpl0), &error)) == NULL) { + if (oldsp != NULL) { + key_unlink(oldsp); + key_freesp(&oldsp); /* second for our reference */ + } return key_senderror(so, m, error); } @@ -2046,18 +2053,32 @@ key_spdadd(struct socket *so, struct mbuf *m, const struct sadb_msghdr *mhp) newsp->validtime = lft ? lft->sadb_lifetime_usetime : 0; bcopy(&spidx, &newsp->spidx, sizeof(spidx)); - /* XXXAE: there is race between key_getsp() and key_insertsp() */ SPTREE_WLOCK(); if ((newsp->id = key_getnewspid()) == 0) { + if (oldsp != NULL) + key_detach(oldsp); SPTREE_WUNLOCK(); + if (oldsp != NULL) { + key_freesp(&oldsp); /* first for key_detach */ + IPSEC_ASSERT(oldsp != NULL, ("null oldsp: refcount bug")); + key_freesp(&oldsp); /* second for our reference */ + if (SPDCACHE_ENABLED()) /* refresh cache because of key_detach */ + spdcache_clear(); + } key_freesp(&newsp); return key_senderror(so, m, ENOBUFS); } + if (oldsp != NULL) + key_detach(oldsp); key_insertsp(newsp); SPTREE_WUNLOCK(); + if (oldsp != NULL) { + key_freesp(&oldsp); /* first for key_detach */ + IPSEC_ASSERT(oldsp != NULL, ("null oldsp: refcount bug")); + key_freesp(&oldsp); /* second for our reference */ + } if (SPDCACHE_ENABLED()) spdcache_clear(); - KEYDBG(KEY_STAMP, printf("%s: SP(%p)\n", __func__, newsp)); KEYDBG(KEY_DATA, kdebug_secpolicy(newsp));