lkml.org 
[lkml]   [2020]   [Nov]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] lib: vsprintf: Fix handling of number field widths in vsscanf
From
Date


On 20/11/2020 15:07, Steven Rostedt wrote:
> On Fri, 20 Nov 2020 12:05:25 +0100
> Petr Mladek <pmladek@suse.com> wrote:
>
>> On Mon 2020-11-16 14:32:52, Richard Fitzgerald wrote:
>>> The existing code attempted to handle numbers by doing a strto[u]l(),
>>> ignoring the field width, and then bitshifting the field out of the
>>> converted value. If the string contains a run of valid digits longer
>>> than will fit in a long or long long, this would overflow and no amount
>>> of bitshifting can recover the correct value.
>>>
>>> This patch fixes vsscanf to obey number field widths.
>>>
>>> A new _parse_integer_limit() is added that takes a limit for the number
>>> of characters to parse. A length of INT_MAX is effectively unlimited, as
>>> we are not likely to need parsing of digit strings >INT_MAX length.
>>>
>>> The number field conversion in vsscanf is changed to use this new
>>> _parse_integer_limit() function so that field widths are obeyed when
>>> parsing the number. Note also that the conversion is always done as a
>>> long long - as there's currently no overflow checking there is no point
>>> implementing separate long and long long conversions.
>>>
>>> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
>>> index a14ccf905055..9867501a4ab0 100644
>>> --- a/lib/kstrtox.c
>>> +++ b/lib/kstrtox.c
>>> @@ -39,20 +39,23 @@ const char *_parse_integer_fixup_radix(const char *s, unsigned int *base)
>>>
>>> /*
>>> * Convert non-negative integer string representation in explicitly given radix
>>> - * to an integer.
>>> + * to an integer. The maximum number of characters to convert can be given.
>>> + * A character limit of INT_MAX is effectively unlimited since a string that
>>> + * long is unreasonable.
>>
>> The INT_MAX value meaning is obvious. It does not need to be
>> mentioned. It is the same as with vsnprintf().
>
> Yeah, but I never think that restating the obvious is a bad idea.
> Especially when something that is obvious to us, is not obvious to a new
> comer. There's been lots of times I wish someone mentioned the obvious in a
> comment somewhere, because it wasn't obvious to me ;-)
>
> I vote to keep it in.
>
>>
>>
>>> * Return number of characters consumed maybe or-ed with overflow bit.
>>> * If overflow occurs, result integer (incorrect) is still returned.
>>> *
>>> * Don't you dare use this function.
>>> */
>>> -unsigned int _parse_integer(const char *s, unsigned int base, unsigned long long *p)
>>> +unsigned int _parse_integer_limit(const char *s, unsigned int base, unsigned long long *p,
>>> + int max_chars)
>>
>> Please, use size_t. Passing negative value usually means
>> that the caller did not handle some situation correctly.
>> And it actually happened in this patch, see below.
>>
>> nit: better ballance the length of the lines above. I mean to move
>> *p to the next line:
>>
>> unsigned int _parse_integer_limit(const char *s, unsigned int base,
>> unsigned long long *p, size_t max_chars)
>>
>>
>>> {
>>> unsigned long long res;
>>> unsigned int rv;
>>>
>>> res = 0;
>>> rv = 0;
>>> - while (1) {
>>> + for (; max_chars > 0; max_chars--) {
>>> unsigned int c = *s;
>>> unsigned int lc = c | 0x20; /* don't tolower() this line */
>>> unsigned int val;
>>> @@ -82,6 +85,11 @@ unsigned int _parse_integer(const char *s, unsigned int base, unsigned long long
>>> return rv;
>>> }
>>>
>>> +unsigned int _parse_integer(const char *s, unsigned int base, unsigned long long *p)
>>> +{
>>> + return _parse_integer_limit(s, base, p, INT_MAX);
>>> +}
>>> +
>>> static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
>>> {
>>> unsigned long long _res;
>>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>>> index 14c9a6af1b23..8ec47b5da2cb 100644
>>> --- a/lib/vsprintf.c
>>> +++ b/lib/vsprintf.c
>>> @@ -53,6 +53,25 @@
>>> #include <linux/string_helpers.h>
>>> #include "kstrtox.h"
>>>
>>> +static unsigned long long simple_strntoull(const char *startp, int max_chars,
>>> + char **endp, unsigned int base)
>>> +{
>>> + const char *cp;
>>> + unsigned long long result;
>>> + unsigned int rv;
>>> +
>>> + cp = _parse_integer_fixup_radix(startp, &base);
>>> + max_chars -= (cp - startp);
>>
>> Negative value means that _parse_integer_fixup_radix() already
>> proceed more characters than allowed. I would handle this
>> the following way:
>>
>> if (cp - startp > max_chars) {
>> cp = startp + max_chars;
>> result = 0LL;
>> goto out;
>
> Agreed. I was looking at what sscanf() in user space does.
>
> And testing it with the following:
>
> char *line = "0x123456789abcdef0123456789\n";
> int i;
>
> for (i = 0; i < 10; i++) {
> char str[32];
> long a, b;
>
> if (i)
> sprintf(str, "%%%dli%%9lx", i);
> else
> strcpy(str, "%li%6lx");
>
> ret = sscanf(line, str, &a, &b);
> switch (ret) {
> case 1:
> printf("read 1 '%s': %lx\n", str, a);
> break;
> case 2:
> printf("read 2 '%s': %lx %lx\n", str, a, b);
> break;
> default:
> printf("Failed to read: '%s' ret = %d\n", str, ret);
> }
> }
>
> And the above produced:
>
> read 1 '%li%6lx': 7fffffffffffffff
> read 1 '%1li%9lx': 0
> read 2 '%2li%9lx': 0 123456789
> read 2 '%3li%9lx': 1 23456789a
> read 2 '%4li%9lx': 12 3456789ab
> read 2 '%5li%9lx': 123 456789abc
> read 2 '%6li%9lx': 1234 56789abcd
> read 2 '%7li%9lx': 12345 6789abcde
> read 2 '%8li%9lx': 123456 789abcdef
> read 2 '%9li%9lx': 1234567 89abcdef0
>
> The first line I'm assuming is because %li overflowed (more digits than a
> 64 bit could hold).
>
> But yeah, we could very much have cp - startp > max_chars.
>

My code handles the prefix overflow, but I did it by having
__parse_integer_limit() simply give 0 if max_chars <= 0.

So if the field width isn't enough for the prefix/leading '-' and at
least one digit, subtracting the prefix length from the field length
will give a max_chars <= 0. And you'll get a result of 0 as in your
'%2li%9lx' test case.

I thought this was nice because it didn't need to add code to check
for the prefix overflow - it comes inherently from the loop in
__parse_integer_limit(). But I'm willing to change
__parse_integer_limit() to take an unsigned and add explicit checks for
the prefix/'-' overflow cases.

>
>>
>>> + rv = _parse_integer_limit(cp, base, &result, max_chars);
>>> + /* FIXME */
>>> + cp += (rv & ~KSTRTOX_OVERFLOW);
>>
>> out:
>>
>>> + if (endp)
>>> + *endp = (char *)cp;
>>> +
>>> + return result;
>>> +}
>>> +
>>> /**
>>> * simple_strtoull - convert a string to an unsigned long long
>>> * @cp: The start of the string
>>> @@ -126,6 +134,15 @@ long long simple_strtoll(const char *cp, char **endp, unsigned int base)
>>> }
>>> EXPORT_SYMBOL(simple_strtoll);
>>>
>>> +static long long simple_strntoll(const char *cp, int max_chars, char **endp,
>>> + unsigned int base)
>>> +{
>>> + if (*cp == '-')
>>> + return -simple_strntoull(cp + 1, max_chars - 1, endp, base);
>>> +
>>> + return simple_strntoull(cp, max_chars, endp, base);
>>> +}
>>
>> Please, use this in simple_strtoll() like it is already done in
>> simple_strtoull(). I mean:
>>
>> long long simple_strtoll(const char *cp, char **endp, unsigned int base)
>> {
>> return simple_strntoll(cp, INT_MAX, endp, base);
>> }
>
> Agreed.
>
>>
>>> +
>>> static noinline_for_stack
>>> int skip_atoi(const char **s)
>>> {
>>
>> Finally, it would be great to add some selftests for this into
>> lib/test_printf.c.
>>
>> Thanks a lot for working on this. I like this approach.
>
> +1
>
> -- Steve
>

\
 
 \ /
  Last update: 2020-11-20 17:05    [W:0.064 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site