lkml.org 
[lkml]   [2020]   [Mar]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/3] lib/test_printf: Clean up test of hashed pointers
From
Date
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 <pmladek@suse.com>
> ---
> 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

\
 
 \ /
  Last update: 2020-03-03 12:15    [W:0.088 / U:1.956 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site