linux-kernel - Re: [PATCH v4 3/9] vsprintf: Do not check address of well-known strings

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]

Date:   Mon, 9 Apr 2018 14:19:33 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Rasmus Villemoes <linux@...musvillemoes.dk>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        "Tobin C . Harding" <me@...in.cc>, Joe Perches <joe@...ches.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Michal Hocko <mhocko@...e.cz>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 3/9] vsprintf: Do not check address of well-known
 strings

On Sat 2018-04-07 17:12:35, Andy Shevchenko wrote:
> On Fri, 2018-04-06 at 11:15 +0200, Petr Mladek wrote:
> > On Thu 2018-04-05 15:30:51, Rasmus Villemoes wrote:
> > > On 2018-04-04 10:58, Petr Mladek wrote:
> > > > We are going to check the address using probe_kernel_address(). It
> > > > will
> > > > be more expensive and it does not make sense for well known
> > > > address.
> > > > 
> > > > This patch splits the string() function. The variant without the
> > > > check
> > > > is then used on locations that handle string constants or strings
> > > > defined
> > > > as local variables.
> > > > 
> > > > This patch does not change the existing behavior.
> > > 
> > > Please leave string() alone, except for moving the < PAGE_SIZE check
> > > to
> > > a new helper checked_string (feel free to find a better name), and
> > > use
> > > checked_string for handling %s and possibly the few other cases
> > > where
> > > we're passing a user-supplied pointer. That avoids cluttering the
> > > entire
> > > file with double-underscore calls, and e.g. in the %pO case, it's
> > > easier
> > > to understand why one uses two different *string() helpers if the
> > > name
> > > of one somehow conveys how it is different from the other.
> > 
> > I understand your reasoning. I thought about exactly this as well.
> > My problem is that string() will then be unsafe. It might be dangerous
> > when porting patches.
> 
> I agree with Rasmus, and your argument here from my point of view kinda
> weak. Are we really going to backport this patches? Why? We lived w/o
> them for a long time. What's changed now?

Someone might have out-of-tree patch that adds yet another format
specifier. It might call string() that checks for (null) now but
it it won't if we rename it as you suggest. People used to safe
string() might miss this when the patch is send upstream for
inclusion, ...


> > Is _string() really that bad? 
> 
> I would think so.

OK, I am going to use valid_string() instead of _string().
Feel free to suggest anything better. Just please no bike-shedding.

Best Regards,
Petr