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]

Date: Wed, 20 Dec 2023 07:47:58 +1030
From: Qu Wenruo <quwenruo.btrfs@....com>
To: David Laight <David.Laight@...LAB.COM>, '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



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