Date:   Thu, 20 Feb 2020 13:57:07 +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 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.


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();
-- 
2.16.4

Powered by blists - more mailing lists