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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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