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: <0fef2a1f-9391-43a9-32d5-2788ae96c529@rasmusvillemoes.dk>
Date:   Wed, 19 Feb 2020 12:53:22 +0100
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Ilya Dryomov <idryomov@...il.com>
Cc:     Petr Mladek <pmladek@...e.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 12.20, Ilya Dryomov wrote:
> On Wed, Feb 19, 2020 at 9:21 AM Rasmus Villemoes
> <linux@...musvillemoes.dk> wrote:
>>
>> Extend %pe to pretty-print NULL in addition to ERR_PTRs,
>> i.e. everything IS_ERR_OR_NULL().
>>
>> Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
>> Signed-off-by: Rasmus Villemoes <linux@...musvillemoes.dk>
>> ---
>> Something like this? The actual code change is +2,-1 with another +1
>> for a test case.
>>
>>  Documentation/core-api/printk-formats.rst | 9 +++++----
>>  lib/errname.c                             | 4 ++++
>>  lib/test_printf.c                         | 1 +
>>  lib/vsprintf.c                            | 4 ++--
>>  4 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
>> index 8ebe46b1af39..964b55291445 100644
>> --- a/Documentation/core-api/printk-formats.rst
>> +++ b/Documentation/core-api/printk-formats.rst
>> @@ -86,10 +86,11 @@ Error Pointers
>>
>>         %pe     -ENOSPC
>>
>> -For printing error pointers (i.e. a pointer for which IS_ERR() is true)
>> -as a symbolic error name. Error values for which no symbolic name is
>> -known are printed in decimal, while a non-ERR_PTR passed as the
>> -argument to %pe gets treated as ordinary %p.
>> +For printing error pointers (i.e. a pointer for which IS_ERR() is
>> +true) as a symbolic error name. Error values for which no symbolic
>> +name is known are printed in decimal. A NULL pointer is printed as
>> +NULL. All other pointer values (i.e. anything !IS_ERR_OR_NULL()) get
>> +treated as ordinary %p.
>>
>>  Symbols/Function Pointers
>>  -------------------------
>> diff --git a/lib/errname.c b/lib/errname.c
>> index 0c4d3e66170e..7757bc00f564 100644
>> --- a/lib/errname.c
>> +++ b/lib/errname.c
>> @@ -11,9 +11,13 @@
>>   * allocated errnos (with EHWPOISON = 257 on parisc, and EDQUOT = 1133
>>   * on mips), so this wastes a bit of space on those - though we
>>   * special case the EDQUOT case.
>> + *
>> + * For the benefit of %pe being able to print any ERR_OR_NULL pointer
>> + * symbolically, 0 is also treated specially.
>>   */
>>  #define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = "-" #err
>>  static const char *names_0[] = {
>> +       [0] = "NULL",
>>         E(E2BIG),
>>         E(EACCES),
>>         E(EADDRINUSE),
>> diff --git a/lib/test_printf.c b/lib/test_printf.c
>> index 2d9f520d2f27..3a37d0e9e735 100644
>> --- a/lib/test_printf.c
>> +++ b/lib/test_printf.c
>> @@ -641,6 +641,7 @@ errptr(void)
>>         test("[-EIO    ]", "[%-8pe]", ERR_PTR(-EIO));
>>         test("[    -EIO]", "[%8pe]", ERR_PTR(-EIO));
>>         test("-EPROBE_DEFER", "%pe", ERR_PTR(-EPROBE_DEFER));
>> +       test("[NULL]", "[%pe]", NULL);
>>  #endif
>>  }
>>
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index 7c488a1ce318..b7118d78eb20 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -2247,8 +2247,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>>         case 'x':
>>                 return pointer_string(buf, end, ptr, spec);
>>         case 'e':
>> -               /* %pe with a non-ERR_PTR gets treated as plain %p */
>> -               if (!IS_ERR(ptr))
>> +               /* %pe with a non-ERR_OR_NULL ptr gets treated as plain %p */
>> +               if (!IS_ERR_OR_NULL(ptr))
>>                         break;
> 
> FWIW I was about to post a patch that just special cases NULL here.
> 
> I think changing errname() to return "NULL" for 0 is overkill.
> People will sooner or later discover that function and start using it
> in contexts that don't have anything to do with pointers.  Returning
> _some_ string for 0 (instead of NULL) makes it very close to standard
> strerror(), and "NULL" for 0 (i.e. success) seems rather odd.

I see what you mean, but I don't share your assumption that errname()
will ever grow callers other than the one in vsprintf.c. But I don't
have any strong opinion either way. Perhaps this on top of my patch

--- 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";

        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.

BTW., your original patch for %p lacks corresponding update of
test_vsprintf.c. Please add appropriate test cases.

Rasmus