Date:   Thu, 20 Feb 2020 16:02:48 +0100
From:   Ilya Dryomov <idryomov@...il.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Rasmus Villemoes <linux@...musvillemoes.dk>,
        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 Documentation List <linux-doc@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] vsprintf: sanely handle NULL passed to %pe

On Thu, Feb 20, 2020 at 1:57 PM Petr Mladek <pmladek@...e.com> wrote:
>
> On Wed 2020-02-19 16:40:08, Rasmus Villemoes wrote:
> > 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?
>
> I see, I was the one who broke the style. Please, find below a patch
> that tries to fix it. If you agree with the approach then I could
> split it into smaller steps.
>
> Also it would make sense to add checks for NULL and ERR pointer
> into each existing %p modifier check. It will make sure that
> check_pointer() is called in all handlers.
>
>
> > So yeah, I'm going to continue testing the behaviour of %pe in errptr, TYVM.
>
> OK.
>
> > >>>> 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.
>
> Grr, I see. I was too fast yesterday. OK, I suggest to fix the
> structure of the tests first. All these patches are for 5.7
> anyway.

My patch fixes a regression introduced by 3e5903eb9cff ("vsprintf:
Prevent crash when dereferencing invalid pointers" in 5.2, which
made debugging based on existing pr_debugs (used extensively in some
subsystems) very annoying.

I would like to see it in 5.6, so that it is backported to 5.4 and 5.5.

>
>
> Here is the proposed clean up:
>
> From 855909f2a1945d3a5bf490ddf4f2cca775ef967b Mon Sep 17 00:00:00 2001
> From: Petr Mladek <pmladek@...e.com>
> Date: Thu, 20 Feb 2020 12:53:43 +0100
> Subject: [PATCH] lib/test_printf: Clean up basic pointer testing
>
> The pointer testing has been originally split by the %p modifiers,
> for example, the function dentry() tested %pd and %pD handling.
>
> There were recently added tests that do not really fit into
> the existing structure, namely:
>
>   + hashed pointers tested by a maze of functions
>   + null and invalid pointer handling with various modifiers
>
> The hash pointer test is really special because the hash depends
> on a random key that is generated during boot. Though, it is
> still possible to check some aspects:
>
>   + output string length
>   + hash differs from the original pointer value
>   + top half bites are zeroed on 64-bit systems
>
> Let's put all these checks into test_hashed() function that has
> the same behavior as the test() functions for well-defined output.
> It increments the number of tests and eventual failures. It prints
> warnings/errors when some aspects of the output are not as expected.
>
> Most of these checks were there even before. The only addition is
> the check whether hash differs from the original pointer value.
> There is a small chance of a false error. It might be reduced
> by checking more pointers but let's keep it simple for now.
>
> The existing null_pointer() and invalid_pointer() checks are
> newly split per-format modifier. And there is also fixed
> difference between invalid pointer in the IS_ERR() range
> and invalid pointer that looks like a valid one.
>
> The invalid pointer Oxdeaddead00000000 should work on most
> architectures. But I am not able to check it everywhere.
> So there is a small chance of a false error. It might get
> fixed when anyone reports a problem.
>
> Signed-off-by: Petr Mladek <pmladek@...e.com>
> ---
>  lib/test_printf.c | 162 ++++++++++++++++++++----------------------------------
>  1 file changed, 59 insertions(+), 103 deletions(-)
>
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 2d9f520d2f27..4e89b508def6 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -206,146 +206,101 @@ test_string(void)
>  }
>
>  #define PLAIN_BUF_SIZE 64      /* leave some space so we don't oops */
> +#define PTR_ERROR ERR_PTR(-EFAULT)
> +#define PTR_VAL_ERROR "fffffff2"
>
>  #if BITS_PER_LONG == 64
>
>  #define PTR_WIDTH 16
>  #define PTR ((void *)0xffff0123456789abUL)
>  #define PTR_STR "ffff0123456789ab"
> +#define PTR_INVALID ((void *)0xdeaddead000000ab)
> +#define PTR_VAL_INVALID "deaddead000000ab"
>  #define PTR_VAL_NO_CRNG "(____ptrval____)"
> +#define ONES "ffffffff"                /* hex 32 one bits */
>  #define ZEROS "00000000"       /* hex 32 zero bits */
>
> -static int __init
> -plain_format(void)
> -{
> -       char buf[PLAIN_BUF_SIZE];
> -       int nchars;
> -
> -       nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR);
> -
> -       if (nchars != PTR_WIDTH)
> -               return -1;
> -
> -       if (strncmp(buf, PTR_VAL_NO_CRNG, PTR_WIDTH) == 0) {
> -               pr_warn("crng possibly not yet initialized. plain 'p' buffer contains \"%s\"",
> -                       PTR_VAL_NO_CRNG);
> -               return 0;
> -       }
> -
> -       if (strncmp(buf, ZEROS, strlen(ZEROS)) != 0)
> -               return -1;
> -
> -       return 0;
> -}
> -
>  #else
>
>  #define PTR_WIDTH 8
>  #define PTR ((void *)0x456789ab)
>  #define PTR_STR "456789ab"
> +#define PTR_INVALID ((void *)0x000000ab)
> +#define PTR_VAL_INVALID "000000ab"
>  #define PTR_VAL_NO_CRNG "(ptrval)"
> +#define ONES ""
>  #define ZEROS ""
>
> -static int __init
> -plain_format(void)
> -{
> -       /* Format is implicitly tested for 32 bit machines by plain_hash() */
> -       return 0;
> -}
> -
>  #endif /* BITS_PER_LONG == 64 */
>
> -static int __init
> -plain_hash_to_buffer(const void *p, char *buf, size_t len)
> +static void __init
> +test_hashed(const char *fmt, const void *p)
>  {
> +       char pointer[PLAIN_BUF_SIZE];
> +       char hash[PLAIN_BUF_SIZE];
>         int nchars;
>
> -       nchars = snprintf(buf, len, "%p", p);
> -
> -       if (nchars != PTR_WIDTH)
> -               return -1;
> +       total_tests++;
>
> -       if (strncmp(buf, PTR_VAL_NO_CRNG, PTR_WIDTH) == 0) {
> -               pr_warn("crng possibly not yet initialized. plain 'p' buffer contains \"%s\"",
> -                       PTR_VAL_NO_CRNG);
> -               return 0;
> +       nchars = snprintf(pointer, sizeof(pointer), "%px", p);
> +       if (nchars != PTR_WIDTH) {
> +               pr_err("error in test suite: vsprintf(\"%%px\", p) returned number of characters %d, expected %d\n",
> +                      nchars, PTR_WIDTH);
> +               goto err;
>         }
>
> -       return 0;
> -}
> -
> -static int __init
> -plain_hash(void)
> -{
> -       char buf[PLAIN_BUF_SIZE];
> -       int ret;
> -
> -       ret = plain_hash_to_buffer(PTR, buf, PLAIN_BUF_SIZE);
> -       if (ret)
> -               return ret;
> -
> -       if (strncmp(buf, PTR_STR, PTR_WIDTH) == 0)
> -               return -1;
> -
> -       return 0;
> -}
> -
> -/*
> - * We can't use test() to test %p because we don't know what output to expect
> - * after an address is hashed.
> - */
> -static void __init
> -plain(void)
> -{
> -       int err;
> -
> -       err = plain_hash();
> -       if (err) {
> -               pr_warn("plain 'p' does not appear to be hashed\n");
> -               failed_tests++;
> -               return;
> +       nchars = snprintf(hash, sizeof(hash), fmt, p);
> +       if (nchars != PTR_WIDTH) {
> +               pr_warn("vsprintf(\"%s\", p) returned number of characters %d, expected %d\n",
> +                       fmt, nchars, PTR_WIDTH);
> +               goto err;
>         }
>
> -       err = plain_format();
> -       if (err) {
> -               pr_warn("hashing plain 'p' has unexpected format\n");
> -               failed_tests++;
> +       if (strncmp(hash, PTR_VAL_NO_CRNG, PTR_WIDTH) == 0) {
> +               pr_warn_once("crng possibly not yet initialized. vsprinf(\"%s\", p) printed \"%s\"",
> +                            fmt, hash);
> +               total_tests--;
> +               return;
>         }
> -}
> -
> -static void __init
> -test_hashed(const char *fmt, const void *p)
> -{
> -       char buf[PLAIN_BUF_SIZE];
> -       int ret;
>
>         /*
> -        * No need to increase failed test counter since this is assumed
> -        * to be called after plain().
> +        * There is a small chance of a false negative on 32-bit systems
> +        * when the hash is the same as the pointer value.
>          */
> -       ret = plain_hash_to_buffer(p, buf, PLAIN_BUF_SIZE);
> -       if (ret)
> -               return;
> +       if (strncmp(hash, pointer, PTR_WIDTH) == 0) {
> +               pr_warn("vsprintf(\"%s\", p) returned %s, expected hashed pointer\n",
> +                       fmt, hash);
> +               goto err;
> +       }
> +
> +#if BITS_PER_LONG == 64
> +       if (strncmp(hash, ZEROS, PTR_WIDTH / 2) != 0) {
> +               pr_warn("vsprintf(\"%s\", p) returned %s, expected %s in the top half bits\n",
> +                       fmt, hash, ZEROS);
> +               goto err;
> +       }
> +#endif
> +       return;
>
> -       test(buf, fmt, p);
> +err:
> +       failed_tests++;
>  }
>
>  static void __init
> -null_pointer(void)
> +plain_pointer(void)
>  {
>         test_hashed("%p", NULL);
> -       test(ZEROS "00000000", "%px", NULL);
> -       test("(null)", "%pE", NULL);
> +       test_hashed("%p", PTR_ERROR);
> +       test_hashed("%p", PTR_INVALID);
>  }
>
> -#define PTR_INVALID ((void *)0x000000ab)
>
>  static void __init
> -invalid_pointer(void)
> +real_pointer(void)
>  {
> -       test_hashed("%p", PTR_INVALID);
> -       test(ZEROS "000000ab", "%px", PTR_INVALID);
> -       test("(efault)", "%pE", PTR_INVALID);
> +       test(ZEROS "00000000", "%px", NULL);
> +       test(ONES PTR_VAL_ERROR, "%px", PTR_ERROR);
> +       test(PTR_VAL_INVALID, "%px", PTR_INVALID);
>  }
>
>  static void __init
> @@ -372,6 +327,8 @@ addr(void)
>  static void __init
>  escaped_str(void)
>  {
> +       test("(null)", "%pE", NULL);
> +       test("(efault)", "%pE", PTR_ERROR);
>  }
>
>  static void __init
> @@ -458,9 +415,9 @@ dentry(void)
>         test("foo", "%pd2", &test_dentry[0]);
>
>         test("(null)", "%pd", NULL);
> -       test("(efault)", "%pd", PTR_INVALID);
> +       test("(efault)", "%pd", PTR_ERROR);
>         test("(null)", "%pD", NULL);
> -       test("(efault)", "%pD", PTR_INVALID);
> +       test("(efault)", "%pD", PTR_ERROR);
>
>         test("romeo", "%pd", &test_dentry[3]);
>         test("alfa/romeo", "%pd2", &test_dentry[3]);
> @@ -647,9 +604,8 @@ errptr(void)
>  static void __init
>  test_pointer(void)
>  {
> -       plain();
> -       null_pointer();
> -       invalid_pointer();
> +       plain_pointer();
> +       real_pointer();
>         symbol_ptr();
>         kernel_ptr();
>         struct_resource();

Please note that I sent v2 of my patch ("[PATCH v2] vsprintf: don't
obfuscate NULL and error pointers"), fixing null_pointer() and adding
error_pointer() test cases, which conflicts with this restructure.

Thanks,

                Ilya

Powered by blists - more mailing lists