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]

Message-ID: <5459eb50-48e2-2fd9-3560-0bc921e3678c@rasmusvillemoes.dk>
Date:   Wed, 19 Feb 2020 14:56:32 +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 14.48, Petr Mladek wrote:
> On Wed 2020-02-19 12:53:22, Rasmus Villemoes wrote:
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -619,7 +619,7 @@ static char *err_ptr(char *buf, char *end, void *ptr,
>>                      struct printf_spec spec)
>>  {
>>         int err = PTR_ERR(ptr);
>> -       const char *sym = errname(err);
>> +       const char *sym = err ? errname(err) : "NULL";
> 
> I like this more than adding "NULL" errname.

OK.

>>         if (sym)
>>                 return string_nocheck(buf, end, sym, spec);
>>
>> instead of the change(s) in errname.c? And then the test case for
>> '"%pe", NULL' should also be moved outside CONFIG_SYMBOLIC_ERRNAME.
> 
> 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.

> Could you send updated patch, please? ;-)

I'll wait a day or two for more comments. It doesn't seem very urgent.

>> BTW., your original patch for %p lacks corresponding update of
>> test_vsprintf.c. Please add appropriate test cases.
> 
> Good point. The existing test_hashed() is rather weak
> and it did not catch this change.
> 
> It would be nice to make test_hash() more powerful.
> Anyway, the minimal udpate would be:
> 
> 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).

Rasmus