netdev - Re: [PATCH v2 iproute2] get_rate: detect 32bit overflows

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1370270231.1918.10.camel@bwh-desktop.uk.level5networks.com>
Date:	Mon, 3 Jun 2013 15:37:11 +0100
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	Stephen Hemminger <stephen@...workplumber.org>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 iproute2] get_rate: detect 32bit overflows

On Sun, 2013-06-02 at 14:36 -0700, Eric Dumazet wrote:
> Current rate limit is 34.359.738.360 bit per second, and
> unfortunately 40Gbps links are above it.
> 
> overflows in get_rate() are currently not detected, and some
> users are confused. Let's detect this and complain.
> 
> Note that some qdisc are ready to get extended range, but this will
> need additional attributes and new iproute2
> 
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> ---
> v2: divide by 8 only at the right time
> 
>  tc/tc_util.c |   13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/tc/tc_util.c b/tc/tc_util.c
> index 8e62a01..8ca9d4e 100644
> --- a/tc/tc_util.c
> +++ b/tc/tc_util.c
> @@ -146,21 +146,26 @@ static const struct rate_suffix {
>  int get_rate(unsigned *rate, const char *str)
>  {
>  	char *p;
> -	double bps = strtod(str, &p);
> +	long long bps = strtoll(str, &p, 0);
>  	const struct rate_suffix *s;
>  
>  	if (p == str)
>  		return -1;
>  
>  	if (*p == '\0') {
> -		*rate = bps / 8.;	/* assume bits/sec */
> +common:
> +		bps /= 8; /* -> bytes per second */
> +		*rate = bps;
> +		/* detect if an overflow happened */
> +		if (*rate != bps)
> +			return -1;
>  		return 0;
>  	}
>  
>  	for (s = suffixes; s->name; ++s) {
>  		if (strcasecmp(s->name, p) == 0) {
> -			*rate = (bps * s->scale) / 8.;
> -			return 0;
> +			bps *= s->scale;
> +			goto common;
>  		}
>  	}

This would be more understandable without a goto.  I think this ordering
would work:

	for (s = suffixes; ...) {
		if (...) {
			bps *= s->scale;
			p += strlen(s->name);
			break;
		}
	}

	if (*p)
		return -1; /* unrecognised suffix */

	bps /= 8;
	...

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html