diff mbox series

[net,1/3] netfilter: ipset: regression in ip_set_hash_ip.c

Message ID 20221122212814.63177-2-pablo@netfilter.org (mailing list archive)
State Accepted
Commit c7aa1a76d4a0a3c401025b60c401412bbb60f8c6
Delegated to: Netdev Maintainers
Headers show
Series [net,1/3] netfilter: ipset: regression in ip_set_hash_ip.c | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/subject_prefix success Link
netdev/cover_letter success Pull request is its own cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers fail 1 blamed authors not CCed: kadlec@netfilter.org; 5 maintainers not CCed: fw@strlen.de vpai@akamai.com coreteam@netfilter.org johunt@akamai.com kadlec@netfilter.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 21 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Pablo Neira Ayuso Nov. 22, 2022, 9:28 p.m. UTC 
From: Vishwanath Pai <vpai@akamai.com>

This patch introduced a regression: commit 48596a8ddc46 ("netfilter:
ipset: Fix adding an IPv4 range containing more than 2^31 addresses")

The variable e.ip is passed to adtfn() function which finally adds the
ip address to the set. The patch above refactored the for loop and moved
e.ip = htonl(ip) to the end of the for loop.

What this means is that if the value of "ip" changes between the first
assignement of e.ip and the forloop, then e.ip is pointing to a
different ip address than "ip".

Test case:
$ ipset create jdtest_tmp hash:ip family inet hashsize 2048 maxelem 100000
$ ipset add jdtest_tmp 10.0.1.1/31
ipset v6.21.1: Element cannot be added to the set: it's already added

The value of ip gets updated inside the  "else if (tb[IPSET_ATTR_CIDR])"
block but e.ip is still pointing to the old value.

Fixes: 48596a8ddc46 ("netfilter: ipset: Fix adding an IPv4 range containing more than 2^31 addresses")
Reviewed-by: Joshua Hunt <johunt@akamai.com>
Signed-off-by: Vishwanath Pai <vpai@akamai.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/ipset/ip_set_hash_ip.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

patchwork-bot+netdevbpf@kernel.org Nov. 24, 2022, 3:20 a.m. UTC | #1 
Hello:

This series was applied to netdev/net.git (master)
by Pablo Neira Ayuso <pablo@netfilter.org>:

On Tue, 22 Nov 2022 22:28:12 +0100 you wrote:
> From: Vishwanath Pai <vpai@akamai.com>
> 
> This patch introduced a regression: commit 48596a8ddc46 ("netfilter:
> ipset: Fix adding an IPv4 range containing more than 2^31 addresses")
> 
> The variable e.ip is passed to adtfn() function which finally adds the
> ip address to the set. The patch above refactored the for loop and moved
> e.ip = htonl(ip) to the end of the for loop.
> 
> [...]

Here is the summary with links:
  - [net,1/3] netfilter: ipset: regression in ip_set_hash_ip.c
    https://git.kernel.org/netdev/net/c/c7aa1a76d4a0
  - [net,2/3] netfilter: ipset: restore allowing 64 clashing elements in hash:net,iface
    https://git.kernel.org/netdev/net/c/6a66ce44a51b
  - [net,3/3] netfilter: flowtable_offload: add missing locking
    https://git.kernel.org/netdev/net/c/bcd9e3c1656d

You are awesome, thank you!
diff mbox series

Patch

diff --git a/net/netfilter/ipset/ip_set_hash_ip.c b/net/netfilter/ipset/ip_set_hash_ip.c
index dd30c03d5a23..75d556d71652 100644
--- a/net/netfilter/ipset/ip_set_hash_ip.c
+++ b/net/netfilter/ipset/ip_set_hash_ip.c
@@ -151,18 +151,16 @@  hash_ip4_uadt(struct ip_set *set, struct nlattr *tb[],
 	if (((u64)ip_to - ip + 1) >> (32 - h->netmask) > IPSET_MAX_RANGE)
 		return -ERANGE;
 
-	if (retried) {
+	if (retried)
 		ip = ntohl(h->next.ip);
-		e.ip = htonl(ip);
-	}
 	for (; ip <= ip_to;) {
+		e.ip = htonl(ip);
 		ret = adtfn(set, &e, &ext, &ext, flags);
 		if (ret && !ip_set_eexist(ret, flags))
 			return ret;
 
 		ip += hosts;
-		e.ip = htonl(ip);
-		if (e.ip == 0)
+		if (ip == 0)
 			return 0;
 
 		ret = 0;