RE: [PATCH 1/2] lib/strtox: introduce kstrtoull_suffix() helper

[David Laight]

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)