Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 1/3] lib/test_printf: Clean up test of hashed pointers
To:     Petr Mladek <pmladek@suse.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
        Steven Rostedt <rostedt@goodmis.org>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
        linux-kernel@vger.kernel.org,
        =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= <uwe@kleine-koenig.org>,
        Ilya Dryomov <idryomov@gmail.com>,
        Kees Cook <keescook@chromium.org>,
        "Tobin C . Harding" <me@tobin.cc>
References: <20200227130123.32442-1-pmladek@suse.com>
 <20200227130123.32442-2-pmladek@suse.com>
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
Message-ID: <2572b127-fd57-3de6-2a49-23886129d781@rasmusvillemoes.dk>
Date:   Tue, 3 Mar 2020 12:14:51 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
In-Reply-To: <20200227130123.32442-2-pmladek@suse.com>
Content-Type: text/plain; charset=windows-1252
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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