linux-kernel - Re: [PATCH v4 2/4] lib: vsprintf: Fix handling of number field widths in vsscanf

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: <YBwiQ+l6yqs+g+rr@alley>
Date:   Thu, 4 Feb 2021 17:35:15 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Richard Fitzgerald <rf@...nsource.cirrus.com>, rostedt@...dmis.org,
        sergey.senozhatsky@...il.com, linux@...musvillemoes.dk,
        shuah@...nel.org, linux-kernel@...r.kernel.org,
        linux-kselftest@...r.kernel.org, patches@...nsource.cirrus.com
Subject: Re: [PATCH v4 2/4] lib: vsprintf: Fix handling of number field
 widths in vsscanf

On Wed 2021-02-03 21:45:55, Andy Shevchenko wrote:
> On Wed, Feb 03, 2021 at 04:50:07PM +0000, Richard Fitzgerald wrote:
> > The existing code attempted to handle numbers by doing a strto[u]l(),
> > ignoring the field width, and then repeatedly dividing to extract the
> > field out of the full 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 dividing can recover the correct value.

> ...
> 
> > +	for (; max_chars > 0; max_chars--) {
> 
> Less fragile is to write
> 
> 	while (max_chars--)

Except that the original was more obvious at least for me.
I always prefer more readable code when the compiler might do
the optimization easily. But this is my personal taste.
I am fine with both variants.

> 
> This allows max_char to be an unsigned type.
> 
> Moreover...
> 
> > +	return _parse_integer_limit(s, base, p, INT_MAX);
> 
> You have inconsistency with INT_MAX vs, size_t above.

Ah, this was on my request. INT_MAX is already used on many other
locations in vsnprintf() for this purpose.

An alternative is to fix all the other locations. We would need to
check if it is really safe. Well, I do not want to force Richard
to fix this historical mess. He already put a lot lot of effort
into fixing this long term issue.

...

> > -	unsigned long long result;
> > +	const char *cp;
> > +	unsigned long long result = 0ULL;
> >  	unsigned int rv;
> >  
> > -	cp = _parse_integer_fixup_radix(cp, &base);
> > -	rv = _parse_integer(cp, base, &result);
> 
> > +	if (max_chars == 0) {
> > +		cp = startp;
> > +		goto out;
> > +	}
> 
> It's redundant if I'm not mistaken.

Also this is more obvious and less error prone from my POV.
But I agree that it is redundant. I am not sure if this
function is used in some fast paths.

Again I am fine with both variants.

> > +	cp = _parse_integer_fixup_radix(startp, &base);
> > +	if ((cp - startp) >= max_chars) {
> > +		cp = startp + max_chars;
> > +		goto out;
> > +	}
> 
> This will be exactly the same, no?

Best Regards,
Petr