Date: Wed, 7 Dec 2016 13:08:24 -0800 From: Gleb Smirnoff <glebius@FreeBSD.org> To: Marcel Moolenaar <marcel@FreeBSD.org> 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> In-Reply-To: <201612020615.uB26Fxs1049431@repo.freebsd.org> References: <201612020615.uB26Fxs1049431@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20161207210824.GN27748>