| 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
| ||
|
Message-ID: <X7fvhY51RIugA7Vq@alley>
Date: Fri, 20 Nov 2020 17:32:05 +0100
From: Petr Mladek <pmladek@...e.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Richard Fitzgerald <rf@...nsource.cirrus.com>,
sergey.senozhatsky@...il.com, linux-kernel@...r.kernel.org,
patches@...nsource.cirrus.com
Subject: Re: [PATCH] lib: vsprintf: Fix handling of number field widths in
vsscanf
On Fri 2020-11-20 10:07:05, Steven Rostedt wrote:
> On Fri, 20 Nov 2020 12:05:25 +0100
> Petr Mladek <pmladek@...e.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.
Fair enough.
I have suggested to remove it because it was somehow hard to parse for
me. It confused me more than helped. It is funny because I often write
these long and complicated sentences as well. And I really confuse
people from time to time.
I wonder if the following is more strightforward:
* Convert non-negative integer string representation in
* explicitly given radix to an integer.
*
* @max_chars limits the number of converted characters.
* Use INT_MAX when unlimited. It is an arbitrary, big enough,
* number that is used also by vsnprintf().
> > > * 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)
> >
> >
> > > {
> > > 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).
Yup, it looks like LONG_LONG_MAX.
> But yeah, we could very much have cp - startp > max_chars.
Thanks a lot for testing it.
Best Regards,
Petr
Powered by blists - more mailing lists