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 16:40:08 +0100
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Petr Mladek <pmladek@...e.com>
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 19/02/2020 15.45, Petr Mladek wrote:
> 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.

Oh please. I wrote test_printf.c originally and structured it with one
helper for each %p<whatever>. How are your additions null_pointer and
invalid_pointer good examples for what the existing style is?

707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 649)
test_pointer(void)
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 650) {
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 651)  plain();
3e5903eb9cff7 (Petr Mladek      2019-04-17 13:53:48 +0200 652)
null_pointer();
3e5903eb9cff7 (Petr Mladek      2019-04-17 13:53:48 +0200 653)
invalid_pointer();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 654)
symbol_ptr();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 655)
kernel_ptr();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 656)
struct_resource();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 657)  addr();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 658)
escaped_str();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 659)
hex_string();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 660)  mac();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 661)  ip();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 662)  uuid();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 663)  dentry();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 664)
struct_va_format();
4d42c44727a06 (Andy Shevchenko  2018-12-04 23:23:11 +0200 665)
struct_rtc_time();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 666)
struct_clk();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 667)  bitmap();
707cc7280f452 (Rasmus Villemoes 2015-11-06 16:30:29 -0800 668)
netdev_features();
edf14cdbf9a0e (Vlastimil Babka  2016-03-15 14:55:56 -0700 669)  flags();
57f5677e535ba (Rasmus Villemoes 2019-10-15 21:07:05 +0200 670)  errptr();
f1ce39df508de (Sakari Ailus     2019-10-03 15:32:19 +0300 671)
fwnode_pointer();


> I am open to crate another logic but it must be consistent.

So yeah, I'm going to continue testing the behaviour of %pe in errptr, TYVM.

> 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.

Agreed, let's leave null_pointer and invalid_pointer alone.

>>>> 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.

Wrong. The original patch (i.e. Ilya's) had subject "vsprintf: don't
obfuscate NULL and error pointers" and did

+	if (IS_ERR_OR_NULL(ptr))

so the tests that should be part of that patch very much need to cover
both NULL and ERR_PTRs passed to plain %p.

Rasmus