linux-kernel - Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

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: <20200224095501.ds7pbjwj2izmcvus@pathway.suse.cz>
Date:   Mon, 24 Feb 2020 10:55:01 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:     Ilya Dryomov <idryomov@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Jonathan Corbet <corbet@....net>,
        Kees Cook <keescook@...omium.org>,
        "Tobin C . Harding" <me@...in.cc>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Linux Documentation List <linux-doc@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

On Sat 2020-02-22 00:52:27, Rasmus Villemoes wrote:
> On 21/02/2020 14.05, Petr Mladek wrote:
> > On Thu 2020-02-20 16:02:48, Ilya Dryomov wrote:
> 
> >> I would like to see it in 5.6, so that it is backported to 5.4 and 5.5.
> > 
> Sorry to be that guy, but yes, I'm against changing the behavior of
> vsnprintf() without at least some test(s) added to the test suite - the
> lack of machine-checked documentation in the form of tests is what led
> to that regression in the first place.

I would not call this regression. It was intentional. The change in
5.2 unified the behavior for the other %p? modifiers. I simply did
not care about plain %p because it was already crippled by the hashing.

I am fine with the proposed change. But the more I think about it
the less I want to rush it in for 5.6. The proposed patch changes
the behavior again:

Value           Output v5.1       Output v5.2      Proposal

NULL                       (null) 00000000<.hash.> 0000000000000000
fffffffffffffffe 00000000<.hash.> 00000000<.hash.> fffffffffffffffe
ffffffff12345678 00000000<.hash.> 00000000<.hash.> 00000000<.hash.>

I do not want to change this in rc phase. I would really like to wait
for 5.7.


> But I agree that there's no point adding another helper function and
> muddying the test suite even more (especially as the name error_pointer
> is too close to the name errptr() I chose a few months back for the %pe).
> 
> So how about
> 
> - remove the now stale test_hashed("%p", NULL); from null_pointer()
> - add tests of "%p", NULL and "%p", ERR_PTR(-123) to plain()
> 
> and we save testing the "%px" case for when we figure out a good name
> for a helper for that (explicit_pointer? pointer_as_hex?)

In this, case I would prefer to fix the tests properly first. There
have been only few commits in lib/test_printf.c since 5.2. And they
should not conflict with the changes proposed at
https://lkml.kernel.org/r/20200220125707.hbcox3xgevpezq4l@pathway.suse.cz
So it should be easy to backport as well.

If you want, I could sent the cleanup patch properly for review.

Best Regards,
Petr