[PATCH v3 0/4] kstrtox: introduce memparse_safe()

Qu Wenruo
2024-01-03T23:28:17Z

[CHANGELOG]
v3:
- Fix the 32bit pointer pattern in the test case
The old pointer pattern for 32 bit systems is in fact 40 bits,
which would still lead to sparse warning.
The newer pattern is using UINTPTR_MAX to trim the pattern, then
converted to a pointer, which should not cause any trimmed bits and
make sparse happy.

v2:
- Make _parse_integer_fixup_radix() to always treat "0x" as hex
This is to make sure invalid strings like "0x" or "0xG" to fail
as expected for memparse_safe().
Or they would only parse the first 0, then leaving "x" for caller
to handle.

- Update the test case to include above failure cases
This including:
* "0x", just hex prefix without any suffix/follow up chars
* "0xK", just hex prefix and a stray suffix
* "0xY", hex prefix with an invalid char

- Fix a bug in btrfs' conversion to memparse_safe()
Where I forgot to delete the old memparse() line.

- Fix a compiler warning on m68K
On that platform, a pointer (32 bits) is smaller than unsigned long long
(64 bits), which can cause static checker to warn.

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

* Valid string with suffix, but disable the corresponding suffix
To make sure we still got the numeric string parsed, and can
still pass the disabled suffix to the caller.

With the new helper, migrate btrfs to the interface, and since the
@retptr behavior is the same, we won't cause any behavior change.

Qu Wenruo (4):
kstrtox: always skip the leading "0x" even if no more valid chars
kstrtox: introduce a safer version of memparse()
kstrtox: add unit tests for memparse_safe()
btrfs: migrate to the newer memparse_safe() helper

arch/x86/boot/string.c | 2 +-
fs/btrfs/ioctl.c | 6 +-
fs/btrfs/super.c | 9 +-
fs/btrfs/sysfs.c | 14 ++-
include/linux/kernel.h | 8 +-
include/linux/kstrtox.h | 15 +++
lib/cmdline.c | 5 +-
lib/kstrtox.c | 98 +++++++++++++++-
lib/test-kstrtox.c | 244 ++++++++++++++++++++++++++++++++++++++++
9 files changed, 392 insertions(+), 9 deletions(-)

--
2.43.0