From owner-svn-src-all@freebsd.org Wed Dec 7 21:08:27 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 067FFC6B9F1; Wed, 7 Dec 2016 21:08:27 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebi.us (glebi.us [96.95.210.25]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "cell.glebi.us", Issuer "cell.glebi.us" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id D71321FB2; Wed, 7 Dec 2016 21:08:26 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebi.us (localhost [127.0.0.1]) by cell.glebi.us (8.15.2/8.15.2) with ESMTPS id uB7L8PJS021523 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Wed, 7 Dec 2016 13:08:25 -0800 (PST) (envelope-from glebius@FreeBSD.org) Received: (from glebius@localhost) by cell.glebi.us (8.15.2/8.15.2/Submit) id uB7L8Ocv021522; Wed, 7 Dec 2016 13:08:24 -0800 (PST) (envelope-from glebius@FreeBSD.org) X-Authentication-Warning: cell.glebi.us: glebius set sender to glebius@FreeBSD.org using -f Date: Wed, 7 Dec 2016 13:08:24 -0800 From: Gleb Smirnoff To: Marcel Moolenaar Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r309394 - head/sys/netpfil/pf Message-ID: <20161207210824.GN27748@FreeBSD.org> References: <201612020615.uB26Fxs1049431@repo.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201612020615.uB26Fxs1049431@repo.freebsd.org> User-Agent: Mutt/1.7.0 (2016-08-17) X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 07 Dec 2016 21:08:27 -0000 Marcel, thanks for the fixes. While the problem with the first chunk in pfsync_sendout() is obvious, the problem you are fixing in th second chunk in the pfsync_delete_state() is not clear to me. Can you please explain what scenario are you fixing there? On Fri, Dec 02, 2016 at 06:15:59AM +0000, Marcel Moolenaar wrote: M> Author: marcel M> Date: Fri Dec 2 06:15:59 2016 M> New Revision: 309394 M> URL: https://svnweb.freebsd.org/changeset/base/309394 M> M> Log: M> Fix use-after-free bugs in pfsync(4) M> M> Use after free happens for state that is deleted. The reference M> count is what prevents the state from being freed. When the M> state is dequeued, the reference count is dropped and the memory M> freed. We can't dereference the next pointer or re-queue the M> state. M> M> MFC after: 1 week M> Differential Revision: https://reviews.freebsd.org/D8671 M> M> Modified: M> head/sys/netpfil/pf/if_pfsync.c M> M> Modified: head/sys/netpfil/pf/if_pfsync.c M> ============================================================================== M> --- head/sys/netpfil/pf/if_pfsync.c Fri Dec 2 06:07:27 2016 (r309393) M> +++ head/sys/netpfil/pf/if_pfsync.c Fri Dec 2 06:15:59 2016 (r309394) M> @@ -1509,7 +1509,7 @@ pfsync_sendout(int schedswi) M> struct ip *ip; M> struct pfsync_header *ph; M> struct pfsync_subheader *subh; M> - struct pf_state *st; M> + struct pf_state *st, *st_next; M> struct pfsync_upd_req_item *ur; M> int offset; M> int q, count = 0; M> @@ -1559,7 +1559,7 @@ pfsync_sendout(int schedswi) M> offset += sizeof(*subh); M> M> count = 0; M> - TAILQ_FOREACH(st, &sc->sc_qs[q], sync_list) { M> + TAILQ_FOREACH_SAFE(st, &sc->sc_qs[q], sync_list, st_next) { M> KASSERT(st->sync_state == q, M> ("%s: st->sync_state == q", M> __func__)); M> @@ -1931,6 +1931,8 @@ pfsync_delete_state(struct pf_state *st) M> if (sc->sc_len == PFSYNC_MINPKT) M> callout_reset(&sc->sc_tmo, 1 * hz, pfsync_timeout, V_pfsyncif); M> M> + pf_ref_state(st); M> + M> switch (st->sync_state) { M> case PFSYNC_S_INS: M> /* We never got to tell the world so just forget about it. */ M> @@ -1950,6 +1952,9 @@ pfsync_delete_state(struct pf_state *st) M> default: M> panic("%s: unexpected sync state %d", __func__, st->sync_state); M> } M> + M> + pf_release_state(st); M> + M> PFSYNC_UNLOCK(sc); M> } M> M> _______________________________________________ M> svn-src-all@freebsd.org mailing list M> https://lists.freebsd.org/mailman/listinfo/svn-src-all M> To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" -- Totus tuus, Glebius.