The discussion about hashing NULL pointer value[1] uncovered that
the basic tests of pointer formatting do not follow the original
structure and cause confusion.
I feel responsible for it and here is a proposed clean up.
[1] https://lkml.kernel.org/r/[email protected]
Petr Mladek (3):
lib/test_printf: Clean up test of hashed pointers
lib/test_printf: Fix structure of basic pointer tests
lib/test_printf: Clean up invalid pointer value testing
lib/test_printf.c | 170 ++++++++++++++++++++----------------------------------
1 file changed, 64 insertions(+), 106 deletions(-)
--
2.16.4
The commit ad67b74d2469d9b82a ("printk: hash addresses printed with %p")
helps to prevent leaking kernel addresses.
The testing of this functionality is a bit problematic because the output
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 bits are zeroed on 64-bit systems
This is currently done by a maze of functions:
+ It is hard to follow.
+ Some code is duplicated, e.g. the check for initialized crng.
+ The zeroed top half bits are tested only with one hardcoded PTR.
+ plain() increments "failed_tests" but not "total_tests".
+ The generic test_hashed() does not touch number of tests at all.
Move all the checks into test_hashed() so that they are done for
any given pointer that should get hashed. Also handle test counters
and internal errors the same way as the existing test() function.
Signed-off-by: Petr Mladek <[email protected]>
---
lib/test_printf.c | 130 ++++++++++++++++++------------------------------------
1 file changed, 42 insertions(+), 88 deletions(-)
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 2d9f520d2f27..6fa6fb606554 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -215,29 +215,6 @@ test_string(void)
#define PTR_VAL_NO_CRNG "(____ptrval____)"
#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
@@ -246,88 +223,65 @@ plain_format(void)
#define PTR_VAL_NO_CRNG "(ptrval)"
#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 real[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(real, sizeof(real), "%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;
+ 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_hash();
- if (err) {
- pr_warn("plain 'p' does not appear to be hashed\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;
}
- err = plain_format();
- if (err) {
- pr_warn("hashing plain 'p' has unexpected format\n");
- failed_tests++;
+ /*
+ * There is a small chance of a false negative on 32-bit systems
+ * when the hash is the same as the pointer value.
+ */
+ if (strncmp(hash, real, 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;
+
+err:
+ failed_tests++;
}
static void __init
-test_hashed(const char *fmt, const void *p)
+plain(void)
{
- char buf[PLAIN_BUF_SIZE];
- int ret;
-
- /*
- * No need to increase failed test counter since this is assumed
- * to be called after plain().
- */
- ret = plain_hash_to_buffer(p, buf, PLAIN_BUF_SIZE);
- if (ret)
- return;
-
- test(buf, fmt, p);
+ test_hashed("%p", PTR);
}
static void __init
--
2.16.4
PTR_INVALID is a confusing name. It might mean any pointer value
that is not accessible. But check_pointer() function is able to
detect only the obviously invalid pointers: NULL pointer,
IS_ERR() range, and the first page.
Check all these three categories by better named values.
Use PTR_STR prefix for all expected output, including
the string used when crng has not been initialized yet.
Signed-off-by: Petr Mladek <[email protected]>
---
lib/test_printf.c | 31 ++++++++++++++++++++-----------
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 1ee1bb703307..d640be78e3ae 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -206,13 +206,18 @@ test_string(void)
}
#define PLAIN_BUF_SIZE 64 /* leave some space so we don't oops */
+#define PTR_FIRST_PAGE ((void *)0x000000ab)
+#define PTR_STR_FIRST_PAGE "000000ab"
+#define PTR_ERROR ERR_PTR(-EFAULT)
+#define PTR_STR_ERROR "fffffff2"
#if BITS_PER_LONG == 64
#define PTR_WIDTH 16
#define PTR ((void *)0xffff0123456789abUL)
#define PTR_STR "ffff0123456789ab"
-#define PTR_VAL_NO_CRNG "(____ptrval____)"
+#define PTR_STR_NO_CRNG "(____ptrval____)"
+#define ONES "ffffffff" /* hex 32 one bits */
#define ZEROS "00000000" /* hex 32 zero bits */
#else
@@ -220,7 +225,8 @@ test_string(void)
#define PTR_WIDTH 8
#define PTR ((void *)0x456789ab)
#define PTR_STR "456789ab"
-#define PTR_VAL_NO_CRNG "(ptrval)"
+#define PTR_STR_NO_CRNG "(ptrval)"
+#define ONES ""
#define ZEROS ""
#endif /* BITS_PER_LONG == 64 */
@@ -248,7 +254,7 @@ test_hashed(const char *fmt, const void *p)
goto err;
}
- if (strncmp(hash, PTR_VAL_NO_CRNG, PTR_WIDTH) == 0) {
+ if (strncmp(hash, PTR_STR_NO_CRNG, PTR_WIDTH) == 0) {
pr_warn_once("crng possibly not yet initialized. vsprinf(\"%s\", p) printed \"%s\"",
fmt, hash);
total_tests--;
@@ -278,22 +284,22 @@ test_hashed(const char *fmt, const void *p)
failed_tests++;
}
-#define PTR_INVALID ((void *)0x000000ab)
-
static void __init
plain_pointer(void)
{
test_hashed("%p", PTR);
test_hashed("%p", NULL);
- test_hashed("%p", PTR_INVALID);
+ test_hashed("%p", PTR_ERROR);
+ test_hashed("%p", PTR_FIRST_PAGE);
}
static void __init
real_pointer(void)
{
test(PTR_STR, "%px", PTR);
- test(ZEROS "00000000", "%px", NULL);
- test(ZEROS "000000ab", "%px", PTR_INVALID);
+ test(ZEROS ZEROS, "%px", NULL);
+ test(ONES PTR_STR_ERROR, "%px", PTR_ERROR);
+ test(ZEROS PTR_STR_FIRST_PAGE, "%px", PTR_FIRST_PAGE);
}
static void __init
@@ -321,7 +327,8 @@ static void __init
escaped_str(void)
{
test("(null)", "%pE", NULL);
- test("(efault)", "%pE", PTR_INVALID);
+ test("(efault)", "%pE", PTR_ERROR);
+ test("(efault)", "%pE", PTR_FIRST_PAGE);
}
static void __init
@@ -408,9 +415,11 @@ dentry(void)
test("foo", "%pd2", &test_dentry[0]);
test("(null)", "%pd", NULL);
- test("(efault)", "%pd", PTR_INVALID);
+ test("(efault)", "%pd", PTR_ERROR);
+ test("(efault)", "%pd", PTR_FIRST_PAGE);
test("(null)", "%pD", NULL);
- test("(efault)", "%pD", PTR_INVALID);
+ test("(efault)", "%pD", PTR_ERROR);
+ test("(efault)", "%pD", PTR_FIRST_PAGE);
test("romeo", "%pd", &test_dentry[3]);
test("alfa/romeo", "%pd2", &test_dentry[3]);
--
2.16.4
The pointer formatting tests have 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 fit into
the existing structure, namely:
+ hashed pointer testing
+ null and invalid pointer handling with various modifiers
Reshuffle these tests to follow the original structure.
For completeness, add a test for "%px" with some "random" pointer
value. Note that it can't be tested with "%pE" because it would
cause crash.
Signed-off-by: Petr Mladek <[email protected]>
---
lib/test_printf.c | 27 +++++++++++----------------
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 6fa6fb606554..1ee1bb703307 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -278,28 +278,22 @@ test_hashed(const char *fmt, const void *p)
failed_tests++;
}
-static void __init
-plain(void)
-{
- test_hashed("%p", PTR);
-}
+#define PTR_INVALID ((void *)0x000000ab)
static void __init
-null_pointer(void)
+plain_pointer(void)
{
+ test_hashed("%p", PTR);
test_hashed("%p", NULL);
- test(ZEROS "00000000", "%px", NULL);
- test("(null)", "%pE", NULL);
+ 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(PTR_STR, "%px", PTR);
+ test(ZEROS "00000000", "%px", NULL);
test(ZEROS "000000ab", "%px", PTR_INVALID);
- test("(efault)", "%pE", PTR_INVALID);
}
static void __init
@@ -326,6 +320,8 @@ addr(void)
static void __init
escaped_str(void)
{
+ test("(null)", "%pE", NULL);
+ test("(efault)", "%pE", PTR_INVALID);
}
static void __init
@@ -601,9 +597,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
Hello Petr,
On 2/27/20 2:01 PM, Petr Mladek wrote:
> The commit ad67b74d2469d9b82a ("printk: hash addresses printed with %p")
> helps to prevent leaking kernel addresses.
>
> The testing of this functionality is a bit problematic because the output
> 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 bits are zeroed on 64-bit systems
Is "hash differs from the original pointer value" a valid check?
Depending on the random value and the actual pointer I can imagine a
valid match. Such a match is unlikely but not necessarily bogus, is it?
Best regards
Uwe
On Thu 2020-02-27 15:30:51, Uwe Kleine-K?nig wrote:
> Hello Petr,
>
> On 2/27/20 2:01 PM, Petr Mladek wrote:
> > The commit ad67b74d2469d9b82a ("printk: hash addresses printed with %p")
> > helps to prevent leaking kernel addresses.
> >
> > The testing of this functionality is a bit problematic because the output
> > 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 bits are zeroed on 64-bit systems
>
> Is "hash differs from the original pointer value" a valid check?
> Depending on the random value and the actual pointer I can imagine a
> valid match. Such a match is unlikely but not necessarily bogus, is it?
Yes, there is a small risk or false negative.
It might be possible to try if the problem persist with PTR+1 value or
so. But I am not sure if it is worth it.
The problem is only on 32-bit systems. The chance is really small.
I have added a comment above the check. It can be found via the added
error message.
Note that this check has been there even before in plain_hash().
But it was worse because it was without any comment or error message.
Best Regards,
Petr
On (20/02/27 14:01), Petr Mladek wrote:
>
> The discussion about hashing NULL pointer value[1] uncovered that
> the basic tests of pointer formatting do not follow the original
> structure and cause confusion.
FWIW, overall looks good to me.
> I feel responsible for it and here is a proposed clean up.
Extra points!
-ss
On Tue 2020-03-03 16:24:56, Sergey Senozhatsky wrote:
> On (20/02/27 14:01), Petr Mladek wrote:
> >
> > The discussion about hashing NULL pointer value[1] uncovered that
> > the basic tests of pointer formatting do not follow the original
> > structure and cause confusion.
>
> FWIW, overall looks good to me.
Could I add Acked-by or Reviewed-by tag, please?
Best Regards,
Petr
On (20/03/03 10:22), Petr Mladek wrote:
> On Tue 2020-03-03 16:24:56, Sergey Senozhatsky wrote:
> > On (20/02/27 14:01), Petr Mladek wrote:
> > >
> > > The discussion about hashing NULL pointer value[1] uncovered that
> > > the basic tests of pointer formatting do not follow the original
> > > structure and cause confusion.
> >
> > FWIW, overall looks good to me.
>
> Could I add Acked-by or Reviewed-by tag, please?
Sure
Reviewed-by: Sergey Senozhatsky <[email protected]>
-ss
On 27/02/2020 14.01, Petr Mladek wrote:
> The commit ad67b74d2469d9b82a ("printk: hash addresses printed with %p")
> helps to prevent leaking kernel addresses.
>
> The testing of this functionality is a bit problematic because the output
> 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 bits are zeroed on 64-bit systems
>
> This is currently done by a maze of functions:
>
> + It is hard to follow.
> + Some code is duplicated, e.g. the check for initialized crng.
> + The zeroed top half bits are tested only with one hardcoded PTR.
> + plain() increments "failed_tests" but not "total_tests".
> + The generic test_hashed() does not touch number of tests at all.
>
> Move all the checks into test_hashed() so that they are done for
> any given pointer that should get hashed. Also handle test counters
> and internal errors the same way as the existing test() function.
>
> Signed-off-by: Petr Mladek <[email protected]>
> ---
> lib/test_printf.c | 130 ++++++++++++++++++------------------------------------
> 1 file changed, 42 insertions(+), 88 deletions(-)
>
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 2d9f520d2f27..6fa6fb606554 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -215,29 +215,6 @@ test_string(void)
> #define PTR_VAL_NO_CRNG "(____ptrval____)"
> #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
> @@ -246,88 +223,65 @@ plain_format(void)
> #define PTR_VAL_NO_CRNG "(ptrval)"
> #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 real[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(real, sizeof(real), "%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;
> + nchars = snprintf(hash, sizeof(hash), fmt, p);
I don't like introducing a use of snprintf in the test suite where the
compiler cannot do the basic type checking. In fact, I think we should
turn on -Werror=format (or whatever the spelling is) for test_printf.c.
So I'd much rather introduce a
int check_hashed(const char *hashed, int ret, void *p)
helper and have the caller do the "%p", p formatting to a local buffer,
pass that buffer and the snprintf return value along with the formatted
pointer p to check_hashed, then do
failed_tests += check_hashed(...)
in the caller. Then you can use a "return 1" in the places where you now
have a "goto err".
And I think you need a rather early check in check_hashed that there's a
nul byte in the buffer that is being checked (as well as in the buffer
containing the "%px" output) before you use those buffers as %s
arguments in the error messages. do_test() carefully postpones the
comparison to the expected content (and writing of the "expected ...,
got ...") until after we at least know %s won't end up reading beyond
the end of the buffer.
> + if (nchars != PTR_WIDTH) {
> + pr_warn("vsprintf(\"%s\", p) returned number of characters %d, expected %d\n",
> + fmt, nchars, PTR_WIDTH);
No, you did not call vsprintf. You called snprintf() - and vsprintf
isn't even in the call chain of that. Given that there are functions in
vsprintf.c that munge the return value (the s_c_nprintf family), please
be more precise.
> + goto err;
> + }
>
> - err = plain_hash();
> - if (err) {
> - pr_warn("plain 'p' does not appear to be hashed\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;
> }
Rather than decrementing total_tests, we should have a skipped_tests to
account for the rare case(s) where we had to skip a test for some
reason. Doing pr_warn_once for each such case is fine.
Also, typo (vsprinf), but use the right name anyway.
>
> - err = plain_format();
> - if (err) {
> - pr_warn("hashing plain 'p' has unexpected format\n");
> - failed_tests++;
> + /*
> + * There is a small chance of a false negative on 32-bit systems
> + * when the hash is the same as the pointer value.
> + */
> + if (strncmp(hash, real, 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
OK, but should we also have a strspn(, "0123456789abcdef") check that
the formatted string consists of precisely PTR_WIDTH hex decimals?
Rasmus