|
From: 'Simon Horman' <horms@xxxxxxxxxx>
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?
|
