[PATCH v2] vsprintf: don't obfuscate NULL and error pointers

[PATCH v2] vsprintf: don't obfuscate NULL and error pointers

[Ilya Dryomov]

I don't see what security concern is addressed by obfuscating NULL
and IS_ERR() error pointers, printed with %p/%pK.  Given the number
of sites where %p is used (over 10000) and the fact that NULL pointers
aren't uncommon, it probably wouldn't take long for an attacker to
find the hash that corresponds to 0.  Although harder, the same goes
for most common error values, such as -1, -2, -11, -14, etc.

The NULL part actually fixes a regression: NULL pointers weren't
obfuscated until commit 3e5903eb9cff ("vsprintf: Prevent crash when
dereferencing invalid pointers") which went into 5.2.  I'm tacking
the IS_ERR() part on here because error pointers won't leak kernel
addresses and printing them as pointers shouldn't be any different
from e.g. %d with PTR_ERR_OR_ZERO().  Obfuscating them just makes
debugging based on existing pr_debug and friends excruciating.

Note that the "always print 0's for %pK when kptr_restrict == 2"
behaviour which goes way back is left as is.

Example output with the patch applied:

                            ptr         error-ptr              NULL
%p:            0000000001f8cc5b  fffffffffffffff2  0000000000000000
%pK, kptr = 0: 0000000001f8cc5b  fffffffffffffff2  0000000000000000
%px:           ffff888048c04020  fffffffffffffff2  0000000000000000
%pK, kptr = 1: ffff888048c04020  fffffffffffffff2  0000000000000000
%pK, kptr = 2: 0000000000000000  0000000000000000  0000000000000000

Fixes: 3e5903eb9cff ("vsprintf: Prevent crash when dereferencing invalid pointers")
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>
Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 lib/test_printf.c | 19 ++++++++++++++++++-
 lib/vsprintf.c    |  7 +++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

v2:
- fix null_pointer() test case (it didn't catch the original
  regression because test_hashed() doesn't really test much)
  and add error_pointer() test case

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 2d9f520d2f27..d8f87f09b55c 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -214,6 +214,7 @@ test_string(void)
 #define PTR_STR "ffff0123456789ab"
 #define PTR_VAL_NO_CRNG "(____ptrval____)"
 #define ZEROS "00000000"	/* hex 32 zero bits */
+#define ONES "ffffffff"		/* hex 32 one bits */
 
 static int __init
 plain_format(void)
@@ -245,6 +246,7 @@ plain_format(void)
 #define PTR_STR "456789ab"
 #define PTR_VAL_NO_CRNG "(ptrval)"
 #define ZEROS ""
+#define ONES ""
 
 static int __init
 plain_format(void)
@@ -330,14 +332,28 @@ test_hashed(const char *fmt, const void *p)
 	test(buf, fmt, p);
 }
 
+/*
+ * NULL pointers aren't hashed.
+ */
 static void __init
 null_pointer(void)
 {
-	test_hashed("%p", NULL);
+	test(ZEROS "00000000", "%p", NULL);
 	test(ZEROS "00000000", "%px", NULL);
 	test("(null)", "%pE", NULL);
 }
 
+/*
+ * Error pointers aren't hashed.
+ */
+static void __init
+error_pointer(void)
+{
+	test(ONES "fffffff5", "%p", ERR_PTR(-EAGAIN));
+	test(ONES "fffffff5", "%px", ERR_PTR(-EAGAIN));
+	test("(efault)", "%pE", ERR_PTR(-EAGAIN));
+}
+
 #define PTR_INVALID ((void *)0x000000ab)
 
 static void __init
@@ -649,6 +665,7 @@ test_pointer(void)
 {
 	plain();
 	null_pointer();
+	error_pointer();
 	invalid_pointer();
 	symbol_ptr();
 	kernel_ptr();
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 7c488a1ce318..f0f0522cd5a7 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -794,6 +794,13 @@ static char *ptr_to_id(char *buf, char *end, const void *ptr,
 	unsigned long hashval;
 	int ret;
 
+	/*
+	 * Print the real pointer value for NULL and error pointers,
+	 * as they are not actual addresses.
+	 */
+	if (IS_ERR_OR_NULL(ptr))
+		return pointer_string(buf, end, ptr, spec);
+
 	/* When debugging early boot use non-cryptographically secure hash. */
 	if (unlikely(debug_boot_weak_hash)) {
 		hashval = hash_long((unsigned long)ptr, 32);
-- 
2.19.2

Re: [PATCH v2] vsprintf: don't obfuscate NULL and error pointers

[Andy Shevchenko]

On Wed, Feb 19, 2020 at 7:13 PM Ilya Dryomov <idryomov@gmail.com> wrote:
>
> I don't see what security concern is addressed by obfuscating NULL
> and IS_ERR() error pointers, printed with %p/%pK.  Given the number
> of sites where %p is used (over 10000) and the fact that NULL pointers
> aren't uncommon, it probably wouldn't take long for an attacker to
> find the hash that corresponds to 0.  Although harder, the same goes
> for most common error values, such as -1, -2, -11, -14, etc.
>
> The NULL part actually fixes a regression: NULL pointers weren't
> obfuscated until commit 3e5903eb9cff ("vsprintf: Prevent crash when
> dereferencing invalid pointers") which went into 5.2.  I'm tacking
> the IS_ERR() part on here because error pointers won't leak kernel
> addresses and printing them as pointers shouldn't be any different
> from e.g. %d with PTR_ERR_OR_ZERO().  Obfuscating them just makes
> debugging based on existing pr_debug and friends excruciating.
>
> Note that the "always print 0's for %pK when kptr_restrict == 2"
> behaviour which goes way back is left as is.
>
> Example output with the patch applied:
>
>                             ptr         error-ptr              NULL
> %p:            0000000001f8cc5b  fffffffffffffff2  0000000000000000
> %pK, kptr = 0: 0000000001f8cc5b  fffffffffffffff2  0000000000000000
> %px:           ffff888048c04020  fffffffffffffff2  0000000000000000
> %pK, kptr = 1: ffff888048c04020  fffffffffffffff2  0000000000000000
> %pK, kptr = 2: 0000000000000000  0000000000000000  0000000000000000

...

> +/*
> + * NULL pointers aren't hashed.
> + */
>  static void __init
>  null_pointer(void)
>  {
> -       test_hashed("%p", NULL);
> +       test(ZEROS "00000000", "%p", NULL);
>         test(ZEROS "00000000", "%px", NULL);
>         test("(null)", "%pE", NULL);
>  }
>
> +/*
> + * Error pointers aren't hashed.
> + */
> +static void __init
> +error_pointer(void)
> +{
> +       test(ONES "fffffff5", "%p", ERR_PTR(-EAGAIN));
> +       test(ONES "fffffff5", "%px", ERR_PTR(-EAGAIN));

> +       test("(efault)", "%pE", ERR_PTR(-EAGAIN));

Hmm... Is capital E on purpose here?
Maybe we may use something else ('%ph'?) for sake of deviation?

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] vsprintf: don't obfuscate NULL and error pointers

[Ilya Dryomov]

On Wed, Feb 19, 2020 at 6:37 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Feb 19, 2020 at 7:13 PM Ilya Dryomov <idryomov@gmail.com> wrote:
> >
> > I don't see what security concern is addressed by obfuscating NULL
> > and IS_ERR() error pointers, printed with %p/%pK.  Given the number
> > of sites where %p is used (over 10000) and the fact that NULL pointers
> > aren't uncommon, it probably wouldn't take long for an attacker to
> > find the hash that corresponds to 0.  Although harder, the same goes
> > for most common error values, such as -1, -2, -11, -14, etc.
> >
> > The NULL part actually fixes a regression: NULL pointers weren't
> > obfuscated until commit 3e5903eb9cff ("vsprintf: Prevent crash when
> > dereferencing invalid pointers") which went into 5.2.  I'm tacking
> > the IS_ERR() part on here because error pointers won't leak kernel
> > addresses and printing them as pointers shouldn't be any different
> > from e.g. %d with PTR_ERR_OR_ZERO().  Obfuscating them just makes
> > debugging based on existing pr_debug and friends excruciating.
> >
> > Note that the "always print 0's for %pK when kptr_restrict == 2"
> > behaviour which goes way back is left as is.
> >
> > Example output with the patch applied:
> >
> >                             ptr         error-ptr              NULL
> > %p:            0000000001f8cc5b  fffffffffffffff2  0000000000000000
> > %pK, kptr = 0: 0000000001f8cc5b  fffffffffffffff2  0000000000000000
> > %px:           ffff888048c04020  fffffffffffffff2  0000000000000000
> > %pK, kptr = 1: ffff888048c04020  fffffffffffffff2  0000000000000000
> > %pK, kptr = 2: 0000000000000000  0000000000000000  0000000000000000
>
> ...
>
> > +/*
> > + * NULL pointers aren't hashed.
> > + */
> >  static void __init
> >  null_pointer(void)
> >  {
> > -       test_hashed("%p", NULL);
> > +       test(ZEROS "00000000", "%p", NULL);
> >         test(ZEROS "00000000", "%px", NULL);
> >         test("(null)", "%pE", NULL);
> >  }
> >
> > +/*
> > + * Error pointers aren't hashed.
> > + */
> > +static void __init
> > +error_pointer(void)
> > +{
> > +       test(ONES "fffffff5", "%p", ERR_PTR(-EAGAIN));
> > +       test(ONES "fffffff5", "%px", ERR_PTR(-EAGAIN));
>
> > +       test("(efault)", "%pE", ERR_PTR(-EAGAIN));
>
> Hmm... Is capital E on purpose here?

Yes.  It shows that for %pE an error pointer is still invalid.
%pe is tested separately, in errptr(), and the output would have
been "-EAGAIN".

> Maybe we may use something else ('%ph'?) for sake of deviation?

If you look at the resulting file, you will see that null_pointer(),
error_pointer() and invalid_pointer() exercise the same three variants:
%p, %px and %pE.

This is somewhat confusing, but there seems to be some disagreement
between Pavel and Rasmus as to how the test suite should be structured
and I didn't want to attempt to restructure anything in this patch.

Thanks,

                Ilya

Re: [PATCH v2] vsprintf: don't obfuscate NULL and error pointers

[Ilya Dryomov]

On Wed, Feb 19, 2020 at 7:07 PM Ilya Dryomov <idryomov@gmail.com> wrote:
>
> On Wed, Feb 19, 2020 at 6:37 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Wed, Feb 19, 2020 at 7:13 PM Ilya Dryomov <idryomov@gmail.com> wrote:
> > >
> > > I don't see what security concern is addressed by obfuscating NULL
> > > and IS_ERR() error pointers, printed with %p/%pK.  Given the number
> > > of sites where %p is used (over 10000) and the fact that NULL pointers
> > > aren't uncommon, it probably wouldn't take long for an attacker to
> > > find the hash that corresponds to 0.  Although harder, the same goes
> > > for most common error values, such as -1, -2, -11, -14, etc.
> > >
> > > The NULL part actually fixes a regression: NULL pointers weren't
> > > obfuscated until commit 3e5903eb9cff ("vsprintf: Prevent crash when
> > > dereferencing invalid pointers") which went into 5.2.  I'm tacking
> > > the IS_ERR() part on here because error pointers won't leak kernel
> > > addresses and printing them as pointers shouldn't be any different
> > > from e.g. %d with PTR_ERR_OR_ZERO().  Obfuscating them just makes
> > > debugging based on existing pr_debug and friends excruciating.
> > >
> > > Note that the "always print 0's for %pK when kptr_restrict == 2"
> > > behaviour which goes way back is left as is.
> > >
> > > Example output with the patch applied:
> > >
> > >                             ptr         error-ptr              NULL
> > > %p:            0000000001f8cc5b  fffffffffffffff2  0000000000000000
> > > %pK, kptr = 0: 0000000001f8cc5b  fffffffffffffff2  0000000000000000
> > > %px:           ffff888048c04020  fffffffffffffff2  0000000000000000
> > > %pK, kptr = 1: ffff888048c04020  fffffffffffffff2  0000000000000000
> > > %pK, kptr = 2: 0000000000000000  0000000000000000  0000000000000000
> >
> > ...
> >
> > > +/*
> > > + * NULL pointers aren't hashed.
> > > + */
> > >  static void __init
> > >  null_pointer(void)
> > >  {
> > > -       test_hashed("%p", NULL);
> > > +       test(ZEROS "00000000", "%p", NULL);
> > >         test(ZEROS "00000000", "%px", NULL);
> > >         test("(null)", "%pE", NULL);
> > >  }
> > >
> > > +/*
> > > + * Error pointers aren't hashed.
> > > + */
> > > +static void __init
> > > +error_pointer(void)
> > > +{
> > > +       test(ONES "fffffff5", "%p", ERR_PTR(-EAGAIN));
> > > +       test(ONES "fffffff5", "%px", ERR_PTR(-EAGAIN));
> >
> > > +       test("(efault)", "%pE", ERR_PTR(-EAGAIN));
> >
> > Hmm... Is capital E on purpose here?
>
> Yes.  It shows that for %pE an error pointer is still invalid.
> %pe is tested separately, in errptr(), and the output would have
> been "-EAGAIN".
>
> > Maybe we may use something else ('%ph'?) for sake of deviation?
>
> If you look at the resulting file, you will see that null_pointer(),
> error_pointer() and invalid_pointer() exercise the same three variants:
> %p, %px and %pE.
>
> This is somewhat confusing, but there seems to be some disagreement
> between Pavel and Rasmus as to how the test suite should be structured
> and I didn't want to attempt to restructure anything in this patch.

Sorry, I meant Petr of course.

Rasmus, who had to deal with mips defining EDQUOT to 1133 by special
casing that in lib/errname.c, reminded me that error codes are a mess:
EAGAIN is different on alpha.  Rather than picking another error code
that is the same on all architectures, let's just use explicit -11.

error_pointer() should be:

        test(ONES "fffffff5", "%p", ERR_PTR(-11));
        test(ONES "fffffff5", "%px", ERR_PTR(-11));
        test("(efault)", "%pE", ERR_PTR(-11));

I'll wait for more feedback and respin (or perhaps this can be
fixed up while applying).

Thanks,

                Ilya

Re: [PATCH v2] vsprintf: don't obfuscate NULL and error pointers

[Ilya Dryomov]

On Wed, Feb 19, 2020 at 8:23 PM Ilya Dryomov <idryomov@gmail.com> wrote:
>
> On Wed, Feb 19, 2020 at 7:07 PM Ilya Dryomov <idryomov@gmail.com> wrote:
> >
> > On Wed, Feb 19, 2020 at 6:37 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Wed, Feb 19, 2020 at 7:13 PM Ilya Dryomov <idryomov@gmail.com> wrote:
> > > >
> > > > I don't see what security concern is addressed by obfuscating NULL
> > > > and IS_ERR() error pointers, printed with %p/%pK.  Given the number
> > > > of sites where %p is used (over 10000) and the fact that NULL pointers
> > > > aren't uncommon, it probably wouldn't take long for an attacker to
> > > > find the hash that corresponds to 0.  Although harder, the same goes
> > > > for most common error values, such as -1, -2, -11, -14, etc.
> > > >
> > > > The NULL part actually fixes a regression: NULL pointers weren't
> > > > obfuscated until commit 3e5903eb9cff ("vsprintf: Prevent crash when
> > > > dereferencing invalid pointers") which went into 5.2.  I'm tacking
> > > > the IS_ERR() part on here because error pointers won't leak kernel
> > > > addresses and printing them as pointers shouldn't be any different
> > > > from e.g. %d with PTR_ERR_OR_ZERO().  Obfuscating them just makes
> > > > debugging based on existing pr_debug and friends excruciating.
> > > >
> > > > Note that the "always print 0's for %pK when kptr_restrict == 2"
> > > > behaviour which goes way back is left as is.
> > > >
> > > > Example output with the patch applied:
> > > >
> > > >                             ptr         error-ptr              NULL
> > > > %p:            0000000001f8cc5b  fffffffffffffff2  0000000000000000
> > > > %pK, kptr = 0: 0000000001f8cc5b  fffffffffffffff2  0000000000000000
> > > > %px:           ffff888048c04020  fffffffffffffff2  0000000000000000
> > > > %pK, kptr = 1: ffff888048c04020  fffffffffffffff2  0000000000000000
> > > > %pK, kptr = 2: 0000000000000000  0000000000000000  0000000000000000
> > >
> > > ...
> > >
> > > > +/*
> > > > + * NULL pointers aren't hashed.
> > > > + */
> > > >  static void __init
> > > >  null_pointer(void)
> > > >  {
> > > > -       test_hashed("%p", NULL);
> > > > +       test(ZEROS "00000000", "%p", NULL);
> > > >         test(ZEROS "00000000", "%px", NULL);
> > > >         test("(null)", "%pE", NULL);
> > > >  }
> > > >
> > > > +/*
> > > > + * Error pointers aren't hashed.
> > > > + */
> > > > +static void __init
> > > > +error_pointer(void)
> > > > +{
> > > > +       test(ONES "fffffff5", "%p", ERR_PTR(-EAGAIN));
> > > > +       test(ONES "fffffff5", "%px", ERR_PTR(-EAGAIN));
> > >
> > > > +       test("(efault)", "%pE", ERR_PTR(-EAGAIN));
> > >
> > > Hmm... Is capital E on purpose here?
> >
> > Yes.  It shows that for %pE an error pointer is still invalid.
> > %pe is tested separately, in errptr(), and the output would have
> > been "-EAGAIN".
> >
> > > Maybe we may use something else ('%ph'?) for sake of deviation?
> >
> > If you look at the resulting file, you will see that null_pointer(),
> > error_pointer() and invalid_pointer() exercise the same three variants:
> > %p, %px and %pE.
> >
> > This is somewhat confusing, but there seems to be some disagreement
> > between Pavel and Rasmus as to how the test suite should be structured
> > and I didn't want to attempt to restructure anything in this patch.
>
> Sorry, I meant Petr of course.
>
> Rasmus, who had to deal with mips defining EDQUOT to 1133 by special
> casing that in lib/errname.c, reminded me that error codes are a mess:
> EAGAIN is different on alpha.  Rather than picking another error code
> that is the same on all architectures, let's just use explicit -11.
>
> error_pointer() should be:
>
>         test(ONES "fffffff5", "%p", ERR_PTR(-11));
>         test(ONES "fffffff5", "%px", ERR_PTR(-11));
>         test("(efault)", "%pE", ERR_PTR(-11));
>
> I'll wait for more feedback and respin (or perhaps this can be
> fixed up while applying).

Hi Petr,

Bump, as I don't see this in linux-next or other public branches.
The discussion was split between several threads, revolving around
the vision for how lib/test_printf.c should be structured, but the
fix itself wasn't disputed.

Could you please pick it up for 5.7-rc1?  If you want to restructure
the test suite before adding any new test cases, v1 doesn't have them.
Other than the test cases, the only difference between v1 and v2 is
added reviews and acks.

Thanks,

                Ilya

Re: [PATCH v2] vsprintf: don't obfuscate NULL and error pointers

[Andy Shevchenko]

On Wed, Apr 8, 2020 at 12:45 AM Ilya Dryomov <idryomov@gmail.com> wrote:
> On Wed, Feb 19, 2020 at 8:23 PM Ilya Dryomov <idryomov@gmail.com> wrote:

> Hi Petr,
>
> Bump, as I don't see this in linux-next or other public branches.
> The discussion was split between several threads, revolving around
> the vision for how lib/test_printf.c should be structured, but the
> fix itself wasn't disputed.
>
> Could you please pick it up for 5.7-rc1?  If you want to restructure
> the test suite before adding any new test cases, v1 doesn't have them.
> Other than the test cases, the only difference between v1 and v2 is
> added reviews and acks.

Petr has some obstacles that prevent him to pay attention on this and
actually do anything right now for Linux kernel.
If Rasmus, Sergey, you and maybe others will got consensus here, I
think Andrew can take it thru his tree.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] vsprintf: don't obfuscate NULL and error pointers

[Ilya Dryomov]

On Wed, Apr 8, 2020 at 12:31 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Apr 8, 2020 at 12:45 AM Ilya Dryomov <idryomov@gmail.com> wrote:
> > On Wed, Feb 19, 2020 at 8:23 PM Ilya Dryomov <idryomov@gmail.com> wrote:
>
> > Hi Petr,
> >
> > Bump, as I don't see this in linux-next or other public branches.
> > The discussion was split between several threads, revolving around
> > the vision for how lib/test_printf.c should be structured, but the
> > fix itself wasn't disputed.
> >
> > Could you please pick it up for 5.7-rc1?  If you want to restructure
> > the test suite before adding any new test cases, v1 doesn't have them.
> > Other than the test cases, the only difference between v1 and v2 is
> > added reviews and acks.
>
> Petr has some obstacles that prevent him to pay attention on this and
> actually do anything right now for Linux kernel.
> If Rasmus, Sergey, you and maybe others will got consensus here, I
> think Andrew can take it thru his tree.

Thanks for letting us know, Andy.

Other than Petr, two people are listed as maintainers of vsprintf.c:
Sergey and Steve.  Both of them were fine with v1 (no tests) and sent
their acks.

Rasmus, you wanted to see some tests, so I posted v2.  However Petr
wanted to restructure the test suite first, and it didn't seem like you
him agreed on what exactly that should look like.  Which would you
prefer to be picked by Andrew, v1 or v2?

                Ilya

Re: [PATCH v2] vsprintf: don't obfuscate NULL and error pointers

[Andy Shevchenko]

On Wed, Apr 08, 2020 at 04:38:25PM +0200, Ilya Dryomov wrote:
> On Wed, Apr 8, 2020 at 12:31 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Wed, Apr 8, 2020 at 12:45 AM Ilya Dryomov <idryomov@gmail.com> wrote:
> > > On Wed, Feb 19, 2020 at 8:23 PM Ilya Dryomov <idryomov@gmail.com> wrote:
> >
> > > Hi Petr,
> > >
> > > Bump, as I don't see this in linux-next or other public branches.
> > > The discussion was split between several threads, revolving around
> > > the vision for how lib/test_printf.c should be structured, but the
> > > fix itself wasn't disputed.
> > >
> > > Could you please pick it up for 5.7-rc1?  If you want to restructure
> > > the test suite before adding any new test cases, v1 doesn't have them.
> > > Other than the test cases, the only difference between v1 and v2 is
> > > added reviews and acks.
> >
> > Petr has some obstacles that prevent him to pay attention on this and
> > actually do anything right now for Linux kernel.
> > If Rasmus, Sergey, you and maybe others will got consensus here, I
> > think Andrew can take it thru his tree.
> 
> Thanks for letting us know, Andy.
> 
> Other than Petr, two people are listed as maintainers of vsprintf.c:
> Sergey and Steve.  Both of them were fine with v1 (no tests) and sent
> their acks.
> 
> Rasmus, you wanted to see some tests, so I posted v2.  However Petr
> wanted to restructure the test suite first, and it didn't seem like you
> him agreed on what exactly that should look like.  Which would you
> prefer to be picked by Andrew, v1 or v2?

I think it's question to maintainers. I'm a reviewer here.

For the better understanding the current state of affairs I suggest to respin
new version after rc1 and we will see again.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] vsprintf: don't obfuscate NULL and error pointers

[Ilya Dryomov]

On Wed, Apr 8, 2020 at 5:06 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Apr 08, 2020 at 04:38:25PM +0200, Ilya Dryomov wrote:
> > On Wed, Apr 8, 2020 at 12:31 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Wed, Apr 8, 2020 at 12:45 AM Ilya Dryomov <idryomov@gmail.com> wrote:
> > > > On Wed, Feb 19, 2020 at 8:23 PM Ilya Dryomov <idryomov@gmail.com> wrote:
> > >
> > > > Hi Petr,
> > > >
> > > > Bump, as I don't see this in linux-next or other public branches.
> > > > The discussion was split between several threads, revolving around
> > > > the vision for how lib/test_printf.c should be structured, but the
> > > > fix itself wasn't disputed.
> > > >
> > > > Could you please pick it up for 5.7-rc1?  If you want to restructure
> > > > the test suite before adding any new test cases, v1 doesn't have them.
> > > > Other than the test cases, the only difference between v1 and v2 is
> > > > added reviews and acks.
> > >
> > > Petr has some obstacles that prevent him to pay attention on this and
> > > actually do anything right now for Linux kernel.
> > > If Rasmus, Sergey, you and maybe others will got consensus here, I
> > > think Andrew can take it thru his tree.
> >
> > Thanks for letting us know, Andy.
> >
> > Other than Petr, two people are listed as maintainers of vsprintf.c:
> > Sergey and Steve.  Both of them were fine with v1 (no tests) and sent
> > their acks.
> >
> > Rasmus, you wanted to see some tests, so I posted v2.  However Petr
> > wanted to restructure the test suite first, and it didn't seem like you
> > him agreed on what exactly that should look like.  Which would you
> > prefer to be picked by Andrew, v1 or v2?
>
> I think it's question to maintainers. I'm a reviewer here.

This question was directed at Rasmus.  Technically he is also a
reviewer, but I wanted to hear from him because he authored the test
suite in question.

Sergey and Steve (who are a listed as maintainers) were OK with v1
which doesn't touch lib/test_printf.c.

Thanks,

                Ilya

Re: [PATCH v2] vsprintf: don't obfuscate NULL and error pointers

[Linus Torvalds]

On Wed, Apr 8, 2020 at 8:08 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> For the better understanding the current state of affairs I suggest to respin
> new version after rc1 and we will see again.

For something simple and straightforward like the obfuscation patches,
I can take them directly if people are unavailable.

Just make sure to re-post the patches, and make it clear that it's to
me (because usually if I'm cc'd I end up just reading and then
ignoring things that I expect to go through a maintainer).

              Linus

RE: [PATCH v2] vsprintf: don't obfuscate NULL and error pointers

[David Laight]

From: Linus Torvalds
> Sent: 08 April 2020 17:23
> On Wed, Apr 8, 2020 at 8:08 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > For the better understanding the current state of affairs I suggest to respin
> > new version after rc1 and we will see again.

If the hash of a pointer ends up being one of the values that
wouldn't be hashed, perhaps it should be hashed again.
That will remove any possible confusion.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2] vsprintf: don't obfuscate NULL and error pointers

[Ilya Dryomov]

On Fri, Apr 10, 2020 at 11:40 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Linus Torvalds
> > Sent: 08 April 2020 17:23
> > On Wed, Apr 8, 2020 at 8:08 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > For the better understanding the current state of affairs I suggest to respin
> > > new version after rc1 and we will see again.
>
> If the hash of a pointer ends up being one of the values that
> wouldn't be hashed, perhaps it should be hashed again.
> That will remove any possible confusion.

Hi David,

That seems sensible, but out of scope of this patch IMO.

My goal was to fix a 5.2 regression for NULL pointers.  I tacked on
IS_ERR pointers because they aren't actual addresses either and hashing
them makes zero sense.  Although simple to implement, I'm afraid that
introducing additional rounds of hashing would drag this patch further
(it's already got bogged down in the test suite structure discussion).

Thanks,

                Ilya