netdev - Re: [PATCH] openvswitch: fix band bucket value computation in ovs_meter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <72dd193a-99c3-46bd-8263-1502e910b725@ovn.org>
Date: Tue, 22 Apr 2025 19:52:44 +0200
From: Ilya Maximets <i.maximets@....org>
To: Dmitry Kandybka <d.kandybka@...il.com>, Aaron Conole <aconole@...hat.com>
Cc: i.maximets@....org, Eelco Chaudron <echaudro@...hat.com>,
 dev@...nvswitch.org, netdev@...r.kernel.org, lvc-project@...uxtesting.org
Subject: Re: [PATCH] openvswitch: fix band bucket value computation in
 ovs_meter_execute()

On 4/21/25 11:28 PM, Dmitry Kandybka wrote:
> In ovs_meter_execute(), promote 'delta_ms' to 'long long int' to avoid
> possible integer overflow. It's assumed that'delta_ms' and 'band->rate'
> multiplication leads to overcomming UINT32_MAX.
> Compile tested only.

Hi, Dmitry.  Thanks for the patch.

It's true that the overflow is possible, however, it will only occur in
cases where meter configuration doesn't make any practical sense, e.g.
when the rate is very low, but the burst size is comparable with U32_MAX.
If the overflow happens in such a scenario it shouldn't cause any real
issues for the traffic, since the original meter was not really enforced
in the first place.

But I agree that the code can be improved to avoid mixing different types.
There is another potential overflow in a way max_delta_t is calculated,
and that is directly related to the overflow here.  So, I'd suggest
instead of just adjusting this one place, move all the internal variables
that can overflow to u64 and get rid of signed long long int usage by
comparing values before subtraction.  i.e. convert band_max_delta_t,
max_delta_t, delta_ms, now_ms and max_bucket_size into u64 and get rid of
the long_delta_ms.  Keep all the checks, obviously, so the math stays
the same.  Types of all the variables that come from uAPI should stay
as they are.  Such unification of types should help avoiding any potential
overflows.

While at it, may also optionally convert all the local 'int' variables
and iterators that store or walk over n_meters or n_bands to u32/u16.

Best regards, Ilya Maximets.

> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitry Kandybka <d.kandybka@...il.com>
> ---
>  net/openvswitch/meter.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/openvswitch/meter.c b/net/openvswitch/meter.c
> index cc08e0403909..2fab53ac60a8 100644
> --- a/net/openvswitch/meter.c
> +++ b/net/openvswitch/meter.c
> @@ -594,11 +594,11 @@ bool ovs_meter_execute(struct datapath *dp, struct sk_buff *skb,
>  {
>  	long long int now_ms = div_u64(ktime_get_ns(), 1000 * 1000);
>  	long long int long_delta_ms;
> +	long long int delta_ms;
>  	struct dp_meter_band *band;
>  	struct dp_meter *meter;
>  	int i, band_exceeded_max = -1;
>  	u32 band_exceeded_rate = 0;
> -	u32 delta_ms;
>  	u32 cost;
>  
>  	meter = lookup_meter(&dp->meter_tbl, meter_id);
> @@ -623,7 +623,7 @@ bool ovs_meter_execute(struct datapath *dp, struct sk_buff *skb,
>  	 * wrap around below.
>  	 */
>  	delta_ms = (long_delta_ms > (long long int)meter->max_delta_t)
> -		   ? meter->max_delta_t : (u32)long_delta_ms;
> +		   ? meter->max_delta_t : long_delta_ms;
>  
>  	/* Update meter statistics.
>  	 */