Re: [PATCH net] ipvs: fix shift-out-of-bounds in ip_vs_rht_desired_size

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
ipvs: fix shift-out-of-bounds in ip_vs_rht_desired_size

This commit addresses a shift-out-of-bounds issue when roundup_pow_of_two()
is called with 0 in ip_vs_rht_desired_size. It updates the assignment to fall
back to 1 if n is 0.

> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index f5b7a2047291..ea450944465f 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -237,7 +237,7 @@ int ip_vs_rht_desired_size(struct netns_ipvs *ipvs, struct ip_vs_rht *t, int n,
>  {
>  	if (!t)
>  		return 1 << min_bits;
> -	n = roundup_pow_of_two(n);
> +	n = n ? roundup_pow_of_two(n) : 1;

The parameter n is a signed int. If n happens to be negative, it will evaluate
to true and be passed to roundup_pow_of_two().

Because roundup_pow_of_two() casts the input to unsigned long, a negative
value like -1 becomes ULONG_MAX. This results in a shift exponent of 64,
triggering a shift-out-of-bounds regression. Would it be safer to check
for n <= 0 instead?

Additionally, if n exceeds 1.07 billion (1 << 30) on systems with large
amounts of RAM, roundup_pow_of_two(n) evaluates to 1UL << 31. Since n is
a 32-bit signed int, assigning 1UL << 31 back to n overflows and results
in a negative number.

Since subsequent bounds checks use signed comparisons:

>  	if (lfactor < 0) {
>  		int factor = min(-lfactor, max_bits);
>  
>  		n = min(n, 1 << (max_bits - factor));

These checks will fail to constrain the size correctly.

[ ... ]

>  	/* Shrink but keep it n * 2 to prevent frequent resizing */
>  	return clamp(n << 1, 1 << min_bits, 1 << max_bits);
>  }

Eventually, the function returns clamp(n << 1, 1 << min_bits, 1 << max_bits).
Shifting a negative number causes undefined behavior, and the clamp operation
will force the hash table to its absolute minimum size.

Could this cause millions of connections to be placed into a minimally sized
hash table, causing severe collisions during RCU hash lookups?