| 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
| ||
|
Date: Wed, 19 Feb 2020 14:48:26 +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 12:53:22, Rasmus Villemoes wrote:
> 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";
I like this more than adding "NULL" errname.
> 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().
Could you send updated patch, please? ;-)
> 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);
test(ZEROS "00000000", "%px", NULL);
test("(null)", "%pE", NULL);
}
Best Regards,
Petr
Powered by blists - more mailing lists