Bug was reported by Chriss Cappucio. It has turned out my earlier change

to pf_lb.c was not complete. We must add a test to determine number of
addresses defined by pool, so we don't treat pool definition
172.16.0.0/16 as a single IP address in pool. If pool is defined as
172.16.0.0/16, then we don't want to fall back to PF_POOL_NONE. Missing
this measure in pf_map_addr() may cause pf_get_sport() to enter infinite
loop when source ports translation become depleted for the first address
found in pool (like 172.16.0.1), because the bug prevents pf_map_addr()
to move to next address in pool (like 172.16.0.2).

while investigating issue I've also noticed an oddity for small random
pools such as 192.168.1.32/28. One would expect the addresses for nat
will be randomly picked from range .32 - .47 in this case. however the
random selection yield significantly more (like 20%) addresses ending by .32
In order to fix it we make random pool to use arc4random_uniform(~mask + 1)
instead of current arc4random().

feedback by claudio@
tested by hrvoje@

diff --git a/sys/net/pf_lb.c b/sys/net/pf_lb.c
index d106073..588115c 100644 (file)
--- a/sys/net/pf_lb.c
+++ b/sys/net/pf_lb.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: pf_lb.c,v 1.70 2022/02/16 08:46:11 sashan Exp $ */
+/*     $OpenBSD: pf_lb.c,v 1.71 2022/08/03 08:16:04 sashan Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -344,6 +344,17 @@ pf_map_addr_sticky(sa_family_t af, struct pf_rule *r, struct pf_addr *saddr,
        return (0);
 }
 
+uint32_t
+pf_rand_addr(uint32_t mask)
+{
+       uint32_t addr;
+
+       mask = ~ntohl(mask);
+       addr = arc4random_uniform(mask + 1);
+
+       return (htonl(addr));
+}
+
 int
 pf_map_addr(sa_family_t af, struct pf_rule *r, struct pf_addr *saddr,
     struct pf_addr *naddr, struct pf_addr *init_addr, struct pf_src_node **sns,
@@ -428,24 +439,29 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct pf_addr *saddr,
                } else if (init_addr != NULL && PF_AZERO(init_addr, af)) {
                        switch (af) {
                        case AF_INET:
-                               rpool->counter.addr32[0] = arc4random();
+                               rpool->counter.addr32[0] = pf_rand_addr(
+                                   rmask->addr32[0]);
                                break;
 #ifdef INET6
                        case AF_INET6:
                                if (rmask->addr32[3] != 0xffffffff)
-                                       rpool->counter.addr32[3] = arc4random();
+                                       rpool->counter.addr32[3] = pf_rand_addr(
+                                           rmask->addr32[3]);
                                else
                                        break;
                                if (rmask->addr32[2] != 0xffffffff)
-                                       rpool->counter.addr32[2] = arc4random();
+                                       rpool->counter.addr32[2] = pf_rand_addr(
+                                           rmask->addr32[2]);
                                else
                                        break;
                                if (rmask->addr32[1] != 0xffffffff)
-                                       rpool->counter.addr32[1] = arc4random();
+                                       rpool->counter.addr32[1] = pf_rand_addr(
+                                           rmask->addr32[1]);
                                else
                                        break;
                                if (rmask->addr32[0] != 0xffffffff)
-                                       rpool->counter.addr32[0] = arc4random();
+                                       rpool->counter.addr32[0] = pf_rand_addr(
+                                           rmask->addr32[0]);
                                break;
 #endif /* INET6 */
                        default:
@@ -500,11 +516,16 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct pf_addr *saddr,
                        }
                } else if (PF_AZERO(&rpool->counter, af)) {
                        /*
-                        * fall back to POOL_NONE if there are no addresses in
-                        * pool
+                        * fall back to POOL_NONE if there is a single host
+                        * address in pool.
                         */
-                       pf_addrcpy(naddr, raddr, af);
-                       break;
+                       if ((af == AF_INET &&
+                           rmask->addr32[0] == INADDR_BROADCAST) ||
+                           (af == AF_INET6 &&
+                           IN6_ARE_ADDR_EQUAL(&rmask->v6, &in6mask128))) {
+                               pf_addrcpy(naddr, raddr, af);
+                               break;
+                       }
                } else if (pf_match_addr(0, raddr, rmask, &rpool->counter, af))
                        return (1);
 
@@ -532,9 +553,9 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct pf_addr *saddr,
                        weight = rpool->weight;
                }
 
-               pf_addrcpy(naddr, &rpool->counter, af);
+               pf_poolmask(naddr, raddr, rmask, &rpool->counter, af);
                if (init_addr != NULL && PF_AZERO(init_addr, af))
-                       pf_addrcpy(init_addr, naddr, af);
+                       pf_addrcpy(init_addr, &rpool->counter, af);
                pf_addr_inc(&rpool->counter, af);
                break;
        case PF_POOL_LEASTSTATES: