Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

From: Helge Deller <deller@gmx.de>
To: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Cc: linux-kernel@vger.kernel.org,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Petr Mladek <pmladek@suse.com>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
Date: Thu, 7 Sep 2017 11:12:21 +0200	[thread overview]
Message-ID: <667b8849-fb60-a312-2483-505252ff737e@gmx.de> (raw)
In-Reply-To: <20170907083207.GC533@jagdpanzerIV.localdomain>

On 07.09.2017 10:32, Sergey Senozhatsky wrote:
> On (09/07/17 16:56), Sergey Senozhatsky wrote:
> [..]

> probe_kernel_address() handles the page fault and returns -EFAULT if
> you give it bad pointer. module_address_lookup() and get_symbol_pos()
> seems to be smart enough not to crash on bad pointer as well. what am
> I missing? could you please explain where we will crash?

Actually I never faced a kernel crash because of this on parisc.
Don't know for ia64 and ppc64.

>> BTW, are we sure we can crash? when attempt to deference IP from
>> the given descriptor? shall we handle page fault in this case and
>> do something sane? just asking.
> 
> I don't know... does the below code make any sense?

No. Read below.

> basically it checks that it's safe to access ptr (we can access it
> without page fault in __dereference_function_descriptor()). then
> we do ptr->ip, and also check if it's safe, but in
> dereference_function_descriptor().
> 
> I suppose somethign like
> 
> 	pr_err("%pF\n", 1);
> 
> can crash ia64, etc. correct?

On parisc it will not crash because we handle that one already.
For others I don't know.

But something like
	pr_err("%pF\n", (unsigned long) (-1));
or any address above 0xffffffff00000000ULL might do bad things
on parisc, because it touches the I/O space and we don't check that yet.

>  lib/vsprintf.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 86c3385b9eb3..0dc39b95e1d9 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1593,6 +1593,16 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
>  
>  int kptr_restrict __read_mostly;
>  
> +static void *__dereference_function_descriptor(void *ptr)
> +{
> +	void *p;
> +
> +	if (!probe_kernel_address(ptr, p))
> +		return dereference_function_descriptor(ptr);
> +
> +	return ptr;
> +}
> +
>  /*
>   * Show a '%p' thing.  A kernel extension is that the '%p' is followed
>   * by an extra set of alphanumeric characters that are extended format
> @@ -1723,7 +1733,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  	switch (*fmt) {
>  	case 'F':
>  	case 'f':
> -		ptr = dereference_function_descriptor(ptr);
> +		ptr = __dereference_function_descriptor(ptr);

This is not needed.
All affected arches (ia64, ppc64, parisc64) already call  
probe_kernel_address() inside their dereference_function_descriptor() function.
So this patch just adds unnecessary overhead for all arches.

> ... here is a question, does function descriptor belong to a special
> section? can we check that supplied ptr belongs to a descriptor section
> and avoid dereference_function_descriptor() if it doesn't? (just fall
> through directly to symbol_string() in this case). is this possible?

I think theoretically yes.
On parisc ptr does *not* point to any code segment, and most likely it
points to the .data segment. I don't know if that's the case for ia64 and
ppc64 too.
I can look into adding such check-code, but even then the warning will
only show up if you run on ia64, ppc64 and parisc64.

Helge