Re: r289932 causes pf reversion - breaks rules with broadcast destination

On 2015-11-05 12:39:22 (-0500), Tom Uffner <tom_at_uffner.com> 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
_at__at_ -1973,9 +1973,9 _at__at_ 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