linux-kernel - Re: [PATCH v5 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] [day] [month] [year] [list]

Message-ID: <YCUx9EqdV4ZOXqyI@alley>
Date:   Thu, 11 Feb 2021 14:32:36 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Richard Fitzgerald <rf@...nsource.cirrus.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.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 v5 2/4] lib: vsprintf: Fix handling of number field
 widths in vsscanf

On Thu 2021-02-11 13:55:26, Petr Mladek wrote:
> On Mon 2021-02-08 17:38:29, Richard Fitzgerald wrote:
> > On 08/02/2021 15:18, Andy Shevchenko wrote:
> > > On Mon, Feb 08, 2021 at 02:01:52PM +0000, Richard Fitzgerald wrote:
> > > A nit-pick: What if we rewrite above as
> > > 
> > > static unsigned long long simple_strntoull(const char *cp, size_t max_chars,
> > > 					   char **endp, unsigned int base)
> > > {
> > > 	unsigned long long result = 0ULL;
> > > 	const char *startp = cp;
> > > 	unsigned int rv;
> > > 	size_t chars;
> > > 
> > > 	cp = _parse_integer_fixup_radix(cp, &base);
> > > 	chars = cp - startp;
> > > 	if (chars >= max_chars) {
> > > 		/* We hit the limit */
> > > 		cp = startp + max_chars;
> > > 	} else {
> > > 		rv = _parse_integer_limit(cp, base, &result, max_chars - chars);
> > > 		/* FIXME */
> > > 		cp += (rv & ~KSTRTOX_OVERFLOW);
> > > 	}
> > > 
> > > 	if (endp)
> > > 		*endp = (char *)cp;
> > > 
> > > 	return result;
> > > }
> > > 
> > > ...
> > 
> > 
> > I don't mind rewriting that code if you prefer that way.
> > I am used to working on other kernel subsytems where the preference is
> > to bail out on the error case so that the "normal" case flows without
> > nesting.
> 
> Yeah. But in this case Andy's variant looks slightly better redable to me.


> ...
> 
> > > 
> > > > +			val.s = simple_strntoll(str,
> > > > +						field_width > 0 ? field_width : SIZE_MAX,
> > > > +						&next, base);
> > > 
> > > is? Also, is field_width == 0 should be treated as "parse to the MAX"?
> > 
> > Earlier code terminates scanning if the width parsed from the format
> > string is <= 0.
> 
> > So field_width can only be -1 or > 0 here. But now you
> > point it out, that test would be better as field_width >= 0 ... so
> > it deals with 0 if it ever happened to sneak through to here
> > somehow.
> 
> It might make sense to be proactive and change it to >= 0.
> But I would do it in a separate patch. The "< 0" condition
> matches the original code.

Ah, I have missed that you have already sent v6 where you did this change
in the same patch. There is no need to resend it just because of this.
I am going to look at v6.

Best Regards,
Petr