linux-kernel - Re: [PATCH v2 03/28] vsprintf: %pf(%p)

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:   Fri, 20 May 2022 00:49:24 -0400
From:   Kent Overstreet <kent.overstreet@...il.com>
To:     David Laight <David.Laight@...LAB.COM>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mm@...r.kernel.org" <linux-mm@...r.kernel.org>,
        "pmladek@...e.com" <pmladek@...e.com>,
        "rostedt@...dmis.org" <rostedt@...dmis.org>,
        "senozhatsky@...omium.org" <senozhatsky@...omium.org>,
        "andriy.shevchenko@...ux.intel.com" 
        <andriy.shevchenko@...ux.intel.com>,
        "willy@...radead.org" <willy@...radead.org>
Subject: Re: [PATCH v2 03/28] vsprintf: %pf(%p)

On Thu, May 19, 2022 at 09:06:24PM +0000, David Laight wrote:
> From: Kent Overstreet
> > Sent: 19 May 2022 18:24
> > 
> > This implements two new format strings: both do the same thing, one more
> > compatible with current gcc format string checking, the other that we'd
> > like to standardize:
> > 
> >  %p(%p) - more compatible
> >  %(%p)  - more prettier
> > 
> > Both can take variable numbers of arguments, i.e. %(%p,%p,%p).
> > 
> > They're used to indicate that snprintf or pr_buf should interpret the
> > next argument as a pretty-printer function to call, and subsequent
> > arguments within the parentheses should be passed to the pretty-printer.
> 
> I suspect this a very good way to blow the kernel stack.
> The highest stack use is already very likely to be inside
> the printf code in an error path somewhere.

By getting rid of stack allocated buffers, I've been _reducing_ stack usage.
Also, the new printbuf calling convention reduces stack usage as well.

It's true that we'll want to keep the stack usage of pr_buf -> pretty printer ->
pr_buf again minimal, but I don't see any difficulties there the way the code is
structured now. 

> 
> ...
> > The goal is to replace most of our %p format extensions with this
> > interface, and to move pretty-printers out of the core vsprintf.c code -
> 
> One advantage of the current scheme is that is reasonably safe
> and easy to use.
> Perhaps too many extra formats have been added recently.
> This all seems like a recipe for disaster with functions being
> called with the wrong number of parameters
> (I can't think how you can compile-time check it).

We can't check it at compile time yet, it's true - printf format checking will
need to be extended. But we're already talking about doing that.

> Double copying using a temporary buffer isn't the end of the world.
> It is only a problem because pr_cont() is basically impossible.
> But since kernel printf ought to be formatted to reasonable
> line length that isn't really an issue.
> printf() is expensive an extra memory copy is probably noise.
> 
> ...
> > Currently, we can only call pretty printers with pointer arguments. This
> > could be changed to also allow at least integer arguments in the future
> > by using libffi.
> 
> I'm sure I remember something else trying to use that.
> IIRC it is basically broken by design.

Hmm? libffi is the standard for calling C from a lot of languages. If it's
broken by design, that's some real news. And it does constructed function calls,
which is exactly what we need here.