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>] [day] [month] [year] [list]

Message-ID: <ZYBMoUT8pDL0Rn5V@smile.fi.intel.com>
Date: Mon, 18 Dec 2023 15:44:01 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Qu Wenruo <wqu@...e.com>
Cc: linux-btrfs@...r.kernel.org, Andrew Morton <akpm@...ux-foundation.org>,
	Christophe JAILLET <christophe.jaillet@...adoo.fr>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] lib/strtox: introduce kstrtoull_suffix() helper


On Fri, Dec 15, 2023 at 07:09:23PM +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'.

How would you distinguish 25E with [0x]25e?
I believe it's unsolvable issue as long as we have it already.

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

So, then you can probably improve memparse()?

> So here we introduce a new helper to address the problem,
> kstrtoull_suffix():

This is a horrible naming. What suffix? What would be without it
(if it's even possible)? I have more questions than answers...

> - Enhance _kstrtoull()
>   This allow _kstrtoull() to return even if it hits an invalid char, as
>   long as the optional parameter @retptr is provided.
> 
>   If @retptr is provided, _kstrtoull() would try its best to parse the
>   valid part, and leave the remaining to be handled by the caller.
> 
>   If @retptr is not provided, the behavior is not altered.

Can we not touch that one. I admit that it may be not used in the hot paths,
but I prefer that it does exactly what it does in a strict way.

> - New kstrtoull_suffix() helper
>   This new helper utilize the new @retptr capability of _kstrtoull(),
>   and provides 2 new ability:
> 
>   * Allow certain suffixes to be chosen
>     The recommended suffix list is "KkMmGgTtPp", excluding the overflow
>     prone "Ee". Undermost cases there is really no need to use "E" suffix
>     anyway.
>     And for those who really need that exabytes suffix, they can enable
>     that suffix pretty easily.
> 
>   * Add overflow checks for the suffixes
>     If the original number string is fine, but with the extra left
>     shift overflow happens, then -EOVERFLOW is returned.

And formal NAK due to lack of test cases. We do not accept new generic
code without test cases.

-- 
With Best Regards,
Andy Shevchenko