linux-kernel - Re: [PATCH] vsprintf: Fix %pK with kptr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YgJ1a0Mi0wEbr88C@alley>
Date:   Tue, 8 Feb 2022 14:51:39 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Christophe Leroy <christophe.leroy@...roup.eu>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        Kees Cook <keescook@...omium.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] vsprintf: Fix %pK with kptr_restrict == 0

Adding Kees and Linus in Cc because it modifies %pK behavior.

On Thu 2022-01-27 11:11:02, Christophe Leroy wrote:
> Although kptr_restrict is set to 0 and the kernel is booted with
> no_hash_pointers parameter, the content of /proc/vmallocinfo is
> lacking the real addresses.
> 
>   / # cat /proc/vmallocinfo
>   0x(ptrval)-0x(ptrval)    8192 load_module+0xc0c/0x2c0c pages=1 vmalloc
>   0x(ptrval)-0x(ptrval)   12288 start_kernel+0x4e0/0x690 pages=2 vmalloc
>   0x(ptrval)-0x(ptrval)   12288 start_kernel+0x4e0/0x690 pages=2 vmalloc
>   0x(ptrval)-0x(ptrval)    8192 _mpic_map_mmio.constprop.0+0x20/0x44 phys=0x80041000 ioremap
>   0x(ptrval)-0x(ptrval)   12288 _mpic_map_mmio.constprop.0+0x20/0x44 phys=0x80041000 ioremap
>     ...
> 
> According to the documentation for /proc/sys/kernel/, %pK is
> equivalent to %p when kptr_restrict is set to 0.

Good catch!

BTW: The behavior is strange also when kptr_restrict == 1. It allways
prints non-hashed pointers for user space adresses. It means that
it is less restrictive than kptr_restrict == 0 by default when
no_hash_pointers == 0. It is probably not a big deal but...


> ---
>  lib/vsprintf.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b8129dd374c..9c60d6e1a0d6 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -857,6 +861,8 @@ char *restricted_pointer(char *buf, char *end, const void *ptr,
>  	switch (kptr_restrict) {
>  	case 0:
>  		/* Handle as %p, hash and do _not_ leak addresses. */
> +		if (unlikely(no_hash_pointers))
> +			break;
>  		return ptr_to_id(buf, end, ptr, spec);

This is a twisted duplication of the following code from pointer():

static noinline_for_stack
char *pointer(const char *fmt, char *buf, char *end, void *ptr,
	      struct printf_spec spec)
{
[...]
	/*
	 * default is to _not_ leak addresses, so hash before printing,
	 * unless no_hash_pointers is specified on the command line.
	 */
	if (unlikely(no_hash_pointers))
		return pointer_string(buf, end, ptr, spec);
	else
		return ptr_to_id(buf, end, ptr, spec);
}

Instead, I would create:

/*
 * default is to _not_ leak addresses, so hash before printing,
 * unless no_hash_pointers is specified on the command line.
 */
static noinline_for_stack
char *default_pointer(const char *fmt, char *buf, char *end, void *ptr,
		      struct printf_spec spec) 
{
	if (unlikely(no_hash_pointers))
		return pointer_string(buf, end, ptr, spec);

	return ptr_to_id(buf, end, ptr, spec);
}

and use it in both hash_pointer() and pointer().

And I would use is also for kptr_restrict == 1. But it probably
should be done in a separate patch and should be acked by Kees.


>  	case 1: {
>  		const struct cred *cred;

Best Regards,
Petr