From owner-freebsd-current@freebsd.org Fri Nov 6 16:06:14 2015 Return-Path: Delivered-To: freebsd-current@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 B80D3A28048 for ; Fri, 6 Nov 2015 16:06:14 +0000 (UTC) (envelope-from kp@vega.codepro.be) Received: from venus.codepro.be (venus.codepro.be [IPv6:2a01:4f8:162:1127::2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.codepro.be", Issuer "Gandi Standard SSL CA 2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 7EBC41F36 for ; Fri, 6 Nov 2015 16:06:14 +0000 (UTC) (envelope-from kp@vega.codepro.be) Received: from vega.codepro.be (unknown [172.16.1.3]) by venus.codepro.be (Postfix) with ESMTP id D299B203B5; Fri, 6 Nov 2015 17:06:10 +0100 (CET) Received: by vega.codepro.be (Postfix, from userid 1001) id CBF774E521; Fri, 6 Nov 2015 17:06:10 +0100 (CET) Date: Fri, 6 Nov 2015 17:06:10 +0100 From: Kristof Provost To: Tom Uffner Cc: FreeBSD-Current Subject: Re: r289932 causes pf reversion - breaks rules with broadcast destination Message-ID: <20151106160610.GB2336@vega.codepro.be> References: <563AB177.6030809@uffner.com> <563B944A.50905@uffner.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <563B944A.50905@uffner.com> X-Checked-By-NSA: Probably User-Agent: Mutt/1.5.24 (2015-08-30) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Nov 2015 16:06:14 -0000 On 2015-11-05 12:39:22 (-0500), Tom Uffner wrote: > So, if my rule was "working" due to false positive in a comparison that has > now been fixed, how many other address comparisons were affected by this > error? > > There are 36 occurrences of PF_ANEQ in pf.c and 2 in if_pfsync.c > Most of them are an optimisation check. They're used in the NAT paths to see if addresses need to be rewritten (and checksums updated) or not. That's probably part of the reason it took so long to notice the bug in the macro: in most cases a false positive only slowed things down a little, it didn't actually produce an incorrect result. I think I've reproduced your problem with very simple rules: pass out block out proto icmp pass out log on vtnet0 proto icmp from any to vtnet0:broadcast pass out log on vtnet0 proto icmp from any to 172.16.2.1 With those rules I can ping to ping 172.16.2.255 (vtnet0 has 172.16.2.2/24), but not to 172.16.2.1. If I remove the broadcast rule I suddenly can ping to 172.16.2.1. I suspect I've also found the source of the problem: pf_addr_wrap_neq() uses PF_ANEQ(), but sets address family 0. As a result of the fix that now means we always return false there. Can you give this a quick test: diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c index 1dfc37d..762b82e 100644 --- a/sys/netpfil/pf/pf.c +++ b/sys/netpfil/pf/pf.c @@ -1973,9 +1973,9 @@ pf_addr_wrap_neq(struct pf_addr_wrap *aw1, struct pf_addr_wrap *aw2) switch (aw1->type) { case PF_ADDR_ADDRMASK: case PF_ADDR_RANGE: - if (PF_ANEQ(&aw1->v.a.addr, &aw2->v.a.addr, 0)) + if (PF_ANEQ(&aw1->v.a.addr, &aw2->v.a.addr, AF_INET6)) return (1); - if (PF_ANEQ(&aw1->v.a.mask, &aw2->v.a.mask, 0)) + if (PF_ANEQ(&aw1->v.a.mask, &aw2->v.a.mask, AF_INET6)) return (1); return (0); case PF_ADDR_DYNIFTL: Regards, Kristof