linux-kernel - RE: [PATCH 1/2] lib/strtox: introduce kstrtoull

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: <8095c6ae5f8d412d8e6ff95707961a08@AcuMS.aculab.com>
Date: Tue, 19 Dec 2023 16:42:11 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'David Disseldorp' <ddiss@...e.de>, Qu Wenruo <wqu@...e.com>
CC: "linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>, Andrew Morton
	<akpm@...ux-foundation.org>, Christophe JAILLET
	<christophe.jaillet@...adoo.fr>, Andy Shevchenko
	<andriy.shevchenko@...ux.intel.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/2] lib/strtox: introduce kstrtoull_suffix() helper

From: David Disseldorp
> Sent: 18 December 2023 13:00
> 
> On Fri, 15 Dec 2023 19:09:23 +1030, Qu Wenruo wrote:
> 
> > Just as mentioned in the comment of memparse(), the simple_stroull()
> > usage can lead to overflow all by itself.
> >
> > Furthermore, the suffix calculation is also super overflow prone because
> > that some suffix like "E" itself would eat 60bits, leaving only 4 bits
> > available.
> >
> > And that suffix "E" can also lead to confusion since it's using the same
> > char of hex Ox'E'.
> >
> > One simple example to expose all the problem is to use memparse() on
> > "25E".
> > The correct value should be 28823037615171174400, but the suffix E makes
> > it super simple to overflow, resulting the incorrect value
> > 10376293541461622784 (9E).

Some more bikeshed paint :-)
...
> > +	ret = _kstrtoull(s, base, &init_value, &endptr);
> > +	/* Either already overflow or no number string at all. */
> > +	if (ret < 0)
> > +		return ret;
> > +	final_value = init_value;
> > +	/* No suffixes. */
> > +	if (!*endptr)
> > +		goto done;

How about:
	suffix = *endptr;
	if (!strchr(suffixes, suffix))
		return -ENIVAL;
	shift = strcspn("KkMmGgTtPp", suffix)/2 * 10 + 10;
	if (shift > 50)
		return -EINVAL;
	if (value >> (64 - shift))
		return -EOVERFLOW;
	value <<= shift;

Although purists might want to multiply by 1000 not 1024.
And SI multipliers are all upper-case - except k.
	
...
> > +	/* Overflow check. */
> > +	if (final_value < init_value)
> > +		return -EOVERFLOW;

That is just plain wrong.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)