[PATCH v5 00/11] vsprintf: Prevent silent crashes and consolidate error handling

[PATCH v5 00/11] vsprintf: Prevent silent crashes and consolidate error handling

[Petr Mladek]

Crash in vsprintf() might be silent when it happens under logbuf_lock
in vprintk_emit(). This patch set prevents most of the crashes by probing
the address. The check is done only by %s and some %p* specifiers that need
to dereference the address.

Only the first byte of the address is checked to keep it simple. It should
be enough to catch most problems.

The check is explicitly done in each function that does the dereference.
It helps to avoid the questionable strchr() of affected specifiers. This
change motivated me to do some preparation patches that consolidated
the error handling and cleaned the code a bit.

I did my best to address the feedback. Note that there is still the
(efault) error message. But it is accompanied with WARN() when
panic_on_warn is not enabled. I hope that it makes it more acceptable.


Changes against v4:

	+ rebased on top of
git://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git for-4.18
	+ Added missing conts into ptr_to_ind() in a separate patch
	+ Renamed __string to valid_string()
	+ Avoid WARN() for invalid poimter specifiers
	+ Removed noinline_for_stack where it was not really useful
	+ WARN() when accessing invalid non-NULL address

Changes against v3:

	+ Add valid_pointer_access() to do the check and store the error
	  message in one call.
	+ Remove strchr(). Instead, validate the address in functions
	  that dereference the address.
	+ Use probe_kernel_address() instead of probe_kernel_real().
	+ Do the check only for unknown address.
	+ Consolidate handling of unsupported pointer modifiers.

Changes against v2:

	+ Fix handling with strchr(string, '\0'). Happens with
	  %p at the very end of the string.
	+ Even more clear commit message
	+ Documentation/core-api/printk-formats.rst update.
	+ Add check into lib/test_printf.c.

Changes against v1:

	+ Do not check access for plain %p.
	+ More clear commit message.


Petr Mladek (11):
  vsprintf: Shuffle misc pointer to string functions
  vsprintf: Add missing const ptr qualifier to prt_to_id()
  vsprintf: Consistent %pK handling for kptr_restrict == 0
  vsprintf: Do not check address of well-known strings
  vsprintf: Consolidate handling of unknown pointer specifiers
  vsprintf: Factor out %p[iI] handler as ip_addr_string()
  vsprintf: Factor out %pV handler as va_format()
  vsprintf: Factor out %pO handler as kobject_string()
  vsprintf: Prevent crash when dereferencing invalid pointers
  vsprintf: WARN() on invalid pointer access
  vsprintf: Avoid confusion between invalid address and value

 Documentation/core-api/printk-formats.rst |  11 +
 lib/test_printf.c                         |  44 ++-
 lib/vsprintf.c                            | 549 ++++++++++++++++++------------
 3 files changed, 372 insertions(+), 232 deletions(-)

-- 
2.13.6

[PATCH v5 01/11] vsprintf: Shuffle misc pointer to string functions

[Petr Mladek]

This is just a preparation step for further changes.

The patch does not change the code.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 lib/vsprintf.c | 244 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 122 insertions(+), 122 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b82f0c6c2aec..19fdfe621b40 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -613,6 +613,128 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 }
 
 static noinline_for_stack
+char *pointer_string(char *buf, char *end, const void *ptr,
+		     struct printf_spec spec)
+{
+	spec.base = 16;
+	spec.flags |= SMALL;
+	if (spec.field_width == -1) {
+		spec.field_width = 2 * sizeof(ptr);
+		spec.flags |= ZEROPAD;
+	}
+
+	return number(buf, end, (unsigned long int)ptr, spec);
+}
+
+static bool have_filled_random_ptr_key __read_mostly;
+static siphash_key_t ptr_key __read_mostly;
+
+static void fill_random_ptr_key(struct random_ready_callback *unused)
+{
+	get_random_bytes(&ptr_key, sizeof(ptr_key));
+	/*
+	 * have_filled_random_ptr_key==true is dependent on get_random_bytes().
+	 * ptr_to_id() needs to see have_filled_random_ptr_key==true
+	 * after get_random_bytes() returns.
+	 */
+	smp_mb();
+	WRITE_ONCE(have_filled_random_ptr_key, true);
+}
+
+static struct random_ready_callback random_ready = {
+	.func = fill_random_ptr_key
+};
+
+static int __init initialize_ptr_random(void)
+{
+	int ret = add_random_ready_callback(&random_ready);
+
+	if (!ret) {
+		return 0;
+	} else if (ret == -EALREADY) {
+		fill_random_ptr_key(&random_ready);
+		return 0;
+	}
+
+	return ret;
+}
+early_initcall(initialize_ptr_random);
+
+/* Maps a pointer to a 32 bit unique identifier. */
+static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
+{
+	const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
+	unsigned long hashval;
+
+	if (unlikely(!have_filled_random_ptr_key)) {
+		spec.field_width = 2 * sizeof(ptr);
+		/* string length must be less than default_width */
+		return string(buf, end, str, spec);
+	}
+
+#ifdef CONFIG_64BIT
+	hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
+	/*
+	 * Mask off the first 32 bits, this makes explicit that we have
+	 * modified the address (and 32 bits is plenty for a unique ID).
+	 */
+	hashval = hashval & 0xffffffff;
+#else
+	hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
+#endif
+	return pointer_string(buf, end, (const void *)hashval, spec);
+}
+
+int kptr_restrict __read_mostly;
+
+static noinline_for_stack
+char *restricted_pointer(char *buf, char *end, const void *ptr,
+			 struct printf_spec spec)
+{
+	switch (kptr_restrict) {
+	case 0:
+		/* Always print %pK values */
+		break;
+	case 1: {
+		const struct cred *cred;
+
+		/*
+		 * kptr_restrict==1 cannot be used in IRQ context
+		 * because its test for CAP_SYSLOG would be meaningless.
+		 */
+		if (in_irq() || in_serving_softirq() || in_nmi()) {
+			if (spec.field_width == -1)
+				spec.field_width = 2 * sizeof(ptr);
+			return string(buf, end, "pK-error", spec);
+		}
+
+		/*
+		 * Only print the real pointer value if the current
+		 * process has CAP_SYSLOG and is running with the
+		 * same credentials it started with. This is because
+		 * access to files is checked at open() time, but %pK
+		 * checks permission at read() time. We don't want to
+		 * leak pointer values if a binary opens a file using
+		 * %pK and then elevates privileges before reading it.
+		 */
+		cred = current_cred();
+		if (!has_capability_noaudit(current, CAP_SYSLOG) ||
+		    !uid_eq(cred->euid, cred->uid) ||
+		    !gid_eq(cred->egid, cred->gid))
+			ptr = NULL;
+		break;
+	}
+	case 2:
+	default:
+		/* Always print 0's for %pK */
+		ptr = NULL;
+		break;
+	}
+
+	return pointer_string(buf, end, ptr, spec);
+}
+
+static noinline_for_stack
 char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_spec spec,
 		  const char *fmt)
 {
@@ -1358,69 +1480,6 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 }
 
 static noinline_for_stack
-char *pointer_string(char *buf, char *end, const void *ptr,
-		     struct printf_spec spec)
-{
-	spec.base = 16;
-	spec.flags |= SMALL;
-	if (spec.field_width == -1) {
-		spec.field_width = 2 * sizeof(ptr);
-		spec.flags |= ZEROPAD;
-	}
-
-	return number(buf, end, (unsigned long int)ptr, spec);
-}
-
-int kptr_restrict __read_mostly;
-
-static noinline_for_stack
-char *restricted_pointer(char *buf, char *end, const void *ptr,
-			 struct printf_spec spec)
-{
-	switch (kptr_restrict) {
-	case 0:
-		/* Always print %pK values */
-		break;
-	case 1: {
-		const struct cred *cred;
-
-		/*
-		 * kptr_restrict==1 cannot be used in IRQ context
-		 * because its test for CAP_SYSLOG would be meaningless.
-		 */
-		if (in_irq() || in_serving_softirq() || in_nmi()) {
-			if (spec.field_width == -1)
-				spec.field_width = 2 * sizeof(ptr);
-			return string(buf, end, "pK-error", spec);
-		}
-
-		/*
-		 * Only print the real pointer value if the current
-		 * process has CAP_SYSLOG and is running with the
-		 * same credentials it started with. This is because
-		 * access to files is checked at open() time, but %pK
-		 * checks permission at read() time. We don't want to
-		 * leak pointer values if a binary opens a file using
-		 * %pK and then elevates privileges before reading it.
-		 */
-		cred = current_cred();
-		if (!has_capability_noaudit(current, CAP_SYSLOG) ||
-		    !uid_eq(cred->euid, cred->uid) ||
-		    !gid_eq(cred->egid, cred->gid))
-			ptr = NULL;
-		break;
-	}
-	case 2:
-	default:
-		/* Always print 0's for %pK */
-		ptr = NULL;
-		break;
-	}
-
-	return pointer_string(buf, end, ptr, spec);
-}
-
-static noinline_for_stack
 char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt)
 {
 	unsigned long long num;
@@ -1654,65 +1713,6 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
-static bool have_filled_random_ptr_key __read_mostly;
-static siphash_key_t ptr_key __read_mostly;
-
-static void fill_random_ptr_key(struct random_ready_callback *unused)
-{
-	get_random_bytes(&ptr_key, sizeof(ptr_key));
-	/*
-	 * have_filled_random_ptr_key==true is dependent on get_random_bytes().
-	 * ptr_to_id() needs to see have_filled_random_ptr_key==true
-	 * after get_random_bytes() returns.
-	 */
-	smp_mb();
-	WRITE_ONCE(have_filled_random_ptr_key, true);
-}
-
-static struct random_ready_callback random_ready = {
-	.func = fill_random_ptr_key
-};
-
-static int __init initialize_ptr_random(void)
-{
-	int ret = add_random_ready_callback(&random_ready);
-
-	if (!ret) {
-		return 0;
-	} else if (ret == -EALREADY) {
-		fill_random_ptr_key(&random_ready);
-		return 0;
-	}
-
-	return ret;
-}
-early_initcall(initialize_ptr_random);
-
-/* Maps a pointer to a 32 bit unique identifier. */
-static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
-{
-	const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
-	unsigned long hashval;
-
-	if (unlikely(!have_filled_random_ptr_key)) {
-		spec.field_width = 2 * sizeof(ptr);
-		/* string length must be less than default_width */
-		return string(buf, end, str, spec);
-	}
-
-#ifdef CONFIG_64BIT
-	hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
-	/*
-	 * Mask off the first 32 bits, this makes explicit that we have
-	 * modified the address (and 32 bits is plenty for a unique ID).
-	 */
-	hashval = hashval & 0xffffffff;
-#else
-	hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
-#endif
-	return pointer_string(buf, end, (const void *)hashval, spec);
-}
-
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
-- 
2.13.6

Re: [PATCH v5 01/11] vsprintf: Shuffle misc pointer to string functions

[Andy Shevchenko]

On Wed, 2018-04-25 at 13:12 +0200, Petr Mladek wrote:
> This is just a preparation step for further changes.
> 
> The patch does not change the code.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  lib/vsprintf.c | 244 ++++++++++++++++++++++++++++------------------
> -----------
>  1 file changed, 122 insertions(+), 122 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index b82f0c6c2aec..19fdfe621b40 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -613,6 +613,128 @@ char *string(char *buf, char *end, const char
> *s, struct printf_spec spec)
>  }
>  
>  static noinline_for_stack
> +char *pointer_string(char *buf, char *end, const void *ptr,
> +		     struct printf_spec spec)
> +{
> +	spec.base = 16;
> +	spec.flags |= SMALL;
> +	if (spec.field_width == -1) {
> +		spec.field_width = 2 * sizeof(ptr);
> +		spec.flags |= ZEROPAD;
> +	}
> +
> +	return number(buf, end, (unsigned long int)ptr, spec);
> +}
> +
> +static bool have_filled_random_ptr_key __read_mostly;
> +static siphash_key_t ptr_key __read_mostly;
> +
> +static void fill_random_ptr_key(struct random_ready_callback *unused)
> +{
> +	get_random_bytes(&ptr_key, sizeof(ptr_key));
> +	/*
> +	 * have_filled_random_ptr_key==true is dependent on
> get_random_bytes().
> +	 * ptr_to_id() needs to see have_filled_random_ptr_key==true
> +	 * after get_random_bytes() returns.
> +	 */
> +	smp_mb();
> +	WRITE_ONCE(have_filled_random_ptr_key, true);
> +}
> +
> +static struct random_ready_callback random_ready = {
> +	.func = fill_random_ptr_key
> +};
> +
> +static int __init initialize_ptr_random(void)
> +{
> +	int ret = add_random_ready_callback(&random_ready);
> +
> +	if (!ret) {
> +		return 0;
> +	} else if (ret == -EALREADY) {
> +		fill_random_ptr_key(&random_ready);
> +		return 0;
> +	}
> +
> +	return ret;
> +}
> +early_initcall(initialize_ptr_random);
> +
> +/* Maps a pointer to a 32 bit unique identifier. */
> +static char *ptr_to_id(char *buf, char *end, void *ptr, struct
> printf_spec spec)
> +{
> +	const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" :
> "(ptrval)";
> +	unsigned long hashval;
> +
> +	if (unlikely(!have_filled_random_ptr_key)) {
> +		spec.field_width = 2 * sizeof(ptr);
> +		/* string length must be less than default_width */
> +		return string(buf, end, str, spec);
> +	}
> +
> +#ifdef CONFIG_64BIT
> +	hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
> +	/*
> +	 * Mask off the first 32 bits, this makes explicit that we
> have
> +	 * modified the address (and 32 bits is plenty for a unique
> ID).
> +	 */
> +	hashval = hashval & 0xffffffff;
> +#else
> +	hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
> +#endif
> +	return pointer_string(buf, end, (const void *)hashval, spec);
> +}
> +
> +int kptr_restrict __read_mostly;
> +
> +static noinline_for_stack
> +char *restricted_pointer(char *buf, char *end, const void *ptr,
> +			 struct printf_spec spec)
> +{
> +	switch (kptr_restrict) {
> +	case 0:
> +		/* Always print %pK values */
> +		break;
> +	case 1: {
> +		const struct cred *cred;
> +
> +		/*
> +		 * kptr_restrict==1 cannot be used in IRQ context
> +		 * because its test for CAP_SYSLOG would be
> meaningless.
> +		 */
> +		if (in_irq() || in_serving_softirq() || in_nmi()) {
> +			if (spec.field_width == -1)
> +				spec.field_width = 2 * sizeof(ptr);
> +			return string(buf, end, "pK-error", spec);
> +		}
> +
> +		/*
> +		 * Only print the real pointer value if the current
> +		 * process has CAP_SYSLOG and is running with the
> +		 * same credentials it started with. This is because
> +		 * access to files is checked at open() time, but %pK
> +		 * checks permission at read() time. We don't want to
> +		 * leak pointer values if a binary opens a file using
> +		 * %pK and then elevates privileges before reading
> it.
> +		 */
> +		cred = current_cred();
> +		if (!has_capability_noaudit(current, CAP_SYSLOG) ||
> +		    !uid_eq(cred->euid, cred->uid) ||
> +		    !gid_eq(cred->egid, cred->gid))
> +			ptr = NULL;
> +		break;
> +	}
> +	case 2:
> +	default:
> +		/* Always print 0's for %pK */
> +		ptr = NULL;
> +		break;
> +	}
> +
> +	return pointer_string(buf, end, ptr, spec);
> +}
> +
> +static noinline_for_stack
>  char *dentry_name(char *buf, char *end, const struct dentry *d,
> struct printf_spec spec,
>  		  const char *fmt)
>  {
> @@ -1358,69 +1480,6 @@ char *uuid_string(char *buf, char *end, const
> u8 *addr,
>  }
>  
>  static noinline_for_stack
> -char *pointer_string(char *buf, char *end, const void *ptr,
> -		     struct printf_spec spec)
> -{
> -	spec.base = 16;
> -	spec.flags |= SMALL;
> -	if (spec.field_width == -1) {
> -		spec.field_width = 2 * sizeof(ptr);
> -		spec.flags |= ZEROPAD;
> -	}
> -
> -	return number(buf, end, (unsigned long int)ptr, spec);
> -}
> -
> -int kptr_restrict __read_mostly;
> -
> -static noinline_for_stack
> -char *restricted_pointer(char *buf, char *end, const void *ptr,
> -			 struct printf_spec spec)
> -{
> -	switch (kptr_restrict) {
> -	case 0:
> -		/* Always print %pK values */
> -		break;
> -	case 1: {
> -		const struct cred *cred;
> -
> -		/*
> -		 * kptr_restrict==1 cannot be used in IRQ context
> -		 * because its test for CAP_SYSLOG would be
> meaningless.
> -		 */
> -		if (in_irq() || in_serving_softirq() || in_nmi()) {
> -			if (spec.field_width == -1)
> -				spec.field_width = 2 * sizeof(ptr);
> -			return string(buf, end, "pK-error", spec);
> -		}
> -
> -		/*
> -		 * Only print the real pointer value if the current
> -		 * process has CAP_SYSLOG and is running with the
> -		 * same credentials it started with. This is because
> -		 * access to files is checked at open() time, but %pK
> -		 * checks permission at read() time. We don't want to
> -		 * leak pointer values if a binary opens a file using
> -		 * %pK and then elevates privileges before reading
> it.
> -		 */
> -		cred = current_cred();
> -		if (!has_capability_noaudit(current, CAP_SYSLOG) ||
> -		    !uid_eq(cred->euid, cred->uid) ||
> -		    !gid_eq(cred->egid, cred->gid))
> -			ptr = NULL;
> -		break;
> -	}
> -	case 2:
> -	default:
> -		/* Always print 0's for %pK */
> -		ptr = NULL;
> -		break;
> -	}
> -
> -	return pointer_string(buf, end, ptr, spec);
> -}
> -
> -static noinline_for_stack
>  char *netdev_bits(char *buf, char *end, const void *addr, const char
> *fmt)
>  {
>  	unsigned long long num;
> @@ -1654,65 +1713,6 @@ char *device_node_string(char *buf, char *end,
> struct device_node *dn,
>  	return widen_string(buf, buf - buf_start, end, spec);
>  }
>  
> -static bool have_filled_random_ptr_key __read_mostly;
> -static siphash_key_t ptr_key __read_mostly;
> -
> -static void fill_random_ptr_key(struct random_ready_callback *unused)
> -{
> -	get_random_bytes(&ptr_key, sizeof(ptr_key));
> -	/*
> -	 * have_filled_random_ptr_key==true is dependent on
> get_random_bytes().
> -	 * ptr_to_id() needs to see have_filled_random_ptr_key==true
> -	 * after get_random_bytes() returns.
> -	 */
> -	smp_mb();
> -	WRITE_ONCE(have_filled_random_ptr_key, true);
> -}
> -
> -static struct random_ready_callback random_ready = {
> -	.func = fill_random_ptr_key
> -};
> -
> -static int __init initialize_ptr_random(void)
> -{
> -	int ret = add_random_ready_callback(&random_ready);
> -
> -	if (!ret) {
> -		return 0;
> -	} else if (ret == -EALREADY) {
> -		fill_random_ptr_key(&random_ready);
> -		return 0;
> -	}
> -
> -	return ret;
> -}
> -early_initcall(initialize_ptr_random);
> -
> -/* Maps a pointer to a 32 bit unique identifier. */
> -static char *ptr_to_id(char *buf, char *end, void *ptr, struct
> printf_spec spec)
> -{
> -	const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" :
> "(ptrval)";
> -	unsigned long hashval;
> -
> -	if (unlikely(!have_filled_random_ptr_key)) {
> -		spec.field_width = 2 * sizeof(ptr);
> -		/* string length must be less than default_width */
> -		return string(buf, end, str, spec);
> -	}
> -
> -#ifdef CONFIG_64BIT
> -	hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
> -	/*
> -	 * Mask off the first 32 bits, this makes explicit that we
> have
> -	 * modified the address (and 32 bits is plenty for a unique
> ID).
> -	 */
> -	hashval = hashval & 0xffffffff;
> -#else
> -	hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
> -#endif
> -	return pointer_string(buf, end, (const void *)hashval, spec);
> -}
> -
>  /*
>   * Show a '%p' thing.  A kernel extension is that the '%p' is
> followed
>   * by an extra set of alphanumeric characters that are extended
> format

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v5 02/11] vsprintf: Add missing const ptr qualifier to prt_to_id()

[Petr Mladek]

vsprintf() must not change any data that parameters point to.
Let's add the missing const qualifier to ptr_to_id().

This patch does not change the existing behavior.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 lib/vsprintf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 19fdfe621b40..eef9f725e9ff 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -661,7 +661,8 @@ static int __init initialize_ptr_random(void)
 early_initcall(initialize_ptr_random);
 
 /* Maps a pointer to a 32 bit unique identifier. */
-static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
+static char *ptr_to_id(char *buf, char *end, const void *ptr,
+		       struct printf_spec spec)
 {
 	const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
 	unsigned long hashval;
-- 
2.13.6

Re: [PATCH v5 02/11] vsprintf: Add missing const ptr qualifier to prt_to_id()

[Andy Shevchenko]

On Wed, 2018-04-25 at 13:12 +0200, Petr Mladek wrote:
> vsprintf() must not change any data that parameters point to.
> Let's add the missing const qualifier to ptr_to_id().
> 
> This patch does not change the existing behavior.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  lib/vsprintf.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 19fdfe621b40..eef9f725e9ff 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -661,7 +661,8 @@ static int __init initialize_ptr_random(void)
>  early_initcall(initialize_ptr_random);
>  
>  /* Maps a pointer to a 32 bit unique identifier. */
> -static char *ptr_to_id(char *buf, char *end, void *ptr, struct
> printf_spec spec)
> +static char *ptr_to_id(char *buf, char *end, const void *ptr,
> +		       struct printf_spec spec)
>  {
>  	const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" :
> "(ptrval)";
>  	unsigned long hashval;

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v5 03/11] vsprintf: Consistent %pK handling for kptr_restrict == 0

[Petr Mladek]

restricted_pointer() pretends that it prints the address when kptr_restrict
is set to zero. But it is never called in this situation. Instead,
pointer() falls back to ptr_to_id() and hashes the pointer.

This patch removes the potential confusion. klp_restrict is checked only
in restricted_pointer().

It actually fixes a small race when the address might get printed unhashed:

CPU0                            CPU1

pointer()
  if (!kptr_restrict)
     /* for example set to 2 */
  restricted_pointer()
				/* echo 0 >/proc/sys/kernel/kptr_restrict */
				proc_dointvec_minmax_sysadmin()
				  klpr_restrict = 0;
    switch(kptr_restrict)
      case 0:
	break:

    number()

Fixes: commit ef0010a30935de4e0211 ("vsprintf: don't use 'restricted_pointer()' when not restricting")
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Tobin Harding <me@tobin.cc>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 lib/vsprintf.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index eef9f725e9ff..2678dfe61d73 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -694,8 +694,8 @@ char *restricted_pointer(char *buf, char *end, const void *ptr,
 {
 	switch (kptr_restrict) {
 	case 0:
-		/* Always print %pK values */
-		break;
+		/* Handle as %p, hash and do _not_ leak addresses. */
+		return ptr_to_id(buf, end, ptr, spec);
 	case 1: {
 		const struct cred *cred;
 
@@ -1915,8 +1915,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			return buf;
 		}
 	case 'K':
-		if (!kptr_restrict)
-			break;
 		return restricted_pointer(buf, end, ptr, spec);
 	case 'N':
 		return netdev_bits(buf, end, ptr, fmt);
-- 
2.13.6

Re: [PATCH v5 03/11] vsprintf: Consistent %pK handling for kptr_restrict == 0

[Andy Shevchenko]

On Wed, 2018-04-25 at 13:12 +0200, Petr Mladek wrote:
> restricted_pointer() pretends that it prints the address when
> kptr_restrict
> is set to zero. But it is never called in this situation. Instead,
> pointer() falls back to ptr_to_id() and hashes the pointer.
> 
> This patch removes the potential confusion. klp_restrict is checked
> only
> in restricted_pointer().
> 
> It actually fixes a small race when the address might get printed
> unhashed:
> 
> CPU0                            CPU1
> 
> pointer()
>   if (!kptr_restrict)
>      /* for example set to 2 */
>   restricted_pointer()
> 				/* echo 0
> >/proc/sys/kernel/kptr_restrict */
> 				proc_dointvec_minmax_sysadmin()
> 				  klpr_restrict = 0;
>     switch(kptr_restrict)
>       case 0:
> 	break:
> 
>     number()
> 

FWIW, 
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Fixes: commit ef0010a30935de4e0211 ("vsprintf: don't use
> 'restricted_pointer()' when not restricting")
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Tobin Harding <me@tobin.cc>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  lib/vsprintf.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index eef9f725e9ff..2678dfe61d73 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -694,8 +694,8 @@ char *restricted_pointer(char *buf, char *end,
> const void *ptr,
>  {
>  	switch (kptr_restrict) {
>  	case 0:
> -		/* Always print %pK values */
> -		break;
> +		/* Handle as %p, hash and do _not_ leak addresses. */
> +		return ptr_to_id(buf, end, ptr, spec);
>  	case 1: {
>  		const struct cred *cred;
>  
> @@ -1915,8 +1915,6 @@ char *pointer(const char *fmt, char *buf, char
> *end, void *ptr,
>  			return buf;
>  		}
>  	case 'K':
> -		if (!kptr_restrict)
> -			break;
>  		return restricted_pointer(buf, end, ptr, spec);
>  	case 'N':
>  		return netdev_bits(buf, end, ptr, fmt);

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v5 04/11] vsprintf: Do not check address of well-known strings

[Petr Mladek]

We are going to check the address using probe_kernel_address(). It will
be more expensive and it does not make sense for well known address.

This patch splits the string() function. The variant without the check
is then used on locations that handle string constants or strings defined
as local variables.

This patch does not change the existing behavior.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 lib/vsprintf.c | 83 ++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 46 insertions(+), 37 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 2678dfe61d73..5c26a4e71cdf 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -591,15 +591,13 @@ char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
 	return buf;
 }
 
-static noinline_for_stack
-char *string(char *buf, char *end, const char *s, struct printf_spec spec)
+/* Handle string from a well known address. */
+static char *valid_string(char *buf, char *end, const char *s,
+			  struct printf_spec spec)
 {
 	int len = 0;
 	size_t lim = spec.precision;
 
-	if ((unsigned long)s < PAGE_SIZE)
-		s = "(null)";
-
 	while (lim--) {
 		char c = *s++;
 		if (!c)
@@ -613,6 +611,15 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 }
 
 static noinline_for_stack
+char *string(char *buf, char *end, const char *s,
+		   struct printf_spec spec)
+{
+	if ((unsigned long)s < PAGE_SIZE)
+		s = "(null)";
+
+	return valid_string(buf, end, s, spec);
+}
+
 char *pointer_string(char *buf, char *end, const void *ptr,
 		     struct printf_spec spec)
 {
@@ -670,7 +677,7 @@ static char *ptr_to_id(char *buf, char *end, const void *ptr,
 	if (unlikely(!have_filled_random_ptr_key)) {
 		spec.field_width = 2 * sizeof(ptr);
 		/* string length must be less than default_width */
-		return string(buf, end, str, spec);
+		return valid_string(buf, end, str, spec);
 	}
 
 #ifdef CONFIG_64BIT
@@ -706,7 +713,7 @@ char *restricted_pointer(char *buf, char *end, const void *ptr,
 		if (in_irq() || in_serving_softirq() || in_nmi()) {
 			if (spec.field_width == -1)
 				spec.field_width = 2 * sizeof(ptr);
-			return string(buf, end, "pK-error", spec);
+			return valid_string(buf, end, "pK-error", spec);
 		}
 
 		/*
@@ -820,7 +827,7 @@ char *symbol_string(char *buf, char *end, void *ptr,
 	else
 		sprint_symbol_no_offset(sym, value);
 
-	return string(buf, end, sym, spec);
+	return valid_string(buf, end, sym, spec);
 #else
 	return special_hex_number(buf, end, value, sizeof(void *));
 #endif
@@ -892,27 +899,27 @@ char *resource_string(char *buf, char *end, struct resource *res,
 
 	*p++ = '[';
 	if (res->flags & IORESOURCE_IO) {
-		p = string(p, pend, "io  ", str_spec);
+		p = valid_string(p, pend, "io  ", str_spec);
 		specp = &io_spec;
 	} else if (res->flags & IORESOURCE_MEM) {
-		p = string(p, pend, "mem ", str_spec);
+		p = valid_string(p, pend, "mem ", str_spec);
 		specp = &mem_spec;
 	} else if (res->flags & IORESOURCE_IRQ) {
-		p = string(p, pend, "irq ", str_spec);
+		p = valid_string(p, pend, "irq ", str_spec);
 		specp = &default_dec_spec;
 	} else if (res->flags & IORESOURCE_DMA) {
-		p = string(p, pend, "dma ", str_spec);
+		p = valid_string(p, pend, "dma ", str_spec);
 		specp = &default_dec_spec;
 	} else if (res->flags & IORESOURCE_BUS) {
-		p = string(p, pend, "bus ", str_spec);
+		p = valid_string(p, pend, "bus ", str_spec);
 		specp = &bus_spec;
 	} else {
-		p = string(p, pend, "??? ", str_spec);
+		p = valid_string(p, pend, "??? ", str_spec);
 		specp = &mem_spec;
 		decode = 0;
 	}
 	if (decode && res->flags & IORESOURCE_UNSET) {
-		p = string(p, pend, "size ", str_spec);
+		p = valid_string(p, pend, "size ", str_spec);
 		p = number(p, pend, resource_size(res), *specp);
 	} else {
 		p = number(p, pend, res->start, *specp);
@@ -923,21 +930,21 @@ char *resource_string(char *buf, char *end, struct resource *res,
 	}
 	if (decode) {
 		if (res->flags & IORESOURCE_MEM_64)
-			p = string(p, pend, " 64bit", str_spec);
+			p = valid_string(p, pend, " 64bit", str_spec);
 		if (res->flags & IORESOURCE_PREFETCH)
-			p = string(p, pend, " pref", str_spec);
+			p = valid_string(p, pend, " pref", str_spec);
 		if (res->flags & IORESOURCE_WINDOW)
-			p = string(p, pend, " window", str_spec);
+			p = valid_string(p, pend, " window", str_spec);
 		if (res->flags & IORESOURCE_DISABLED)
-			p = string(p, pend, " disabled", str_spec);
+			p = valid_string(p, pend, " disabled", str_spec);
 	} else {
-		p = string(p, pend, " flags ", str_spec);
+		p = valid_string(p, pend, " flags ", str_spec);
 		p = number(p, pend, res->flags, default_flag_spec);
 	}
 	*p++ = ']';
 	*p = '\0';
 
-	return string(buf, end, sym, spec);
+	return valid_string(buf, end, sym, spec);
 }
 
 static noinline_for_stack
@@ -1105,7 +1112,7 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
 	}
 	*p = '\0';
 
-	return string(buf, end, mac_addr, spec);
+	return valid_string(buf, end, mac_addr, spec);
 }
 
 static noinline_for_stack
@@ -1268,7 +1275,7 @@ char *ip6_addr_string(char *buf, char *end, const u8 *addr,
 	else
 		ip6_string(ip6_addr, addr, fmt);
 
-	return string(buf, end, ip6_addr, spec);
+	return valid_string(buf, end, ip6_addr, spec);
 }
 
 static noinline_for_stack
@@ -1279,7 +1286,7 @@ char *ip4_addr_string(char *buf, char *end, const u8 *addr,
 
 	ip4_string(ip4_addr, addr, fmt);
 
-	return string(buf, end, ip4_addr, spec);
+	return valid_string(buf, end, ip4_addr, spec);
 }
 
 static noinline_for_stack
@@ -1341,7 +1348,7 @@ char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
 	}
 	*p = '\0';
 
-	return string(buf, end, ip6_addr, spec);
+	return valid_string(buf, end, ip6_addr, spec);
 }
 
 static noinline_for_stack
@@ -1376,7 +1383,7 @@ char *ip4_addr_string_sa(char *buf, char *end, const struct sockaddr_in *sa,
 	}
 	*p = '\0';
 
-	return string(buf, end, ip4_addr, spec);
+	return valid_string(buf, end, ip4_addr, spec);
 }
 
 static noinline_for_stack
@@ -1477,7 +1484,7 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 
 	*p = 0;
 
-	return string(buf, end, uuid, spec);
+	return valid_string(buf, end, uuid, spec);
 }
 
 static noinline_for_stack
@@ -1614,13 +1621,13 @@ char *device_node_gen_full_name(const struct device_node *np, char *buf, char *e
 
 	/* special case for root node */
 	if (!parent)
-		return string(buf, end, "/", default_str_spec);
+		return valid_string(buf, end, "/", default_str_spec);
 
 	for (depth = 0; parent->parent; depth++)
 		parent = parent->parent;
 
 	for ( ; depth >= 0; depth--) {
-		buf = string(buf, end, "/", default_str_spec);
+		buf = valid_string(buf, end, "/", default_str_spec);
 		buf = string(buf, end, device_node_name_for_depth(np, depth),
 			     default_str_spec);
 	}
@@ -1648,10 +1655,10 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	str_spec.field_width = -1;
 
 	if (!IS_ENABLED(CONFIG_OF))
-		return string(buf, end, "(!OF)", spec);
+		return valid_string(buf, end, "(!OF)", spec);
 
 	if ((unsigned long)dn < PAGE_SIZE)
-		return string(buf, end, "(null)", spec);
+		return valid_string(buf, end, "(null)", spec);
 
 	/* simple case without anything any more format specifiers */
 	fmt++;
@@ -1687,7 +1694,7 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 			tbuf[2] = of_node_check_flag(dn, OF_POPULATED) ? 'P' : '-';
 			tbuf[3] = of_node_check_flag(dn, OF_POPULATED_BUS) ? 'B' : '-';
 			tbuf[4] = 0;
-			buf = string(buf, end, tbuf, str_spec);
+			buf = valid_string(buf, end, tbuf, str_spec);
 			break;
 		case 'c':	/* major compatible string */
 			ret = of_property_read_string(dn, "compatible", &p);
@@ -1698,10 +1705,11 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 			has_mult = false;
 			of_property_for_each_string(dn, "compatible", prop, p) {
 				if (has_mult)
-					buf = string(buf, end, ",", str_spec);
-				buf = string(buf, end, "\"", str_spec);
+					buf = valid_string(buf, end, ",",
+							   str_spec);
+				buf = valid_string(buf, end, "\"", str_spec);
 				buf = string(buf, end, p, str_spec);
-				buf = string(buf, end, "\"", str_spec);
+				buf = valid_string(buf, end, "\"", str_spec);
 
 				has_mult = true;
 			}
@@ -1840,7 +1848,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		 */
 		if (spec.field_width == -1)
 			spec.field_width = default_width;
-		return string(buf, end, "(null)", spec);
+		return valid_string(buf, end, "(null)", spec);
 	}
 
 	switch (*fmt) {
@@ -1896,7 +1904,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			case AF_INET6:
 				return ip6_addr_string_sa(buf, end, &sa->v6, spec, fmt);
 			default:
-				return string(buf, end, "(invalid address)", spec);
+				return valid_string(buf, end,
+						    "(invalid address)", spec);
 			}}
 		}
 		break;
-- 
2.13.6

Re: [PATCH v5 04/11] vsprintf: Do not check address of well-known strings

[Andy Shevchenko]

On Wed, 2018-04-25 at 13:12 +0200, Petr Mladek wrote:
> We are going to check the address using probe_kernel_address(). It
> will
> be more expensive and it does not make sense for well known address.
> 
> This patch splits the string() function. The variant without the check
> is then used on locations that handle string constants or strings
> defined
> as local variables.
> 
> This patch does not change the existing behavior.

> -					buf = string(buf, end, ",",
> str_spec);

> +					buf = valid_string(buf, end,
> ",",
> +							   str_spec);

> -				return string(buf, end, "(invalid
> address)", spec);
> +				return valid_string(buf, end,
> +						    "(invalid
> address)", spec);
> 

I wouldn't give a crap about over 80 in above two cases. 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v5 05/11] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

There are few printk formats that make sense only with two or more
specifiers. Also some specifiers make sense only when a kernel feature
is enabled.

The handling of unknown specifiers is strange, inconsistent, and
even leaking the address. For example, netdev_bits() prints the
non-hashed pointer value or clock() prints "(null)".

Using WARN() looks like an overkill for this type of error. pr_warn()
is not good either. It would by handled via printk_sage buffer and
it might be hard to match it with the problematic string.

A reasonable compromise seems to be writing the unknown format specifier
into the original string with a question mark, for example (%pC?).
It should be self-explaining enough. Note that it is in brackets
to follow the (null) style.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 lib/vsprintf.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 5c26a4e71cdf..587175a528b7 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1488,7 +1488,8 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 }
 
 static noinline_for_stack
-char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt)
+char *netdev_bits(char *buf, char *end, const void *addr,
+		  struct printf_spec spec, const char *fmt)
 {
 	unsigned long long num;
 	int size;
@@ -1499,9 +1500,7 @@ char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt)
 		size = sizeof(netdev_features_t);
 		break;
 	default:
-		num = (unsigned long)addr;
-		size = sizeof(unsigned long);
-		break;
+		return valid_string(buf, end, "(%pN?)", spec);
 	}
 
 	return special_hex_number(buf, end, num, size);
@@ -1532,7 +1531,10 @@ static noinline_for_stack
 char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
 	    const char *fmt)
 {
-	if (!IS_ENABLED(CONFIG_HAVE_CLK) || !clk)
+	if (!IS_ENABLED(CONFIG_HAVE_CLK))
+		return valid_string(buf, end, "(%pC?)", spec);
+
+	if (!clk)
 		return string(buf, end, NULL, spec);
 
 	switch (fmt[1]) {
@@ -1544,7 +1546,7 @@ char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
 #ifdef CONFIG_COMMON_CLK
 		return string(buf, end, __clk_get_name(clk), spec);
 #else
-		return special_hex_number(buf, end, (unsigned long)clk, sizeof(unsigned long));
+		return valid_string(buf, end, "(%pC?)", spec);
 #endif
 	}
 }
@@ -1577,7 +1579,8 @@ char *format_flags(char *buf, char *end, unsigned long flags,
 }
 
 static noinline_for_stack
-char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)
+char *flags_string(char *buf, char *end, void *flags_ptr,
+		   struct printf_spec spec, const char *fmt)
 {
 	unsigned long flags;
 	const struct trace_print_flags *names;
@@ -1598,8 +1601,7 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)
 		names = gfpflag_names;
 		break;
 	default:
-		WARN_ONCE(1, "Unsupported flags modifier: %c\n", fmt[1]);
-		return buf;
+		return valid_string(buf, end, "(%pG?)", spec);
 	}
 
 	return format_flags(buf, end, flags, names);
@@ -1655,7 +1657,7 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	str_spec.field_width = -1;
 
 	if (!IS_ENABLED(CONFIG_OF))
-		return valid_string(buf, end, "(!OF)", spec);
+		return valid_string(buf, end, "(%OF?)", spec);
 
 	if ((unsigned long)dn < PAGE_SIZE)
 		return valid_string(buf, end, "(null)", spec);
@@ -1926,7 +1928,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'K':
 		return restricted_pointer(buf, end, ptr, spec);
 	case 'N':
-		return netdev_bits(buf, end, ptr, fmt);
+		return netdev_bits(buf, end, ptr, spec, fmt);
 	case 'a':
 		return address_val(buf, end, ptr, fmt);
 	case 'd':
@@ -1943,7 +1945,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 #endif
 
 	case 'G':
-		return flags_string(buf, end, ptr, fmt);
+		return flags_string(buf, end, ptr, spec, fmt);
 	case 'O':
 		switch (fmt[1]) {
 		case 'F':
-- 
2.13.6

Re: [PATCH v5 05/11] vsprintf: Consolidate handling of unknown pointer specifiers

[Andy Shevchenko]

On Wed, 2018-04-25 at 13:12 +0200, Petr Mladek wrote:
> There are few printk formats that make sense only with two or more
> specifiers. Also some specifiers make sense only when a kernel feature
> is enabled.
> 
> The handling of unknown specifiers is strange, inconsistent, and
> even leaking the address. For example, netdev_bits() prints the
> non-hashed pointer value or clock() prints "(null)".
> 
> Using WARN() looks like an overkill for this type of error. pr_warn()
> is not good either. It would by handled via printk_sage buffer and
> it might be hard to match it with the problematic string.
> 
> A reasonable compromise seems to be writing the unknown format
> specifier
> into the original string with a question mark, for example (%pC?).
> It should be self-explaining enough. Note that it is in brackets
> to follow the (null) style.

> +		return valid_string(buf, end, "(%pG?)", spec);

>  
>  	if (!IS_ENABLED(CONFIG_OF))
> -		return valid_string(buf, end, "(!OF)", spec);
> +		return valid_string(buf, end, "(%OF?)", spec);

"(%pOF?)" ?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v5 06/11] vsprintf: Factor out %p[iI] handler as ip_addr_string()

[Petr Mladek]

Move the non-trivial code from the long pointer() function. We are going
to add a check for the access to the address that will make it even more
complicated.

Also it is better to warn about unknown specifier instead of falling
back to the %p behavior. It will help people to understand what is
going wrong. They expect the IP address and not a pointer anyway
in this situation.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 lib/vsprintf.c | 57 ++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 23 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 587175a528b7..92793060bb1f 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1387,6 +1387,39 @@ char *ip4_addr_string_sa(char *buf, char *end, const struct sockaddr_in *sa,
 }
 
 static noinline_for_stack
+char *ip_addr_string(char *buf, char *end, const void *ptr,
+		     struct printf_spec spec, const char *fmt)
+{
+	char *err_fmt_msg;
+
+	switch (fmt[1]) {
+	case '6':
+		return ip6_addr_string(buf, end, ptr, spec, fmt);
+	case '4':
+		return ip4_addr_string(buf, end, ptr, spec, fmt);
+	case 'S': {
+		const union {
+			struct sockaddr		raw;
+			struct sockaddr_in	v4;
+			struct sockaddr_in6	v6;
+		} *sa = ptr;
+
+		switch (sa->raw.sa_family) {
+		case AF_INET:
+			return ip4_addr_string_sa(buf, end, &sa->v4, spec, fmt);
+		case AF_INET6:
+			return ip6_addr_string_sa(buf, end, &sa->v6, spec, fmt);
+		default:
+			return valid_string(buf, end, "(invalid address)",
+					    spec);
+		}}
+	}
+
+	err_fmt_msg = fmt[0] == 'i' ? "(%pi?)" : "(%pI?)";
+	return valid_string(buf, end, err_fmt_msg, spec);
+}
+
+static noinline_for_stack
 char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 		     const char *fmt)
 {
@@ -1888,29 +1921,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 					 * 4:	001.002.003.004
 					 * 6:   000102...0f
 					 */
-		switch (fmt[1]) {
-		case '6':
-			return ip6_addr_string(buf, end, ptr, spec, fmt);
-		case '4':
-			return ip4_addr_string(buf, end, ptr, spec, fmt);
-		case 'S': {
-			const union {
-				struct sockaddr		raw;
-				struct sockaddr_in	v4;
-				struct sockaddr_in6	v6;
-			} *sa = ptr;
-
-			switch (sa->raw.sa_family) {
-			case AF_INET:
-				return ip4_addr_string_sa(buf, end, &sa->v4, spec, fmt);
-			case AF_INET6:
-				return ip6_addr_string_sa(buf, end, &sa->v6, spec, fmt);
-			default:
-				return valid_string(buf, end,
-						    "(invalid address)", spec);
-			}}
-		}
-		break;
+		return ip_addr_string(buf, end, ptr, spec, fmt);
 	case 'E':
 		return escaped_string(buf, end, ptr, spec, fmt);
 	case 'U':
-- 
2.13.6

Re: [PATCH v5 06/11] vsprintf: Factor out %p[iI] handler as ip_addr_string()

[Andy Shevchenko]

On Wed, 2018-04-25 at 13:12 +0200, Petr Mladek wrote:
> Move the non-trivial code from the long pointer() function. We are
> going
> to add a check for the access to the address that will make it even
> more
> complicated.
> 
> Also it is better to warn about unknown specifier instead of falling
> back to the %p behavior. It will help people to understand what is
> going wrong. They expect the IP address and not a pointer anyway
> in this situation.
> 

Can we split to 1) move out + 2) misc changes ?

> +			return valid_string(buf, end, "(invalid
> address)",
> +					    spec);

I would still put it on one line.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v5 07/11] vsprintf: Factor out %pV handler as va_format()

[Petr Mladek]

Move the code from the long pointer() function. We are going to add a check
for the access to the address that will make it even more complicated.

This patch does not change the existing behavior.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 lib/vsprintf.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 92793060bb1f..e58436ef9f7f 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1479,6 +1479,18 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 	return buf;
 }
 
+static char *va_format(char *buf, char *end, struct va_format *va_fmt)
+{
+	va_list va;
+
+	va_copy(va, *va_fmt->va);
+	buf += vsnprintf(buf, end > buf ? end - buf : 0,
+			 va_fmt->fmt, va);
+	va_end(va);
+
+	return buf;
+}
+
 static noinline_for_stack
 char *uuid_string(char *buf, char *end, const u8 *addr,
 		  struct printf_spec spec, const char *fmt)
@@ -1927,15 +1939,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'U':
 		return uuid_string(buf, end, ptr, spec, fmt);
 	case 'V':
-		{
-			va_list va;
-
-			va_copy(va, *((struct va_format *)ptr)->va);
-			buf += vsnprintf(buf, end > buf ? end - buf : 0,
-					 ((struct va_format *)ptr)->fmt, va);
-			va_end(va);
-			return buf;
-		}
+		return va_format(buf, end, ptr);
 	case 'K':
 		return restricted_pointer(buf, end, ptr, spec);
 	case 'N':
-- 
2.13.6

Re: [PATCH v5 07/11] vsprintf: Factor out %pV handler as va_format()

[Andy Shevchenko]

On Wed, 2018-04-25 at 13:12 +0200, Petr Mladek wrote:
> Move the code from the long pointer() function. We are going to add a
> check
> for the access to the address that will make it even more complicated.
> 
> This patch does not change the existing behavior.
 
> +static char *va_format(char *buf, char *end, struct va_format
> *va_fmt)
> +{
> +	va_list va;
> +
> +	va_copy(va, *va_fmt->va);

> +	buf += vsnprintf(buf, end > buf ? end - buf : 0,
> +			 va_fmt->fmt, va);

One line?

> +	va_end(va);
> +
> +	return buf;
> +}

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v5 08/11] vsprintf: Factor out %pO handler as kobject_string()

[Petr Mladek]

Move code from the long pointer() function. We are going to add a check for
the access to the address that will make it even more complicated.

Also it is better to warn about unknown specifier instead of falling
back to the %p behavior. It will help people to understand what is
going wrong. They expect some device node names and not a pointer
in this situation.

In fact, this avoids leaking the address when invalid %pO format
specifier is used. The old code fallen back to printing the
non-hashed value.

Fixes: commit 7b1924a1d930eb27f ("vsprintf: add printk specifier %px")
Signed-off-by: Petr Mladek <pmladek@suse.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Tobin Harding <me@tobin.cc>
Cc: Kees Cook <keescook@chromium.org>
---
 lib/vsprintf.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index e58436ef9f7f..3536796c483c 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1769,6 +1769,17 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
+static char *kobject_string(char *buf, char *end, void *ptr,
+			    struct printf_spec spec, const char *fmt)
+{
+	switch (fmt[1]) {
+	case 'F':
+		return device_node_string(buf, end, ptr, spec, fmt + 1);
+	}
+
+	return valid_string(buf, end, "(%pO?)", spec);
+}
+
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
@@ -1962,10 +1973,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'G':
 		return flags_string(buf, end, ptr, spec, fmt);
 	case 'O':
-		switch (fmt[1]) {
-		case 'F':
-			return device_node_string(buf, end, ptr, spec, fmt + 1);
-		}
+		return kobject_string(buf, end, ptr, spec, fmt);
 	case 'x':
 		return pointer_string(buf, end, ptr, spec);
 	}
-- 
2.13.6

Re: [PATCH v5 08/11] vsprintf: Factor out %pO handler as kobject_string()

[Andy Shevchenko]

On Wed, 2018-04-25 at 13:12 +0200, Petr Mladek wrote:
> Move code from the long pointer() function. We are going to add a
> check for
> the access to the address that will make it even more complicated.
> 
> Also it is better to warn about unknown specifier instead of falling
> back to the %p behavior. It will help people to understand what is
> going wrong. They expect some device node names and not a pointer
> in this situation.
> 
> In fact, this avoids leaking the address when invalid %pO format
> specifier is used. The old code fallen back to printing the
> non-hashed value.
> 

> +static char *kobject_string(char *buf, char *end, void *ptr,
> +			    struct printf_spec spec, const char *fmt)

Do we need noinline_for_stack annotation? (Same question applies to
patch 7)

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v5 09/11] vsprintf: Prevent crash when dereferencing invalid pointers

[Petr Mladek]

We already prevent crash when dereferencing some obviously broken
pointers. But the handling is not consistent. Sometimes we print "(null)"
only for pure NULL pointer, sometimes for pointers in the first
page and sometimes also for pointers in the last page (error codes).

Note that printk() call this code under logbuf_lock. Any recursive
printks are redirected to the printk_safe implementation and the messages
are stored into per-CPU buffers. These buffers might be eventually flushed
in printk_safe_flush_on_panic() but it is not guaranteed.

This patch adds a check using probe_kernel_read(). It is not a full-proof
test. But it should help to see the error message in 99% situations where
the kernel would silently crash otherwise.

Also it makes the error handling unified for "%s" and the many %p*
specifiers that need to read the data from a given address. We print:

   + (null)   when accessing data on pure pure NULL address
   + (efault) when accessing data on an invalid address

It does not affect the %p* specifiers that just print the given address
in some form, namely %pF, %pf, %pS, %ps, %pB, %pK, %px, and plain %p.

Note that we print (efault) from security reasons. In fact, the real
address can be seen only by %px or eventually %pK.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 Documentation/core-api/printk-formats.rst |   7 ++
 lib/test_printf.c                         |  37 +++++++--
 lib/vsprintf.c                            | 128 ++++++++++++++++++++++--------
 3 files changed, 131 insertions(+), 41 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index eb30efdd2e78..7c73bed2fad8 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -50,6 +50,13 @@ A raw pointer value may be printed with %p which will hash the address
 before printing. The kernel also supports extended specifiers for printing
 pointers of different types.
 
+Some of the extended specifiers print the data on the given address instead
+of printing the address itself. In this case, the following error messages
+might be printed instead of the unreachable information::
+
+	(null)	 data on plain NULL address
+	(efault) data on invalid address
+
 Plain Pointers
 --------------
 
diff --git a/lib/test_printf.c b/lib/test_printf.c
index cea592f402ed..45c33143fb4a 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -209,12 +209,12 @@ test_string(void)
 #define ZEROS "00000000"	/* hex 32 zero bits */
 
 static int __init
-plain_format(void)
+plain_format(void *ptr)
 {
 	char buf[PLAIN_BUF_SIZE];
 	int nchars;
 
-	nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR);
+	nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", ptr);
 
 	if (nchars != PTR_WIDTH || strncmp(buf, ZEROS, strlen(ZEROS)) != 0)
 		return -1;
@@ -227,6 +227,7 @@ plain_format(void)
 #define PTR_WIDTH 8
 #define PTR ((void *)0x456789ab)
 #define PTR_STR "456789ab"
+#define ZEROS ""
 
 static int __init
 plain_format(void)
@@ -238,12 +239,12 @@ plain_format(void)
 #endif	/* BITS_PER_LONG == 64 */
 
 static int __init
-plain_hash(void)
+plain_hash(void *ptr)
 {
 	char buf[PLAIN_BUF_SIZE];
 	int nchars;
 
-	nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR);
+	nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", ptr);
 
 	if (nchars != PTR_WIDTH || strncmp(buf, PTR_STR, PTR_WIDTH) == 0)
 		return -1;
@@ -256,18 +257,18 @@ plain_hash(void)
  * after an address is hashed.
  */
 static void __init
-plain(void)
+plain(void *ptr)
 {
 	int err;
 
-	err = plain_hash();
+	err = plain_hash(ptr);
 	if (err) {
 		pr_warn("plain 'p' does not appear to be hashed\n");
 		failed_tests++;
 		return;
 	}
 
-	err = plain_format();
+	err = plain_format(ptr);
 	if (err) {
 		pr_warn("hashing plain 'p' has unexpected format\n");
 		failed_tests++;
@@ -275,6 +276,24 @@ plain(void)
 }
 
 static void __init
+null_pointer(void)
+{
+	plain(NULL);
+	test(ZEROS "00000000", "%px", NULL);
+	test("(null)", "%pE", NULL);
+}
+
+#define PTR_INVALID ((void *)0x000000ab)
+
+static void __init
+invalid_pointer(void)
+{
+	plain(PTR_INVALID);
+	test(ZEROS "000000ab", "%px", PTR_INVALID);
+	test("(efault)", "%pE", PTR_INVALID);
+}
+
+static void __init
 symbol_ptr(void)
 {
 }
@@ -497,7 +516,9 @@ flags(void)
 static void __init
 test_pointer(void)
 {
-	plain();
+	plain(PTR);
+	null_pointer();
+	invalid_pointer();
 	symbol_ptr();
 	kernel_ptr();
 	struct_resource();
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3536796c483c..5dfdc7e11d05 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -610,12 +610,45 @@ static char *valid_string(char *buf, char *end, const char *s,
 	return widen_string(buf, len, end, spec);
 }
 
+ /*
+  * This is not a fool-proof test. 99% of the time that this will fault is
+  * due to a bad pointer, not one that crosses into bad memory. Just test
+  * the address to make sure it doesn't fault due to a poorly added printk
+  * during debugging.
+  */
+static const char *check_pointer_access(const void *ptr)
+{
+	char byte;
+
+	if (!ptr)
+		return "(null)";
+
+	if (probe_kernel_address(ptr, byte))
+		return "(efault)";
+
+	return NULL;
+}
+
+static bool valid_pointer_access(char **buf, char *end, const void *ptr,
+				 struct printf_spec spec)
+{
+	const char *err_msg;
+
+	err_msg = check_pointer_access(ptr);
+	if (err_msg) {
+		*buf = valid_string(*buf, end, err_msg, spec);
+		return false;
+	}
+
+	return true;
+}
+
 static noinline_for_stack
 char *string(char *buf, char *end, const char *s,
 		   struct printf_spec spec)
 {
-	if ((unsigned long)s < PAGE_SIZE)
-		s = "(null)";
+	if (!valid_pointer_access(&buf, end, s, spec))
+		return buf;
 
 	return valid_string(buf, end, s, spec);
 }
@@ -761,6 +794,11 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
 
 	rcu_read_lock();
 	for (i = 0; i < depth; i++, d = p) {
+		if (!valid_pointer_access(&buf, end, d, spec)) {
+			rcu_read_unlock();
+			return buf;
+		}
+
 		p = READ_ONCE(d->d_parent);
 		array[i] = READ_ONCE(d->d_name.name);
 		if (p == d) {
@@ -791,8 +829,12 @@ static noinline_for_stack
 char *bdev_name(char *buf, char *end, struct block_device *bdev,
 		struct printf_spec spec, const char *fmt)
 {
-	struct gendisk *hd = bdev->bd_disk;
-	
+	struct gendisk *hd;
+
+	if (!valid_pointer_access(&buf, end, bdev, spec))
+		return buf;
+
+	hd = bdev->bd_disk;
 	buf = string(buf, end, hd->disk_name, spec);
 	if (bdev->bd_part->partno) {
 		if (isdigit(hd->disk_name[strlen(hd->disk_name)-1])) {
@@ -897,6 +939,9 @@ char *resource_string(char *buf, char *end, struct resource *res,
 	int decode = (fmt[0] == 'R') ? 1 : 0;
 	const struct printf_spec *specp;
 
+	if (!valid_pointer_access(&buf, end, res, spec))
+		return buf;
+
 	*p++ = '[';
 	if (res->flags & IORESOURCE_IO) {
 		p = valid_string(p, pend, "io  ", str_spec);
@@ -959,9 +1004,8 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 		/* nothing to print */
 		return buf;
 
-	if (ZERO_OR_NULL_PTR(addr))
-		/* NULL pointer */
-		return string(buf, end, NULL, spec);
+	if (!valid_pointer_access(&buf, end, addr, spec))
+		return buf;
 
 	switch (fmt[1]) {
 	case 'C':
@@ -1008,6 +1052,9 @@ char *bitmap_string(char *buf, char *end, unsigned long *bitmap,
 	int i, chunksz;
 	bool first = true;
 
+	if (!valid_pointer_access(&buf, end, bitmap, spec))
+		return buf;
+
 	/* reused to print numbers */
 	spec = (struct printf_spec){ .flags = SMALL | ZEROPAD, .base = 16 };
 
@@ -1049,6 +1096,9 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap,
 	int cur, rbot, rtop;
 	bool first = true;
 
+	if (!valid_pointer_access(&buf, end, bitmap, spec))
+		return buf;
+
 	rbot = cur = find_first_bit(bitmap, nr_bits);
 	while (cur < nr_bits) {
 		rtop = cur;
@@ -1087,6 +1137,9 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
 	char separator;
 	bool reversed = false;
 
+	if (!valid_pointer_access(&buf, end, addr, spec))
+		return buf;
+
 	switch (fmt[1]) {
 	case 'F':
 		separator = '-';
@@ -1387,11 +1440,14 @@ char *ip4_addr_string_sa(char *buf, char *end, const struct sockaddr_in *sa,
 }
 
 static noinline_for_stack
-char *ip_addr_string(char *buf, char *end, const void *ptr,
-		     struct printf_spec spec, const char *fmt)
+char *ip_addr_string(char *buf, char *end, void *ptr, struct printf_spec spec,
+		     const char *fmt)
 {
 	char *err_fmt_msg;
 
+	if (!valid_pointer_access(&buf, end, ptr, spec))
+		return buf;
+
 	switch (fmt[1]) {
 	case '6':
 		return ip6_addr_string(buf, end, ptr, spec, fmt);
@@ -1431,9 +1487,8 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 	if (spec.field_width == 0)
 		return buf;				/* nothing to print */
 
-	if (ZERO_OR_NULL_PTR(addr))
-		return string(buf, end, NULL, spec);	/* NULL pointer */
-
+	if (!valid_pointer_access(&buf, end, addr, spec))
+		return buf;
 
 	do {
 		switch (fmt[count++]) {
@@ -1479,10 +1534,14 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 	return buf;
 }
 
-static char *va_format(char *buf, char *end, struct va_format *va_fmt)
+static char *va_format(char *buf, char *end, struct va_format *va_fmt,
+		       struct printf_spec spec, const char *fmt)
 {
 	va_list va;
 
+	if (!valid_pointer_access(&buf, end, va_fmt, spec))
+		return buf;
+
 	va_copy(va, *va_fmt->va);
 	buf += vsnprintf(buf, end > buf ? end - buf : 0,
 			 va_fmt->fmt, va);
@@ -1501,6 +1560,9 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 	const u8 *index = uuid_index;
 	bool uc = false;
 
+	if (!valid_pointer_access(&buf, end, addr, spec))
+		return buf;
+
 	switch (*(++fmt)) {
 	case 'L':
 		uc = true;		/* fall-through */
@@ -1539,6 +1601,9 @@ char *netdev_bits(char *buf, char *end, const void *addr,
 	unsigned long long num;
 	int size;
 
+	if (!valid_pointer_access(&buf, end, addr, spec))
+		return buf;
+
 	switch (fmt[1]) {
 	case 'F':
 		num = *(const netdev_features_t *)addr;
@@ -1552,11 +1617,15 @@ char *netdev_bits(char *buf, char *end, const void *addr,
 }
 
 static noinline_for_stack
-char *address_val(char *buf, char *end, const void *addr, const char *fmt)
+char *address_val(char *buf, char *end, const void *addr,
+		  struct printf_spec spec, const char *fmt)
 {
 	unsigned long long num;
 	int size;
 
+	if (!valid_pointer_access(&buf, end, addr, spec))
+		return buf;
+
 	switch (fmt[1]) {
 	case 'd':
 		num = *(const dma_addr_t *)addr;
@@ -1579,8 +1648,8 @@ char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
 	if (!IS_ENABLED(CONFIG_HAVE_CLK))
 		return valid_string(buf, end, "(%pC?)", spec);
 
-	if (!clk)
-		return string(buf, end, NULL, spec);
+	if (!valid_pointer_access(&buf, end, clk, spec))
+		return buf;
 
 	switch (fmt[1]) {
 	case 'r':
@@ -1630,6 +1699,9 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
 	unsigned long flags;
 	const struct trace_print_flags *names;
 
+	if (!valid_pointer_access(&buf, end, flags_ptr, spec))
+		return buf;
+
 	switch (fmt[1]) {
 	case 'p':
 		flags = *(unsigned long *)flags_ptr;
@@ -1897,18 +1969,6 @@ static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	      struct printf_spec spec)
 {
-	const int default_width = 2 * sizeof(void *);
-
-	if (!ptr && *fmt != 'K' && *fmt != 'x') {
-		/*
-		 * Print (null) with the same width as a pointer so it makes
-		 * tabular output look nice.
-		 */
-		if (spec.field_width == -1)
-			spec.field_width = default_width;
-		return valid_string(buf, end, "(null)", spec);
-	}
-
 	switch (*fmt) {
 	case 'F':
 	case 'f':
@@ -1950,13 +2010,13 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'U':
 		return uuid_string(buf, end, ptr, spec, fmt);
 	case 'V':
-		return va_format(buf, end, ptr);
+		return va_format(buf, end, ptr, spec, fmt);
 	case 'K':
 		return restricted_pointer(buf, end, ptr, spec);
 	case 'N':
 		return netdev_bits(buf, end, ptr, spec, fmt);
 	case 'a':
-		return address_val(buf, end, ptr, fmt);
+		return address_val(buf, end, ptr, spec, fmt);
 	case 'd':
 		return dentry_name(buf, end, ptr, spec, fmt);
 	case 'C':
@@ -2588,11 +2648,13 @@ int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args)
 
 		case FORMAT_TYPE_STR: {
 			const char *save_str = va_arg(args, char *);
+			const char *err_msg;
 			size_t len;
 
-			if ((unsigned long)save_str > (unsigned long)-PAGE_SIZE
-					|| (unsigned long)save_str < PAGE_SIZE)
-				save_str = "(null)";
+			err_msg = check_pointer_access(save_str);
+			if (err_msg)
+				save_str = err_msg;
+
 			len = strlen(save_str) + 1;
 			if (str + len < end)
 				memcpy(str, save_str, len);
-- 
2.13.6

Re: [PATCH v5 09/11] vsprintf: Prevent crash when dereferencing invalid pointers

[Andy Shevchenko]

On Wed, 2018-04-25 at 13:12 +0200, Petr Mladek wrote:
> We already prevent crash when dereferencing some obviously broken
> pointers. But the handling is not consistent. Sometimes we print
> "(null)"
> only for pure NULL pointer, sometimes for pointers in the first
> page and sometimes also for pointers in the last page (error codes).
> 
> Note that printk() call this code under logbuf_lock. Any recursive
> printks are redirected to the printk_safe implementation and the
> messages
> are stored into per-CPU buffers. These buffers might be eventually
> flushed
> in printk_safe_flush_on_panic() but it is not guaranteed.
> 
> This patch adds a check using probe_kernel_read(). It is not a full-
> proof
> test. But it should help to see the error message in 99% situations
> where
> the kernel would silently crash otherwise.
> 
> Also it makes the error handling unified for "%s" and the many %p*
> specifiers that need to read the data from a given address. We print:
> 
>    + (null)   when accessing data on pure pure NULL address
>    + (efault) when accessing data on an invalid address
> 
> It does not affect the %p* specifiers that just print the given
> address
> in some form, namely %pF, %pf, %pS, %ps, %pB, %pK, %px, and plain %p.
> 
> Note that we print (efault) from security reasons. In fact, the real
> address can be seen only by %px or eventually %pK.


> +static const char *check_pointer_access(const void *ptr)
> +{
> +	char byte;
> +
> +	if (!ptr)
> +		return "(null)";
> +
> +	if (probe_kernel_address(ptr, byte))
> +		return "(efault)";
> +
> +	return NULL;
> +}
> +
> +static bool valid_pointer_access(char **buf, char *end, const void
> *ptr,
> +				 struct printf_spec spec)
> +{
> +	const char *err_msg;
> +
> +	err_msg = check_pointer_access(ptr);
> +	if (err_msg) {
> +		*buf = valid_string(*buf, end, err_msg, spec);
> +		return false;
> +	}
> +
> +	return true;
> +}

I would preserve similar style of buf pointer handling, i.e.

static char *valid_pointer_access(char **buf, char *end,
				  const void *ptr, struct printf_spec spec)
{
	const char *err_msg;

	err_msg = check_pointer_access(ptr);
	if (err_msg)
		return = valid_string(*buf, end, err_msg, spec);

	return NULL;
}

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v5 09/11] vsprintf: Prevent crash when dereferencing invalid pointers

[Andy Shevchenko]

On Wed, 2018-04-25 at 18:10 +0300, Andy Shevchenko wrote:

Some typos fixed

> I would preserve similar style of buf pointer handling, i.e.
> 
> static char *valid_pointer_access(char **buf, char *end,

char *buf

> 				  const void *ptr, struct printf_spec
> spec)
> {
> 	const char *err_msg;
> 
> 	err_msg = check_pointer_access(ptr);
> 	if (err_msg)
> 		return = valid_string(*buf, end, err_msg, spec);

		return valid_string(buf, end, err_msg, spec);

> 	return NULL;
> }
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v5 09/11] vsprintf: Prevent crash when dereferencing invalid pointers

[Petr Mladek]

On Wed 2018-04-25 18:10:54, Andy Shevchenko wrote:
> On Wed, 2018-04-25 at 13:12 +0200, Petr Mladek wrote:
> > We already prevent crash when dereferencing some obviously broken
> > pointers. But the handling is not consistent. Sometimes we print
> > "(null)"
> > only for pure NULL pointer, sometimes for pointers in the first
> > page and sometimes also for pointers in the last page (error codes).
> > 
> > Note that printk() call this code under logbuf_lock. Any recursive
> > printks are redirected to the printk_safe implementation and the
> > messages
> > are stored into per-CPU buffers. These buffers might be eventually
> > flushed
> > in printk_safe_flush_on_panic() but it is not guaranteed.
> 
> > +static const char *check_pointer_access(const void *ptr)
> > +{
> > +	char byte;
> > +
> > +	if (!ptr)
> > +		return "(null)";
> > +
> > +	if (probe_kernel_address(ptr, byte))
> > +		return "(efault)";
> > +
> > +	return NULL;
> > +}
> > +
> > +static bool valid_pointer_access(char **buf, char *end, const void
> > *ptr,
> > +				 struct printf_spec spec)
> > +{
> > +	const char *err_msg;
> > +
> > +	err_msg = check_pointer_access(ptr);
> > +	if (err_msg) {
> > +		*buf = valid_string(*buf, end, err_msg, spec);
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> 
> I would preserve similar style of buf pointer handling, i.e.
> 
> static char *valid_pointer_access(char **buf, char *end,
> 				  const void *ptr, struct printf_spec spec)
> {
> 	const char *err_msg;
> 
> 	err_msg = check_pointer_access(ptr);
> 	if (err_msg)
> 		return = valid_string(*buf, end, err_msg, spec);
> 
> 	return NULL;
> }

Heh, I actually started with exactly this code. But it caused confusion.
The name suggests that it should return true on success and NULL
is false:

	if (!valid_pointer_access())
		return err;

Any better naming/code is welcome.

Best Reagrds,
Petr

Re: [PATCH v5 09/11] vsprintf: Prevent crash when dereferencing invalid pointers

[Andy Shevchenko]

On Fri, 2018-04-27 at 14:47 +0200, Petr Mladek wrote:
> On Wed 2018-04-25 18:10:54, Andy Shevchenko wrote:
> > On Wed, 2018-04-25 at 13:12 +0200, Petr Mladek wrote:
> > > We already prevent crash when dereferencing some obviously broken
> > > pointers. But the handling is not consistent. Sometimes we print
> > > "(null)"
> > > only for pure NULL pointer, sometimes for pointers in the first
> > > page and sometimes also for pointers in the last page (error
> > > codes).
> > > 
> > > Note that printk() call this code under logbuf_lock. Any recursive
> > > printks are redirected to the printk_safe implementation and the
> > > messages
> > > are stored into per-CPU buffers. These buffers might be eventually
> > > flushed
> > > in printk_safe_flush_on_panic() but it is not guaranteed.
> > > +static const char *check_pointer_access(const void *ptr)
> > > +{
> > > +	char byte;
> > > +
> > > +	if (!ptr)
> > > +		return "(null)";
> > > +
> > > +	if (probe_kernel_address(ptr, byte))
> > > +		return "(efault)";
> > > +
> > > +	return NULL;
> > > +}
> > > +
> > > +static bool valid_pointer_access(char **buf, char *end, const
> > > void
> > > *ptr,
> > > +				 struct printf_spec spec)
> > > +{
> > > +	const char *err_msg;
> > > +
> > > +	err_msg = check_pointer_access(ptr);
> > > +	if (err_msg) {
> > > +		*buf = valid_string(*buf, end, err_msg, spec);
> > > +		return false;
> > > +	}
> > > +
> > > +	return true;
> > > +}
> > 
> > I would preserve similar style of buf pointer handling, i.e.
> > 
> > static char *valid_pointer_access(char **buf, char *end,
> > 				  const void *ptr, struct printf_spec
> > spec)
> > {
> > 	const char *err_msg;
> > 
> > 	err_msg = check_pointer_access(ptr);
> > 	if (err_msg)
> > 		return = valid_string(*buf, end, err_msg, spec);
> > 
> > 	return NULL;
> > }
> 
> Heh, I actually started with exactly this code. But it caused
> confusion.
> The name suggests that it should return true on success and NULL
> is false:
> 
> 	if (!valid_pointer_access())
> 		return err;

Confusion is already created by valid_string() to return char *.

> Any better naming/code is welcome.

Have nothing in my mind currently.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v5 09/11] vsprintf: Prevent crash when dereferencing invalid pointers

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1679 bytes --]

Hi Petr,

I love your patch! Yet something to improve:

[auto build test ERROR on next-20180424]
[cannot apply to linus/master v4.17-rc2 v4.17-rc1 v4.16 v4.17-rc2]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Petr-Mladek/vsprintf-Prevent-silent-crashes-and-consolidate-error-handling/20180427-044114
config: i386-randconfig-x000-201816 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   lib/test_printf.c: In function 'plain':
>> lib/test_printf.c:271:8: error: too many arguments to function 'plain_format'
     err = plain_format(ptr);
           ^~~~~~~~~~~~
   lib/test_printf.c:233:1: note: declared here
    plain_format(void)
    ^~~~~~~~~~~~

vim +/plain_format +271 lib/test_printf.c

   254	
   255	/*
   256	 * We can't use test() to test %p because we don't know what output to expect
   257	 * after an address is hashed.
   258	 */
   259	static void __init
   260	plain(void *ptr)
   261	{
   262		int err;
   263	
   264		err = plain_hash(ptr);
   265		if (err) {
   266			pr_warn("plain 'p' does not appear to be hashed\n");
   267			failed_tests++;
   268			return;
   269		}
   270	
 > 271		err = plain_format(ptr);
   272		if (err) {
   273			pr_warn("hashing plain 'p' has unexpected format\n");
   274			failed_tests++;
   275		}
   276	}
   277	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27160 bytes --]

[PATCH v5 10/11] vsprintf: WARN() on invalid pointer access

[Petr Mladek]

vsprintf puts "(efault)" into the output string when it is unable
to read information from the given address.

But "(efault)" might be hard to spot. And any invalid pointer is likely
to cause problems later. It is reasonable to WARN() about it.

The only problem might be a code that rely on the fact that some
specifiers, e.g. %s, printed (null) for invalid addresses pointing
to the first or the last memory page. Such a behavior should be
avoided. But it is the reason why this change is done in a separate
patch so that it can be easily reverted.

Also we must not trigger WARN_ON() when panic_on_warn() is enabled.
Note that probe_kernel_address() was added to avoid panic() and
a potentially silent crash in printk_safe context.

Finally, we want to avoid WARN() also when testing invalid pointer access
in test_printf module. It would taint kernel and mess the kernel log.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 Documentation/core-api/printk-formats.rst | 3 +++
 lib/test_printf.c                         | 7 +++++++
 lib/vsprintf.c                            | 9 ++++++++-
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 7c73bed2fad8..3b25adde1ec7 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -57,6 +57,9 @@ might be printed instead of the unreachable information::
 	(null)	 data on plain NULL address
 	(efault) data on invalid address
 
+Also a WARN_ON() is triggered when non-NULL address is not reachable
+and panic_on_warn is disabled.
+
 Plain Pointers
 --------------
 
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 45c33143fb4a..74dff6c44ec6 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -285,12 +285,19 @@ null_pointer(void)
 
 #define PTR_INVALID ((void *)0x000000ab)
 
+extern int test_printf_pointer_access;
+
 static void __init
 invalid_pointer(void)
 {
+	/* Avoid calling WARN() */
+	test_printf_pointer_access = 1;
+
 	plain(PTR_INVALID);
 	test(ZEROS "000000ab", "%px", PTR_INVALID);
 	test("(efault)", "%pE", PTR_INVALID);
+
+	test_printf_pointer_access = 0;
 }
 
 static void __init
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 5dfdc7e11d05..46e3e7c71229 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -610,6 +610,9 @@ static char *valid_string(char *buf, char *end, const char *s,
 	return widen_string(buf, len, end, spec);
 }
 
+int test_printf_pointer_access;
+EXPORT_SYMBOL_GPL(test_printf_pointer_access);
+
  /*
   * This is not a fool-proof test. 99% of the time that this will fault is
   * due to a bad pointer, not one that crosses into bad memory. Just test
@@ -623,8 +626,12 @@ static const char *check_pointer_access(const void *ptr)
 	if (!ptr)
 		return "(null)";
 
-	if (probe_kernel_address(ptr, byte))
+	/* Prevent silent crashes when called in printk_safe context. */
+	if (probe_kernel_address(ptr, byte)) {
+		WARN(!panic_on_warn && !test_printf_pointer_access,
+		     "vsprintf: invalid pointer address\n");
 		return "(efault)";
+	}
 
 	return NULL;
 }
-- 
2.13.6

Re: [PATCH v5 10/11] vsprintf: WARN() on invalid pointer access

[Rasmus Villemoes]

On 2018-04-25 13:12, Petr Mladek wrote:

>  
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 45c33143fb4a..74dff6c44ec6 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -285,12 +285,19 @@ null_pointer(void)
>  
>  #define PTR_INVALID ((void *)0x000000ab)
>  
> +extern int test_printf_pointer_access;
> +
>  static void __init
>  invalid_pointer(void)
>  {
> +	/* Avoid calling WARN() */
> +	test_printf_pointer_access = 1;
> +
>  	plain(PTR_INVALID);
>  	test(ZEROS "000000ab", "%px", PTR_INVALID);
>  	test("(efault)", "%pE", PTR_INVALID);
> +
> +	test_printf_pointer_access = 0;
>  }
>  
>  static void __init
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 5dfdc7e11d05..46e3e7c71229 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -610,6 +610,9 @@ static char *valid_string(char *buf, char *end, const char *s,
>  	return widen_string(buf, len, end, spec);
>  }
>  
> +int test_printf_pointer_access;
> +EXPORT_SYMBOL_GPL(test_printf_pointer_access);
> +

I understand that the printf test module needs this hook, and I
considered adding similar things for other tests when I originally wrote
the printf test module. But can we please make that

#if IS_ENABLED(CONFIG_TEST_PRINTF)
int test_printf_pointer_access;
EXPORT_SYMBOL_GPL(test_printf_pointer_access);
#else
#define test_printf_pointer_access 0
#endif

and maybe also wrap the EXPORT_SYMBOL_GPL in another #if
IS_MODULE(CONFIG_TEST_PRINTF). It's not something random modules should
play with, and I'd hate adding ~100 bytes (or whatever the export
metadata uses these days) to vmlinux for the sake of a module that is
most likely not built at all.

Rasmus

Re: [PATCH v5 10/11] vsprintf: WARN() on invalid pointer access

[Sergey Senozhatsky]

On (04/25/18 13:12), Petr Mladek wrote:
[..]
>   /*
>    * This is not a fool-proof test. 99% of the time that this will fault is
>    * due to a bad pointer, not one that crosses into bad memory. Just test
> @@ -623,8 +626,12 @@ static const char *check_pointer_access(const void *ptr)
>  	if (!ptr)
>  		return "(null)";
>  
> -	if (probe_kernel_address(ptr, byte))
> +	/* Prevent silent crashes when called in printk_safe context. */
> +	if (probe_kernel_address(ptr, byte)) {
> +		WARN(!panic_on_warn && !test_printf_pointer_access,
> +		     "vsprintf: invalid pointer address\n");
>  		return "(efault)";
> +	}

Can we have a rate-limited print out here? Or may be even a WARN_ONCE()?
Yes, printk()-s from check_pointer_access() are OK, printk_safe() helps us,
but at the same time every single invalid pointer access printk()-message
will log_store() WARN() extra entries. Theoretically, this can harm. What
do you think?

	-ss

Re: [PATCH v5 10/11] vsprintf: WARN() on invalid pointer access

[Petr Mladek]

On Thu 2018-04-26 10:28:05, Sergey Senozhatsky wrote:
> On (04/25/18 13:12), Petr Mladek wrote:
> [..]
> >   /*
> >    * This is not a fool-proof test. 99% of the time that this will fault is
> >    * due to a bad pointer, not one that crosses into bad memory. Just test
> > @@ -623,8 +626,12 @@ static const char *check_pointer_access(const void *ptr)
> >  	if (!ptr)
> >  		return "(null)";
> >  
> > -	if (probe_kernel_address(ptr, byte))
> > +	/* Prevent silent crashes when called in printk_safe context. */
> > +	if (probe_kernel_address(ptr, byte)) {
> > +		WARN(!panic_on_warn && !test_printf_pointer_access,
> > +		     "vsprintf: invalid pointer address\n");
> >  		return "(efault)";
> > +	}
> 
> Can we have a rate-limited print out here? Or may be even a WARN_ONCE()?
> Yes, printk()-s from check_pointer_access() are OK, printk_safe() helps us,
> but at the same time every single invalid pointer access printk()-message
> will log_store() WARN() extra entries. Theoretically, this can harm. What
> do you think?

I believe that these WARNs will be rare. After all they happen in situations
where the kernel crashed so far.

I suggest to keep it as is for now. We could always ratelimit it later if
needed.

Best Regards,
Petr

[PATCH v5 11/11] vsprintf: Avoid confusion between invalid address and value

[Petr Mladek]

We are able to detect invalid values handled by %p[iI] printk specifier.
The current error message is "invalid address". It might cause confusion
against "(efault)" reported by the generic valid_pointer_address() check.

Let's unify the style and use the more appropriate error code description
"(einval)".

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 Documentation/core-api/printk-formats.rst | 1 +
 lib/vsprintf.c                            | 3 +--
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 3b25adde1ec7..e1a0ef2179f0 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -56,6 +56,7 @@ might be printed instead of the unreachable information::
 
 	(null)	 data on plain NULL address
 	(efault) data on invalid address
+	(einval) invalid data on a valid address
 
 Also a WARN_ON() is triggered when non-NULL address is not reachable
 and panic_on_warn is disabled.
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 46e3e7c71229..79ee96afd538 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1473,8 +1473,7 @@ char *ip_addr_string(char *buf, char *end, void *ptr, struct printf_spec spec,
 		case AF_INET6:
 			return ip6_addr_string_sa(buf, end, &sa->v6, spec, fmt);
 		default:
-			return valid_string(buf, end, "(invalid address)",
-					    spec);
+			return valid_string(buf, end, "(einval)", spec);
 		}}
 	}
 
-- 
2.13.6

Re: [PATCH v5 00/11] vsprintf: Prevent silent crashes and consolidate error handling

[Petr Mladek]

On Wed 2018-04-25 13:12:40, Petr Mladek wrote:
> Crash in vsprintf() might be silent when it happens under logbuf_lock
> in vprintk_emit(). This patch set prevents most of the crashes by probing
> the address. The check is done only by %s and some %p* specifiers that need
> to dereference the address.
> 
> Only the first byte of the address is checked to keep it simple. It should
> be enough to catch most problems.
> 
> The check is explicitly done in each function that does the dereference.
> It helps to avoid the questionable strchr() of affected specifiers. This
> change motivated me to do some preparation patches that consolidated
> the error handling and cleaned the code a bit.
> 
> I did my best to address the feedback. Note that there is still the
> (efault) error message. But it is accompanied with WARN() when
> panic_on_warn is not enabled. I hope that it makes it more acceptable.
> 
> 
> Changes against v4:
> 
> 	+ rebased on top of
> git://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git for-4.18
> 	+ Added missing conts into ptr_to_ind() in a separate patch
> 	+ Renamed __string to valid_string()
> 	+ Avoid WARN() for invalid poimter specifiers
> 	+ Removed noinline_for_stack where it was not really useful
> 	+ WARN() when accessing invalid non-NULL address

Thanks a lot everyone for feedback. I'll incorporate it into v6. It
might take some time.

BTW: I also got report from 0day robot about that the size of vmlinux
increased by 545 bytes in i386-tinyconfig. I guess that it is mainly
because all the copies of

	if (!valid_pointer_access(&buf, end, bdev, spec))
		return buf;

got inlined. I guess that I would need to address it somehow
as well.

Best Regards,
Petr