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

Date:   Wed, 19 Feb 2020 15:45:58 +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-doc@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

On Wed 2020-02-19 14:56:32, Rasmus Villemoes wrote:
> On 19/02/2020 14.48, Petr Mladek wrote:
> > On Wed 2020-02-19 12:53:22, Rasmus Villemoes wrote:
> >> --- a/lib/vsprintf.c
> >> +++ b/lib/vsprintf.c
> > The test should go into null_pointer() instead of errptr().
> 
> Eh, no, the behaviour of %pe is tested by errptr(). I'll keep it that
> way. But I should add a #else section that tests how %pe behaves without
> CONFIG_SYMBOLIC_ERRNAME - though that's orthogonal to this patch.

OK, we should agree on some structure first.

We already have two top level functions that test how a particular
pointer is printed using different pointer modifiers:

	null_pointer();     -> NULL with %p, %pX, %pE
	invalid_pointer();  -> random pointer with %p, %pX, %pE

Following this logic, errptr() should test how a pointer from IS_ERR() range
is printed using different pointer formats.

I am open to crate another logic but it must be consistent.
If you want to check %pe with NULL in errptr(), you have to
split the other two functions per-modifier. IMHO, it is not
worth it.

Sigh, I should have been more strict[*]. The function should have been
called err_ptr() and located right below null_pointer().

[*] I am still trying to find a right balance between preventing
nitpicking, bikeshedding, enforcing my style, and creating a mess.

> > Could you send updated patch, please? ;-)
> 
> I'll wait a day or two for more comments. It doesn't seem very urgent.

Sure.

> >> BTW., your original patch for %p lacks corresponding update of
> >> test_vsprintf.c. Please add appropriate test cases.
> > 
> > diff --git a/lib/test_printf.c b/lib/test_printf.c
> > index 2d9f520d2f27..1726a678bccd 100644
> > --- a/lib/test_printf.c
> > +++ b/lib/test_printf.c
> > @@ -333,7 +333,7 @@ test_hashed(const char *fmt, const void *p)
> >  static void __init
> >  null_pointer(void)
> >  {
> > -	test_hashed("%p", NULL);
> > +	test(ZEROS "00000000", "%p", NULL);
> 
> No, it most certainly also needs to check a few "%p", ERR_PTR(-4) cases
> (where one of course has to use explicit integers and not E* constants).

Yes, it would be great to add checks for %p, %px for IS_ERR() range.
But it is different story. The above change is for the original patch
and it was about NULL pointer handling.

Best Regards,
Petr