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] [day] [month] [year] [list]

Message-ID: <YCF7N0Fc9WC9flyd@alley>
Date:   Mon, 8 Feb 2021 18:56:07 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Richard Fitzgerald <rf@...nsource.cirrus.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Shuah Khan <shuah@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <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 Fri 2021-02-05 14:50:56, Andy Shevchenko wrote:
> On Fri, Feb 5, 2021 at 1:35 PM Richard Fitzgerald
> <rf@...nsource.cirrus.com> wrote:
> > On 04/02/2021 16:35, Petr Mladek wrote:
> > > On Wed 2021-02-03 21:45:55, Andy Shevchenko wrote:
> > >> On Wed, Feb 03, 2021 at 04:50:07PM +0000, Richard Fitzgerald wrote:
> > >> 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.
> >
> > I originally had UINT_MAX and changed on Petr's request to be
> > consistent with other code. (Sorry Andy - my mistake not including
> > you on the earlier review versions).
> >
> > But 0 < INT_MAX < UINT_MAX, so ok to pass to an unsigned. And as Petr
> > said on his original review, INT_MAX is "big enough".
> 
> Some code has INT_MAX, some has UINT_MAX, while the parameter is size_t.

Yeah, if I remember correctly I wanted to have INT_MAX everywhere but
I did not want to nitpick about it in the later versions. It looked
like an arbitrary number anyway.

> I think all of these inconsistencies should have a comment either in
> the code, or in the commit message, or in the cover letter (depending
> on the importance).
> Or being fixed to be more consistent with existing code. Whichever you
> consider better.

OK, you made me to do some archaeology. The INT_MAX limit has
been added into vsnprintf() in 2.6.2 by the commit:

Author: Linus Torvalds <torvalds@...e.osdl.org>
Date:   Mon Feb 2 21:17:29 2004 -0800

    Warn loudly if somebody passes a negative value as
    the size to "vsnprintf()".

    That's a pretty clear case of overflow.


It might catch problems. And the limit seems to have worked all the time.

IMHO, it would make sense to have INT_MAX limit also in
_parse_integer_limit() and WARN() when a larger value is passed.

By other words, it would mean to add this check and use INT_MAX
everywhere in this patch.

Best Regards,
Petr