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

[Qu Wenruo]

On 2023/12/20 03:12, David Laight wrote:
> 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;

This means the caller has to provide the suffix string in this
particular order.
For default suffix list it's not that hard as it's already defined as a
macro.

But for those call sites which needs "E", wrongly located "Ee" can screw
up the whole process.

> 	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.

Indeed, I just found a very simple example to prove it wrong, 4 bit
binary 0110, left shift 2, result is 1000, still larger than the
original one.

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