[v4,2/4] kstrtox: introduce a safer version of memparse()
Commit Message
[BUGS]
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.
[CAUSE]
The root cause is already mentioned in the comments of the function, the
usage of simple_strtoull() is the source of evil.
Furthermore the function prototype is no good either, just returning an
unsigned long long gives us no way to indicate an error.
[FIX]
Due to the prototype limits, we can not have a drop-in replacement for
memparse().
This patch can only help by introduce a new helper, memparse_safe(), and
mark the old memparse() deprecated.
The new memparse_safe() has the following improvement:
- Invalid string detection
If no number string can be detected at all, -EINVAL would be returned.
- Better overflow detection
Both the string part and the extra left shift would have overflow
detection.
Any overflow would result -ERANGE.
- Safer default suffix selection
The helper allows the caller to choose the suffixes that they want to
use.
But only "KMGTP" are recommended by default since the "E" leaves only
4 bits before overflow.
For those callers really know what they are doing, they can still
manually to include all suffixes.
Due to the prototype change, callers should migrate to the new one and
change their code and add extra error handling.
Signed-off-by: Qu Wenruo <wqu@suse.com>
Reviewed-by: David Disseldorp <ddiss@suse.de>
---
include/linux/kernel.h | 8 +++-
include/linux/kstrtox.h | 15 +++++++
lib/cmdline.c | 4 +-
lib/kstrtox.c | 99 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 124 insertions(+), 2 deletions(-)
Comments
Hi,
On 1/4/24 18:35, Qu Wenruo wrote:
> [BUGS]
> 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
overflow
> 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.
>
> [CAUSE]
> The root cause is already mentioned in the comments of the function, the
> usage of simple_strtoull() is the source of evil.
> Furthermore the function prototype is no good either, just returning an
> unsigned long long gives us no way to indicate an error.
>
> [FIX]
> Due to the prototype limits, we can not have a drop-in replacement for
> memparse().
>
> This patch can only help by introduce a new helper, memparse_safe(), and
> mark the old memparse() deprecated.
>
> The new memparse_safe() has the following improvement:
>
> - Invalid string detection
> If no number string can be detected at all, -EINVAL would be returned.
is returned.
>
> - Better overflow detection
> Both the string part and the extra left shift would have overflow
have overflow
> detection.
> Any overflow would result -ERANGE.
Any overflow results in -ERANGE.
>
> - Safer default suffix selection
> The helper allows the caller to choose the suffixes that they want to
> use.
> But only "KMGTP" are recommended by default since the "E" leaves only
> 4 bits before overflow.
> For those callers really know what they are doing, they can still
> manually to include all suffixes.
>
> Due to the prototype change, callers should migrate to the new one and
> change their code and add extra error handling.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> Reviewed-by: David Disseldorp <ddiss@suse.de>
> ---
> include/linux/kernel.h | 8 +++-
> include/linux/kstrtox.h | 15 +++++++
> lib/cmdline.c | 4 +-
> lib/kstrtox.c | 99 +++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 124 insertions(+), 2 deletions(-)
>
> diff --git a/lib/cmdline.c b/lib/cmdline.c
> index 90ed997d9570..35dbb03b5592 100644
> --- a/lib/cmdline.c
> +++ b/lib/cmdline.c
> @@ -139,10 +139,12 @@ char *get_options(const char *str, int nints, int *ints)
> EXPORT_SYMBOL(get_options);
>
> /**
> - * memparse - parse a string with mem suffixes into a number
> + * memparse - DEPRECATED, parse a string with mem suffixes into a number
> * @ptr: Where parse begins
> * @retptr: (output) Optional pointer to next char after parse completes
> *
> + * There is no way to handle errors, and no overflown detection and string
> + * sanity checks.
> * Parses a string into a number. The number stored at @ptr is
> * potentially suffixed with K, M, G, T, P, E.
> */
> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
> index 41c9a499bbf3..375c7f0842e3 100644
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -113,6 +113,105 @@ static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
> return 0;
> }
>
> +/**
> + * memparse_safe - convert a string to an unsigned long long, safer version of
> + * memparse()
> + *
> + * @s: The start of the string. Must be null-terminated.
Unless I misunderstand, this is the biggest problem that I see with
memparse_safe(): "Must be null-terminated".
memparse() does not have that requirement.
And how is @retptr updated if the string is null-terminated?
If the "Must be null-terminated." is correct, it requires that every user/caller
first determine the end of the number (how? space and/or any special character
or any alphabetic character that is not in KMGTPE? Then save that ending char,
change it to NUL, call memparse_safe(), then restore the saved char?
I'm hoping that the documentation is not correct...
> + * The base is determined automatically, if it starts with "0x"
> + * the base is 16, if it starts with "0" the base is 8, otherwise
> + * the base is 10.
> + * After a valid number string, there can be at most one
> + * case-insensitive suffix character, specified by the @suffixes
> + * parameter.
> + *
> + * @suffixes: The suffixes which should be handled. Use logical ORed
> + * memparse_suffix enum to indicate the supported suffixes.
> + * The suffixes are case-insensitive, all 2 ^ 10 based.
> + * Supported ones are "KMGPTE".
> + * If one suffix (one of "KMGPTE") is hit but that suffix is
> + * not specified in the @suffxies parameter, it ends the parse
> + * normally, with @retptr pointed to the (unsupported) suffix.
> + * E.g. "68k" with suffxies "M" returns 68 decimal, @retptr
> + * updated to 'k'.
> + *
> + * @res: Where to write the result.
> + *
> + * @retptr: (output) Optional pointer to the next char after parse completes.
> + *
> + * Returns:
> + * * %0 if any valid numeric string can be parsed, and @retptr is updated.
> + * * %-EINVAL if no valid number string can be found.
> + * * %-ERANGE if the number overflows.
> + * * For negative return values, @retptr is not updated.
> + */
> +noinline int memparse_safe(const char *s, enum memparse_suffix suffixes,
> + unsigned long long *res, char **retptr)
> +{
Thanks.
On 2024/1/15 14:57, Randy Dunlap wrote:
[...]
>> @@ -113,6 +113,105 @@ static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
>> return 0;
>> }
>>
>> +/**
>> + * memparse_safe - convert a string to an unsigned long long, safer version of
>> + * memparse()
>> + *
>> + * @s: The start of the string. Must be null-terminated.
>
> Unless I misunderstand, this is the biggest problem that I see with
> memparse_safe(): "Must be null-terminated".
> memparse() does not have that requirement.
This is just an extra safety requirement.
In reality, memparse_safe() would end at the either the first
unsupported suffix after the valid numeric string (including '\0'),
or won't be updated if any error is hit (either no valid string at all,
or some overflow happened).
For most if not all call sites, the string passed in is already
null-terminated.
>
> And how is @retptr updated if the string is null-terminated?
E.g "123456G\0", in this case if suffix "G" is allowed, then @retptr
would be updated to '\0'.
Or another example "123456\0", @retptr would still be updated to '\0'.
>
> If the "Must be null-terminated." is correct, it requires that every user/caller
> first determine the end of the number (how? space and/or any special character
> or any alphabetic character that is not in KMGTPE? Then save that ending char,
> change it to NUL, call memparse_safe(), then restore the saved char?
There are already test cases like "86k \0" (note all strings in the test
case is all null terminated), which would lead to a success parse, with
@retptr updated to ' ' (if suffix K is specified) or 'k' (if suffix K is
not specified).
So the behavior is still the same.
It may be my expression too confusing.
Any recommendation for the comments?
Thanks,
Qu
>
> I'm hoping that the documentation is not correct...
>
>> + * The base is determined automatically, if it starts with "0x"
>> + * the base is 16, if it starts with "0" the base is 8, otherwise
>> + * the base is 10.
>> + * After a valid number string, there can be at most one
>> + * case-insensitive suffix character, specified by the @suffixes
>> + * parameter.
>> + *
>> + * @suffixes: The suffixes which should be handled. Use logical ORed
>> + * memparse_suffix enum to indicate the supported suffixes.
>> + * The suffixes are case-insensitive, all 2 ^ 10 based.
>> + * Supported ones are "KMGPTE".
>> + * If one suffix (one of "KMGPTE") is hit but that suffix is
>> + * not specified in the @suffxies parameter, it ends the parse
>> + * normally, with @retptr pointed to the (unsupported) suffix.
>> + * E.g. "68k" with suffxies "M" returns 68 decimal, @retptr
>> + * updated to 'k'.
>> + *
>> + * @res: Where to write the result.
>> + *
>> + * @retptr: (output) Optional pointer to the next char after parse completes.
>> + *
>> + * Returns:
>> + * * %0 if any valid numeric string can be parsed, and @retptr is updated.
>> + * * %-EINVAL if no valid number string can be found.
>> + * * %-ERANGE if the number overflows.
>> + * * For negative return values, @retptr is not updated.
>> + */
>> +noinline int memparse_safe(const char *s, enum memparse_suffix suffixes,
>> + unsigned long long *res, char **retptr)
>> +{
>
> Thanks.
Hi,
On 1/14/24 21:27, Qu Wenruo wrote:
>
>
> On 2024/1/15 14:57, Randy Dunlap wrote:
> [...]
>>> @@ -113,6 +113,105 @@ static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
>>> return 0;
>>> }
>>>
>>> +/**
>>> + * memparse_safe - convert a string to an unsigned long long, safer version of
>>> + * memparse()
>>> + *
>>> + * @s: The start of the string. Must be null-terminated.
>>
>> Unless I misunderstand, this is the biggest problem that I see with
>> memparse_safe(): "Must be null-terminated".
>> memparse() does not have that requirement.
>
> This is just an extra safety requirement.
>
> In reality, memparse_safe() would end at the either the first
> unsupported suffix after the valid numeric string (including '\0'),
> or won't be updated if any error is hit (either no valid string at all,
> or some overflow happened).
>
> For most if not all call sites, the string passed in is already
> null-terminated.
>
>>
>> And how is @retptr updated if the string is null-terminated?
>
> E.g "123456G\0", in this case if suffix "G" is allowed, then @retptr
> would be updated to '\0'.
>
> Or another example "123456\0", @retptr would still be updated to '\0'.
>
>>
>> If the "Must be null-terminated." is correct, it requires that every user/caller
>> first determine the end of the number (how? space and/or any special character
>> or any alphabetic character that is not in KMGTPE? Then save that ending char,
>> change it to NUL, call memparse_safe(), then restore the saved char?
>
> There are already test cases like "86k \0" (note all strings in the test
> case is all null terminated), which would lead to a success parse, with
> @retptr updated to ' ' (if suffix K is specified) or 'k' (if suffix K is
> not specified).
>
> So the behavior is still the same.
> It may be my expression too confusing.
>
> Any recommendation for the comments?
Well, "Must be null-terminated." is incorrect, so explain better where
the numeric conversion ends.
Thanks.
>
> Thanks,
> Qu
>
>>
>> I'm hoping that the documentation is not correct...
>>
>>> + * The base is determined automatically, if it starts with "0x"
>>> + * the base is 16, if it starts with "0" the base is 8, otherwise
>>> + * the base is 10.
>>> + * After a valid number string, there can be at most one
>>> + * case-insensitive suffix character, specified by the @suffixes
>>> + * parameter.
>>> + *
>>> + * @suffixes: The suffixes which should be handled. Use logical ORed
>>> + * memparse_suffix enum to indicate the supported suffixes.
>>> + * The suffixes are case-insensitive, all 2 ^ 10 based.
>>> + * Supported ones are "KMGPTE".
>>> + * If one suffix (one of "KMGPTE") is hit but that suffix is
>>> + * not specified in the @suffxies parameter, it ends the parse
>>> + * normally, with @retptr pointed to the (unsupported) suffix.
>>> + * E.g. "68k" with suffxies "M" returns 68 decimal, @retptr
>>> + * updated to 'k'.
>>> + *
>>> + * @res: Where to write the result.
>>> + *
>>> + * @retptr: (output) Optional pointer to the next char after parse completes.
>>> + *
>>> + * Returns:
>>> + * * %0 if any valid numeric string can be parsed, and @retptr is updated.
>>> + * * %-EINVAL if no valid number string can be found.
>>> + * * %-ERANGE if the number overflows.
>>> + * * For negative return values, @retptr is not updated.
>>> + */
>>> +noinline int memparse_safe(const char *s, enum memparse_suffix suffixes,
>>> + unsigned long long *res, char **retptr)
>>> +{
>>
>> Thanks.
>
@@ -201,7 +201,13 @@ void do_exit(long error_code) __noreturn;
extern int get_option(char **str, int *pint);
extern char *get_options(const char *str, int nints, int *ints);
-extern unsigned long long memparse(const char *ptr, char **retptr);
+
+/*
+ * DEPRECATED, lack of any kind of error handling.
+ *
+ * Use memparse_safe() from lib/kstrtox.c instead.
+ */
+extern __deprecated unsigned long long memparse(const char *ptr, char **retptr);
extern bool parse_option_str(const char *str, const char *option);
extern char *next_arg(char *args, char **param, char **val);
@@ -9,6 +9,21 @@
int __must_check _kstrtoul(const char *s, unsigned int base, unsigned long *res);
int __must_check _kstrtol(const char *s, unsigned int base, long *res);
+enum memparse_suffix {
+ MEMPARSE_SUFFIX_K = 1 << 0,
+ MEMPARSE_SUFFIX_M = 1 << 1,
+ MEMPARSE_SUFFIX_G = 1 << 2,
+ MEMPARSE_SUFFIX_T = 1 << 3,
+ MEMPARSE_SUFFIX_P = 1 << 4,
+ MEMPARSE_SUFFIX_E = 1 << 5,
+};
+
+#define MEMPARSE_SUFFIXES_DEFAULT (MEMPARSE_SUFFIX_K | MEMPARSE_SUFFIX_M |\
+ MEMPARSE_SUFFIX_G | MEMPARSE_SUFFIX_T |\
+ MEMPARSE_SUFFIX_P)
+
+int __must_check memparse_safe(const char *s, enum memparse_suffix suffixes,
+ unsigned long long *res, char **retptr);
int __must_check kstrtoull(const char *s, unsigned int base, unsigned long long *res);
int __must_check kstrtoll(const char *s, unsigned int base, long long *res);
@@ -139,10 +139,12 @@ char *get_options(const char *str, int nints, int *ints)
EXPORT_SYMBOL(get_options);
/**
- * memparse - parse a string with mem suffixes into a number
+ * memparse - DEPRECATED, parse a string with mem suffixes into a number
* @ptr: Where parse begins
* @retptr: (output) Optional pointer to next char after parse completes
*
+ * There is no way to handle errors, and no overflown detection and string
+ * sanity checks.
* Parses a string into a number. The number stored at @ptr is
* potentially suffixed with K, M, G, T, P, E.
*/
@@ -113,6 +113,105 @@ static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
return 0;
}
+/**
+ * memparse_safe - convert a string to an unsigned long long, safer version of
+ * memparse()
+ *
+ * @s: The start of the string. Must be null-terminated.
+ * The base is determined automatically, if it starts with "0x"
+ * the base is 16, if it starts with "0" the base is 8, otherwise
+ * the base is 10.
+ * After a valid number string, there can be at most one
+ * case-insensitive suffix character, specified by the @suffixes
+ * parameter.
+ *
+ * @suffixes: The suffixes which should be handled. Use logical ORed
+ * memparse_suffix enum to indicate the supported suffixes.
+ * The suffixes are case-insensitive, all 2 ^ 10 based.
+ * Supported ones are "KMGPTE".
+ * If one suffix (one of "KMGPTE") is hit but that suffix is
+ * not specified in the @suffxies parameter, it ends the parse
+ * normally, with @retptr pointed to the (unsupported) suffix.
+ * E.g. "68k" with suffxies "M" returns 68 decimal, @retptr
+ * updated to 'k'.
+ *
+ * @res: Where to write the result.
+ *
+ * @retptr: (output) Optional pointer to the next char after parse completes.
+ *
+ * Returns:
+ * * %0 if any valid numeric string can be parsed, and @retptr is updated.
+ * * %-EINVAL if no valid number string can be found.
+ * * %-ERANGE if the number overflows.
+ * * For negative return values, @retptr is not updated.
+ */
+noinline int memparse_safe(const char *s, enum memparse_suffix suffixes,
+ unsigned long long *res, char **retptr)
+{
+ unsigned long long value;
+ unsigned int rv;
+ int shift = 0;
+ int base = 0;
+
+ s = _parse_integer_fixup_radix(s, &base);
+ rv = _parse_integer(s, base, &value);
+ if (rv & KSTRTOX_OVERFLOW)
+ return -ERANGE;
+ if (rv == 0)
+ return -EINVAL;
+
+ s += rv;
+ switch (*s) {
+ case 'K':
+ case 'k':
+ if (!(suffixes & MEMPARSE_SUFFIX_K))
+ break;
+ shift = 10;
+ break;
+ case 'M':
+ case 'm':
+ if (!(suffixes & MEMPARSE_SUFFIX_M))
+ break;
+ shift = 20;
+ break;
+ case 'G':
+ case 'g':
+ if (!(suffixes & MEMPARSE_SUFFIX_G))
+ break;
+ shift = 30;
+ break;
+ case 'T':
+ case 't':
+ if (!(suffixes & MEMPARSE_SUFFIX_T))
+ break;
+ shift = 40;
+ break;
+ case 'P':
+ case 'p':
+ if (!(suffixes & MEMPARSE_SUFFIX_P))
+ break;
+ shift = 50;
+ break;
+ case 'E':
+ case 'e':
+ if (!(suffixes & MEMPARSE_SUFFIX_E))
+ break;
+ shift = 60;
+ break;
+ }
+ if (shift) {
+ s++;
+ if (value >> (64 - shift))
+ return -ERANGE;
+ value <<= shift;
+ }
+ *res = value;
+ if (retptr)
+ *retptr = (char *)s;
+ return 0;
+}
+EXPORT_SYMBOL(memparse_safe);
+
/**
* kstrtoull - convert a string to an unsigned long long
* @s: The start of the string. The string must be null-terminated, and may also