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.kandy...@gmail.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.
> */
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
