linux-kernel - Re: [PATCH 0/3] kstrtox: introduce memparse

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]

Message-ID: <ZYxM0_hPdqdWxdOv@smile.fi.intel.com>
Date: Wed, 27 Dec 2023 18:12:03 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Qu Wenruo <wqu@...e.com>
Cc: linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org,
	akpm@...ux-foundation.org, christophe.jaillet@...adoo.fr,
	David.Laight@...lab.com, ddiss@...e.de
Subject: Re: [PATCH 0/3] kstrtox: introduce memparse_safe()

On Sat, Dec 23, 2023 at 08:28:04PM +1030, Qu Wenruo wrote:
> Function memparse() lacks error handling:
> 
> - If no valid number string at all
>   In that case @retptr would just be updated and return value would be
>   zero.
> 
> - No overflown detection
>   This applies to both the number string part, and the suffixes part.
>   And since we have no way to indicate errors, we can get weird results
>   like:
> 
>   	"25E" -> 10376293541461622784 (9E)
> 
>   This is due to the fact that for "E" suffix, there is only 4 bits
>   left, and 25 with 60 bits left shift would lead to overflow.
>   (And decision to support for that "E" suffix is already cursed)
> 
> So here we introduce a safer version of it: memparse_safe(), and mark
> the original one deprecated.
> Unfortunately I didn't find a good way to mark it deprecated, as with
> recent -Werror changes, '__deprecated' marco does not seem to warn
> anymore.
> 
> The new helper has the following advantages:
> 
> - Better overflow and invalid string detection
>   The overflow detection is for both the numberic part, and with the
>   suffix. Thus above "25E" would be rejected correctly.
>   The invalid string part means if there is no valid number starts at
>   the buffer, we return -EINVAL.
> 
> - Allow caller to select the suffixes, and saner default ones
>   The new default one would be "KMGTP", without the cursed and overflow
>   prone "E".
>   Some older code like setup_elfcorehdr() would benefit from it, if the
>   code really wants to only allow "KMG" suffixes.
> 
> - Keep the old @retptr behavior
>   So the existing callers can easily migrate to the new one, without the
>   need to do extra strsep() work.
> 
> - Finally test cases
>   The test case would cover more things other than the existing kstrtox
>   tests:
>   * The @retptr behavior
>     Either for bad cases, which @retptr should not be touched,
>     or for good cases, the @retptr is properly advanced,
> 
>   * Return value verification
>     Make sure we distinguish -EINVAL and -ERANGE correctly.
> 
> With the new helper, migrate btrfs to the interface, and since the
> @retptr behavior is the same, we won't cause any behavior change.

Thank you for the prompt update, I will be off till end of January,
and in any case this is material for v6.9+, so I will look at this
afterwards.

-- 
With Best Regards,
Andy Shevchenko