Received-SPF: pass (google.com: domain of linux-kernel+bounces-5587-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: David Laight <David.Laight@ACULAB.COM>
To: 'David Disseldorp' <ddiss@suse.de>, Qu Wenruo <wqu@suse.com>
CC: "linux-btrfs@vger.kernel.org" <linux-btrfs@vger.kernel.org>, Andrew Morton
	<akpm@linux-foundation.org>, Christophe JAILLET
	<christophe.jaillet@wanadoo.fr>, Andy Shevchenko
	<andriy.shevchenko@linux.intel.com>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 1/2] lib/strtox: introduce kstrtoull_suffix() helper
Thread-Topic: [PATCH 1/2] lib/strtox: introduce kstrtoull_suffix() helper
Thread-Index: AQHaMbPBRujg17A2g0aXH8N7pLP0+bCwzl3Q
Date: Tue, 19 Dec 2023 16:42:11 +0000
Message-ID: <8095c6ae5f8d412d8e6ff95707961a08@AcuMS.aculab.com>
References: <cover.1702628925.git.wqu@suse.com>
 <11da10b4d07bf472cd47410db65dc0e222d61e83.1702628925.git.wqu@suse.com>
 <20231218235946.32ab7a69@echidna>
In-Reply-To: <20231218235946.32ab7a69@echidna>
Accept-Language: en-GB, en-US
Precedence: bulk
MIME-Version: 1.0
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

From: David Disseldorp
> Sent: 18 December 2023 13:00
>=20
> On Fri, 15 Dec 2023 19:09:23 +1030, Qu Wenruo wrote:
>=20
> > 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 becaus=
e
> > 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 sam=
e
> > 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 make=
s
> > it super simple to overflow, resulting the incorrect value
> > 10376293541461622784 (9E).

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

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

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

That is just plain wrong.

=09David

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