From 094a60281b9e8faf8f3e1a4497ab46955b5a1417 Mon Sep 17 00:00:00 2001 From: Kristof Provost Date: Mon, 18 Aug 2025 08:42:26 +0200 Subject: [PATCH] pf: fix potential infinite loop adding/deleting addresses in tables The 'nadd' returned by these calls is the number of addresses actually added or deleted. It can differ from the number userspace sent to the kernel if the addresses are already present (or not present for the delete case). This meant that if all of the addresses were already handled the kernel would return zero, putting us in an infinite loop. Handle this, and extend the test case to provoke this scenario. Reported by: netchild@ Fixes: bad279e12deb ("pf: convert DIOCRDELADDRS to netlink") Fixes: 8b388995b8b2 ("pf: convert DIOCRADDADDRS to netlink") Sponsored by: Rubicon Communications, LLC ("Netgate") --- lib/libpfctl/libpfctl.c | 22 ++++++++++------------ tests/sys/netpfil/pf/table.sh | 24 +++++++++++++++++++++++- 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/lib/libpfctl/libpfctl.c b/lib/libpfctl/libpfctl.c index cbd9d467714..0037f31df04 100644 --- a/lib/libpfctl/libpfctl.c +++ b/lib/libpfctl/libpfctl.c @@ -2453,7 +2453,7 @@ _pfctl_table_add_addrs_h(struct pfctl_handle *h, struct pfr_table *tbl, struct p snl_add_msg_attr_table(&nw, PF_TA_TABLE, tbl); snl_add_msg_attr_u32(&nw, PF_TA_FLAGS, flags); - for (int i = 0; i < size && i < 256; i++) + for (int i = 0; i < size; i++) snl_add_msg_attr_pfr_addr(&nw, PF_TA_ADDR, &addrs[i]); if ((hdr = snl_finalize_msg(&nw)) == NULL) @@ -2481,19 +2481,18 @@ pfctl_table_add_addrs_h(struct pfctl_handle *h, struct pfr_table *tbl, struct pf int ret; int off = 0; int partial_added; + int chunk_size; do { - ret = _pfctl_table_add_addrs_h(h, tbl, &addr[off], size - off, &partial_added, flags); + chunk_size = MIN(size - off, 256); + ret = _pfctl_table_add_addrs_h(h, tbl, &addr[off], chunk_size, &partial_added, flags); if (ret != 0) break; if (nadd) *nadd += partial_added; - off += partial_added; + off += chunk_size; } while (off < size); - if (nadd) - *nadd = off; - return (ret); } @@ -2521,7 +2520,7 @@ _pfctl_table_del_addrs_h(struct pfctl_handle *h, struct pfr_table *tbl, struct p snl_add_msg_attr_table(&nw, PF_TA_TABLE, tbl); snl_add_msg_attr_u32(&nw, PF_TA_FLAGS, flags); - for (int i = 0; i < size && i < 256; i++) + for (int i = 0; i < size; i++) snl_add_msg_attr_pfr_addr(&nw, PF_TA_ADDR, &addrs[i]); if ((hdr = snl_finalize_msg(&nw)) == NULL) @@ -2572,20 +2571,19 @@ pfctl_table_del_addrs_h(struct pfctl_handle *h, struct pfr_table *tbl, struct pf int ret; int off = 0; int partial_deleted; + int chunk_size; do { - ret = _pfctl_table_del_addrs_h(h, tbl, &addr[off], size - off, + chunk_size = MIN(size - off, 256); + ret = _pfctl_table_del_addrs_h(h, tbl, &addr[off], chunk_size, &partial_deleted, flags); if (ret != 0) break; if (ndel) *ndel += partial_deleted; - off += partial_deleted; + off += chunk_size; } while (off < size); - if (ndel) - *ndel = off; - return (ret); } diff --git a/tests/sys/netpfil/pf/table.sh b/tests/sys/netpfil/pf/table.sh index c773518e95e..65492545a13 100644 --- a/tests/sys/netpfil/pf/table.sh +++ b/tests/sys/netpfil/pf/table.sh @@ -641,9 +641,31 @@ large_body() -e match:"${expected}/${expected} addresses added." \ jexec alcatraz pfctl -t foo -T add -f ${pwd}/foo.lst actual=$(jexec alcatraz pfctl -t foo -T show | wc -l | awk '{ print $1; }') - if [[ $actual -ne $expected ]]; then + if [ $actual -ne $expected ]; then atf_fail "Unexpected number of table entries $expected $acual" fi + + # The second pass should work too, but confirm we've inserted everything + atf_check -s exit:0 \ + -e match:"0/${expected} addresses added." \ + jexec alcatraz pfctl -t foo -T add -f ${pwd}/foo.lst + + echo '42.42.42.42' >> ${pwd}/foo.lst + expected=$((${expected} + 1)) + + # And we can also insert one additional address + atf_check -s exit:0 \ + -e match:"1/${expected} addresses added." \ + jexec alcatraz pfctl -t foo -T add -f ${pwd}/foo.lst + + # Try to delete one address + atf_check -s exit:0 \ + -e match:"1/1 addresses deleted." \ + jexec alcatraz pfctl -t foo -T delete 42.42.42.42 + # And again, for the same address + atf_check -s exit:0 \ + -e match:"0/1 addresses deleted." \ + jexec alcatraz pfctl -t foo -T delete 42.42.42.42 } large_cleanup()