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:	bad279e12d ("pf: convert DIOCRDELADDRS to netlink")
Fixes:	8b388995b8 ("pf: convert DIOCRADDADDRS to netlink")
Sponsored by:	Rubicon Communications, LLC ("Netgate")

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);
 }
 

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()