[PATCH v6 0/9] vsprintf: Prevent silent crashes and consolidate error handling

[PATCH v6 0/9] 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 against v5.


Note: Kbuild test robot reported slightly increased size of
      the tiny kernel on i386. I was able to get close to
      the original size with some dirty hacks. I think that
      the 0.03% sise is not worth it.

      Also I tried to remove all noinline_for_stack annotations.
      It surprisingly increased the size of the tiny kernel on i386.
      In addition, everything got inlined into pointer() function.
      It became a huge and hard to debug beast. I tend to leave it
      as is for now.


Changes against v5:

	+ Rebased on top of current Linus' tree
	+ Removed already included patch adding const qualifier
	  to ptr_to_id()
	+ Removed controversial patch adding WARN() on invalid pointers
	  [Rasmus, Sergey, Andy]
	+ Reshuffled changes to do all refactoring first.
	+ Added handling also for the new %pt* modifiers
	+ Use better descriptive function names [Andy]:
		+ valid_string() -> string_nocheck()
		+ valid_pointer_access() -> check_pointer()
		+ check_pointer_access() -> check_pointer_msg()
	+ Fixed typo and formatting [Andy]

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 (9):
  vsprintf: Shuffle restricted_pointer()
  vsprintf: Consistent %pK handling for kptr_restrict == 0
  vsprintf: Do not check address of well-known strings
  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: Consolidate handling of unknown pointer specifiers
  vsprintf: Prevent crash when dereferencing invalid pointers
  vsprintf: Avoid confusion between invalid address and value

 Documentation/core-api/printk-formats.rst |   8 +
 lib/test_printf.c                         |  62 ++---
 lib/vsprintf.c                            | 409 ++++++++++++++++++------------
 3 files changed, 286 insertions(+), 193 deletions(-)

-- 
2.13.7

[PATCH v6 1/9] vsprintf: Shuffle restricted_pointer()

[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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c | 98 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 49 insertions(+), 49 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3add92329bae..e164d7b734f3 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -716,6 +716,55 @@ static char *ptr_to_id(char *buf, char *end, const void *ptr,
 	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)
@@ -1475,55 +1524,6 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 	return string(buf, end, uuid, 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,
 		  struct printf_spec spec,  const char *fmt)
-- 
2.13.7

[PATCH v6 2/9] 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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index e164d7b734f3..76ce12b278c3 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -724,8 +724,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;
 
@@ -2041,8 +2041,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, spec, fmt);
-- 
2.13.7

[PATCH v6 3/9] 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 | 81 +++++++++++++++++++++++++++++++---------------------------
 1 file changed, 44 insertions(+), 37 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 76ce12b278c3..0b6209854f5c 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -592,15 +592,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 *string_nocheck(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)
@@ -614,6 +612,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 string_nocheck(buf, end, s, spec);
+}
+
 char *pointer_string(char *buf, char *end, const void *ptr,
 		     struct printf_spec spec)
 {
@@ -700,7 +707,7 @@ static char *ptr_to_id(char *buf, char *end, const void *ptr,
 	if (static_branch_unlikely(&not_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 string_nocheck(buf, end, str, spec);
 	}
 
 #ifdef CONFIG_64BIT
@@ -736,7 +743,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 string_nocheck(buf, end, "pK-error", spec);
 		}
 
 		/*
@@ -850,7 +857,7 @@ char *symbol_string(char *buf, char *end, void *ptr,
 	else
 		sprint_symbol_no_offset(sym, value);
 
-	return string(buf, end, sym, spec);
+	return string_nocheck(buf, end, sym, spec);
 #else
 	return special_hex_number(buf, end, value, sizeof(void *));
 #endif
@@ -936,27 +943,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 = string_nocheck(p, pend, "io  ", str_spec);
 		specp = &io_spec;
 	} else if (res->flags & IORESOURCE_MEM) {
-		p = string(p, pend, "mem ", str_spec);
+		p = string_nocheck(p, pend, "mem ", str_spec);
 		specp = &mem_spec;
 	} else if (res->flags & IORESOURCE_IRQ) {
-		p = string(p, pend, "irq ", str_spec);
+		p = string_nocheck(p, pend, "irq ", str_spec);
 		specp = &default_dec_spec;
 	} else if (res->flags & IORESOURCE_DMA) {
-		p = string(p, pend, "dma ", str_spec);
+		p = string_nocheck(p, pend, "dma ", str_spec);
 		specp = &default_dec_spec;
 	} else if (res->flags & IORESOURCE_BUS) {
-		p = string(p, pend, "bus ", str_spec);
+		p = string_nocheck(p, pend, "bus ", str_spec);
 		specp = &bus_spec;
 	} else {
-		p = string(p, pend, "??? ", str_spec);
+		p = string_nocheck(p, pend, "??? ", str_spec);
 		specp = &mem_spec;
 		decode = 0;
 	}
 	if (decode && res->flags & IORESOURCE_UNSET) {
-		p = string(p, pend, "size ", str_spec);
+		p = string_nocheck(p, pend, "size ", str_spec);
 		p = number(p, pend, resource_size(res), *specp);
 	} else {
 		p = number(p, pend, res->start, *specp);
@@ -967,21 +974,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 = string_nocheck(p, pend, " 64bit", str_spec);
 		if (res->flags & IORESOURCE_PREFETCH)
-			p = string(p, pend, " pref", str_spec);
+			p = string_nocheck(p, pend, " pref", str_spec);
 		if (res->flags & IORESOURCE_WINDOW)
-			p = string(p, pend, " window", str_spec);
+			p = string_nocheck(p, pend, " window", str_spec);
 		if (res->flags & IORESOURCE_DISABLED)
-			p = string(p, pend, " disabled", str_spec);
+			p = string_nocheck(p, pend, " disabled", str_spec);
 	} else {
-		p = string(p, pend, " flags ", str_spec);
+		p = string_nocheck(p, pend, " flags ", str_spec);
 		p = number(p, pend, res->flags, default_flag_spec);
 	}
 	*p++ = ']';
 	*p = '\0';
 
-	return string(buf, end, sym, spec);
+	return string_nocheck(buf, end, sym, spec);
 }
 
 static noinline_for_stack
@@ -1149,7 +1156,7 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
 	}
 	*p = '\0';
 
-	return string(buf, end, mac_addr, spec);
+	return string_nocheck(buf, end, mac_addr, spec);
 }
 
 static noinline_for_stack
@@ -1312,7 +1319,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 string_nocheck(buf, end, ip6_addr, spec);
 }
 
 static noinline_for_stack
@@ -1323,7 +1330,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 string_nocheck(buf, end, ip4_addr, spec);
 }
 
 static noinline_for_stack
@@ -1385,7 +1392,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 string_nocheck(buf, end, ip6_addr, spec);
 }
 
 static noinline_for_stack
@@ -1420,7 +1427,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 string_nocheck(buf, end, ip4_addr, spec);
 }
 
 static noinline_for_stack
@@ -1521,7 +1528,7 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 
 	*p = 0;
 
-	return string(buf, end, uuid, spec);
+	return string_nocheck(buf, end, uuid, spec);
 }
 
 static noinline_for_stack
@@ -1735,13 +1742,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 string_nocheck(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 = string_nocheck(buf, end, "/", default_str_spec);
 		buf = string(buf, end, device_node_name_for_depth(np, depth),
 			     default_str_spec);
 	}
@@ -1769,10 +1776,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 string_nocheck(buf, end, "(!OF)", spec);
 
 	if ((unsigned long)dn < PAGE_SIZE)
-		return string(buf, end, "(null)", spec);
+		return string_nocheck(buf, end, "(null)", spec);
 
 	/* simple case without anything any more format specifiers */
 	fmt++;
@@ -1813,7 +1820,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 = string_nocheck(buf, end, tbuf, str_spec);
 			break;
 		case 'c':	/* major compatible string */
 			ret = of_property_read_string(dn, "compatible", &p);
@@ -1824,10 +1831,10 @@ 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 = string_nocheck(buf, end, ",", str_spec);
+				buf = string_nocheck(buf, end, "\"", str_spec);
 				buf = string(buf, end, p, str_spec);
-				buf = string(buf, end, "\"", str_spec);
+				buf = string_nocheck(buf, end, "\"", str_spec);
 
 				has_mult = true;
 			}
@@ -1966,7 +1973,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 string_nocheck(buf, end, "(null)", spec);
 	}
 
 	switch (*fmt) {
@@ -2022,7 +2029,7 @@ 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 string_nocheck(buf, end, "(invalid address)", spec);
 			}}
 		}
 		break;
-- 
2.13.7

Re: [PATCH v6 3/9] vsprintf: Do not check address of well-known strings

[Andy Shevchenko]

On Fri, Feb 08, 2019 at 04:23:04PM +0100, 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.
> 

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

> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  lib/vsprintf.c | 81 +++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 44 insertions(+), 37 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 76ce12b278c3..0b6209854f5c 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -592,15 +592,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 *string_nocheck(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)
> @@ -614,6 +612,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 string_nocheck(buf, end, s, spec);
> +}
> +
>  char *pointer_string(char *buf, char *end, const void *ptr,
>  		     struct printf_spec spec)
>  {
> @@ -700,7 +707,7 @@ static char *ptr_to_id(char *buf, char *end, const void *ptr,
>  	if (static_branch_unlikely(&not_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 string_nocheck(buf, end, str, spec);
>  	}
>  
>  #ifdef CONFIG_64BIT
> @@ -736,7 +743,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 string_nocheck(buf, end, "pK-error", spec);
>  		}
>  
>  		/*
> @@ -850,7 +857,7 @@ char *symbol_string(char *buf, char *end, void *ptr,
>  	else
>  		sprint_symbol_no_offset(sym, value);
>  
> -	return string(buf, end, sym, spec);
> +	return string_nocheck(buf, end, sym, spec);
>  #else
>  	return special_hex_number(buf, end, value, sizeof(void *));
>  #endif
> @@ -936,27 +943,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 = string_nocheck(p, pend, "io  ", str_spec);
>  		specp = &io_spec;
>  	} else if (res->flags & IORESOURCE_MEM) {
> -		p = string(p, pend, "mem ", str_spec);
> +		p = string_nocheck(p, pend, "mem ", str_spec);
>  		specp = &mem_spec;
>  	} else if (res->flags & IORESOURCE_IRQ) {
> -		p = string(p, pend, "irq ", str_spec);
> +		p = string_nocheck(p, pend, "irq ", str_spec);
>  		specp = &default_dec_spec;
>  	} else if (res->flags & IORESOURCE_DMA) {
> -		p = string(p, pend, "dma ", str_spec);
> +		p = string_nocheck(p, pend, "dma ", str_spec);
>  		specp = &default_dec_spec;
>  	} else if (res->flags & IORESOURCE_BUS) {
> -		p = string(p, pend, "bus ", str_spec);
> +		p = string_nocheck(p, pend, "bus ", str_spec);
>  		specp = &bus_spec;
>  	} else {
> -		p = string(p, pend, "??? ", str_spec);
> +		p = string_nocheck(p, pend, "??? ", str_spec);
>  		specp = &mem_spec;
>  		decode = 0;
>  	}
>  	if (decode && res->flags & IORESOURCE_UNSET) {
> -		p = string(p, pend, "size ", str_spec);
> +		p = string_nocheck(p, pend, "size ", str_spec);
>  		p = number(p, pend, resource_size(res), *specp);
>  	} else {
>  		p = number(p, pend, res->start, *specp);
> @@ -967,21 +974,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 = string_nocheck(p, pend, " 64bit", str_spec);
>  		if (res->flags & IORESOURCE_PREFETCH)
> -			p = string(p, pend, " pref", str_spec);
> +			p = string_nocheck(p, pend, " pref", str_spec);
>  		if (res->flags & IORESOURCE_WINDOW)
> -			p = string(p, pend, " window", str_spec);
> +			p = string_nocheck(p, pend, " window", str_spec);
>  		if (res->flags & IORESOURCE_DISABLED)
> -			p = string(p, pend, " disabled", str_spec);
> +			p = string_nocheck(p, pend, " disabled", str_spec);
>  	} else {
> -		p = string(p, pend, " flags ", str_spec);
> +		p = string_nocheck(p, pend, " flags ", str_spec);
>  		p = number(p, pend, res->flags, default_flag_spec);
>  	}
>  	*p++ = ']';
>  	*p = '\0';
>  
> -	return string(buf, end, sym, spec);
> +	return string_nocheck(buf, end, sym, spec);
>  }
>  
>  static noinline_for_stack
> @@ -1149,7 +1156,7 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
>  	}
>  	*p = '\0';
>  
> -	return string(buf, end, mac_addr, spec);
> +	return string_nocheck(buf, end, mac_addr, spec);
>  }
>  
>  static noinline_for_stack
> @@ -1312,7 +1319,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 string_nocheck(buf, end, ip6_addr, spec);
>  }
>  
>  static noinline_for_stack
> @@ -1323,7 +1330,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 string_nocheck(buf, end, ip4_addr, spec);
>  }
>  
>  static noinline_for_stack
> @@ -1385,7 +1392,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 string_nocheck(buf, end, ip6_addr, spec);
>  }
>  
>  static noinline_for_stack
> @@ -1420,7 +1427,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 string_nocheck(buf, end, ip4_addr, spec);
>  }
>  
>  static noinline_for_stack
> @@ -1521,7 +1528,7 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
>  
>  	*p = 0;
>  
> -	return string(buf, end, uuid, spec);
> +	return string_nocheck(buf, end, uuid, spec);
>  }
>  
>  static noinline_for_stack
> @@ -1735,13 +1742,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 string_nocheck(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 = string_nocheck(buf, end, "/", default_str_spec);
>  		buf = string(buf, end, device_node_name_for_depth(np, depth),
>  			     default_str_spec);
>  	}
> @@ -1769,10 +1776,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 string_nocheck(buf, end, "(!OF)", spec);
>  
>  	if ((unsigned long)dn < PAGE_SIZE)
> -		return string(buf, end, "(null)", spec);
> +		return string_nocheck(buf, end, "(null)", spec);
>  
>  	/* simple case without anything any more format specifiers */
>  	fmt++;
> @@ -1813,7 +1820,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 = string_nocheck(buf, end, tbuf, str_spec);
>  			break;
>  		case 'c':	/* major compatible string */
>  			ret = of_property_read_string(dn, "compatible", &p);
> @@ -1824,10 +1831,10 @@ 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 = string_nocheck(buf, end, ",", str_spec);
> +				buf = string_nocheck(buf, end, "\"", str_spec);
>  				buf = string(buf, end, p, str_spec);
> -				buf = string(buf, end, "\"", str_spec);
> +				buf = string_nocheck(buf, end, "\"", str_spec);
>  
>  				has_mult = true;
>  			}
> @@ -1966,7 +1973,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 string_nocheck(buf, end, "(null)", spec);
>  	}
>  
>  	switch (*fmt) {
> @@ -2022,7 +2029,7 @@ 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 string_nocheck(buf, end, "(invalid address)", spec);
>  			}}
>  		}
>  		break;
> -- 
> 2.13.7
> 

-- 
With Best Regards,
Andy Shevchenko

[PATCH v6 4/9] 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 improve error handling 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 | 52 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 0b6209854f5c..d8bc3bb8ce18 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1431,6 +1431,35 @@ 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)
+{
+	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 string_nocheck(buf, end, "(invalid address)", spec);
+		}}
+	}
+
+	return ptr_to_id(buf, end, ptr, spec);
+}
+
+static noinline_for_stack
 char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 		     const char *fmt)
 {
@@ -2011,28 +2040,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 string_nocheck(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.7

[PATCH v6 5/9] vsprintf: Factor out %pV handler as va_format()

[Petr Mladek]

Move the code from the long pointer() function. We are going to improve
error handling that will make it more complicated.

This patch does not change the existing behavior.

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

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d8bc3bb8ce18..9621b76f6c60 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1519,6 +1519,17 @@ 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)
@@ -2046,15 +2057,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.7

Re: [PATCH v6 5/9] vsprintf: Factor out %pV handler as va_format()

[Joe Perches]

On Fri, 2019-02-08 at 16:23 +0100, Petr Mladek wrote:
> Move the code from the long pointer() function. We are going to improve
> error handling that will make it more complicated.
> 
> This patch does not change the existing behavior.

But doesn't this increase stack use?
%pV is recursive and increasing the stack is undesired
for this use.

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> @@ -1519,6 +1519,17 @@ 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)
> @@ -2046,15 +2057,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':

Re: [PATCH v6 5/9] vsprintf: Factor out %pV handler as va_format()

[Petr Mladek]

On Fri 2019-02-08 09:11:17, Joe Perches wrote:
> On Fri, 2019-02-08 at 16:23 +0100, Petr Mladek wrote:
> > Move the code from the long pointer() function. We are going to improve
> > error handling that will make it more complicated.
> > 
> > This patch does not change the existing behavior.
> 
> But doesn't this increase stack use?
> %pV is recursive and increasing the stack is undesired
> for this use.

%pV handler is stack sensitive because the entire vsnprintf()
machinery is called recursively. This one extra call does
not make it much worse.

Best Regards,
Petr

Re: [PATCH v6 5/9] vsprintf: Factor out %pV handler as va_format()

[Steven Rostedt]

On Tue, 12 Feb 2019 14:00:30 +0100
Petr Mladek <pmladek@suse.com> wrote:

> On Fri 2019-02-08 09:11:17, Joe Perches wrote:
> > On Fri, 2019-02-08 at 16:23 +0100, Petr Mladek wrote:  
> > > Move the code from the long pointer() function. We are going to improve
> > > error handling that will make it more complicated.
> > > 
> > > This patch does not change the existing behavior.  
> > 
> > But doesn't this increase stack use?
> > %pV is recursive and increasing the stack is undesired
> > for this use.  
> 
> %pV handler is stack sensitive because the entire vsnprintf()
> machinery is called recursively. This one extra call does
> not make it much worse.
>

Not to mention that if it is the only static call, gcc will most likely
just keep it inlined. Could add an inline annotation just to be
paranoid.

-- Steve

Re: [PATCH v6 5/9] vsprintf: Factor out %pV handler as va_format()

[Joe Perches]

On Tue, 2019-02-12 at 14:00 +0100, Petr Mladek wrote:
> On Fri 2019-02-08 09:11:17, Joe Perches wrote:
> > On Fri, 2019-02-08 at 16:23 +0100, Petr Mladek wrote:
> > > Move the code from the long pointer() function. We are going to improve
> > > error handling that will make it more complicated.
> > > 
> > > This patch does not change the existing behavior.
> > 
> > But doesn't this increase stack use?
> > %pV is recursive and increasing the stack is undesired
> > for this use.
> 
> %pV handler is stack sensitive because the entire vsnprintf()
> machinery is called recursively. This one extra call does
> not make it much worse.

That's an argument?.

Refactoring is good, but you need to add
__always_inline here.

Re: [PATCH v6 5/9] vsprintf: Factor out %pV handler as va_format()

[Steven Rostedt]

On Tue, 12 Feb 2019 09:58:25 -0800
Joe Perches <joe@perches.com> wrote:

> On Tue, 2019-02-12 at 14:00 +0100, Petr Mladek wrote:
> > On Fri 2019-02-08 09:11:17, Joe Perches wrote:  
> > > On Fri, 2019-02-08 at 16:23 +0100, Petr Mladek wrote:  
> > > > Move the code from the long pointer() function. We are going to improve
> > > > error handling that will make it more complicated.
> > > > 
> > > > This patch does not change the existing behavior.  
> > > 
> > > But doesn't this increase stack use?
> > > %pV is recursive and increasing the stack is undesired
> > > for this use.  
> > 
> > %pV handler is stack sensitive because the entire vsnprintf()
> > machinery is called recursively. This one extra call does
> > not make it much worse.  
> 
> That's an argument?.
> 
> Refactoring is good, but you need to add
> __always_inline here.
> 

If a single function call causes this to overflow the stack, then the
code is already broken to begin with.

-- Steve

Re: [PATCH v6 5/9] vsprintf: Factor out %pV handler as va_format()

[Rasmus Villemoes]

On 12/02/2019 18.58, Joe Perches wrote:
> On Tue, 2019-02-12 at 14:00 +0100, Petr Mladek wrote:
>> On Fri 2019-02-08 09:11:17, Joe Perches wrote:
>>> On Fri, 2019-02-08 at 16:23 +0100, Petr Mladek wrote:
>>>> Move the code from the long pointer() function. We are going to improve
>>>> error handling that will make it more complicated.
>>>>
>>>> This patch does not change the existing behavior.
>>>
>>> But doesn't this increase stack use?
>>> %pV is recursive and increasing the stack is undesired
>>> for this use.
>>
>> %pV handler is stack sensitive because the entire vsnprintf()
>> machinery is called recursively. This one extra call does
>> not make it much worse.
> 
> That's an argument?.
> 
> Refactoring is good, but you need to add
> __always_inline here.
> 
> 

No. No no no. Please at least try to send the code through a compiler.
Petr's patch actually makes things uniformly better, since gcc ends up
doing a tail call from pointer().

    2e65:       4c 89 cf                mov    %r9,%rdi
    2e68:       e9 23 05 00 00          jmpq   3390 <va_format>

$ scripts/stackusage -o before.su lib/vsprintf.o
$ cp lib/vsprintf.o lib/vsprintf.o.0
$ # apply patch
$ scripts/stackusage -o after.su lib/vsprintf.o
$ cp lib/vsprintf.o lib/vsprintf.o.1
$ scripts/stackdelta before.su after.su
./lib/vsprintf.c        pointer 40      8       -32
$ grep va_format after.su
after.su:./lib/vsprintf.c:1437     va_format       40      static
# Icing
$ scripts/bloat-o-meter lib/vsprintf.o.0 lib/vsprintf.o.1
add/remove: 1/0 grow/shrink: 0/1 up/down: 77/-186 (-109)
Function                                     old     new   delta
va_format                                      -      77     +77
pointer                                      634     448    -186

Of course, all of this depends very much on compiler version,
architecture (if the calling convention says to pass arguments on stack,
tail calls are a lot harder to come by), CONFIG_STACKPROTECTOR etc.
etc., but nothing suggests that forcing the va_format logic into
pointer() would be a good thing.

Rasmus

[PATCH v6 6/9] vsprintf: Factor out %pO handler as kobject_string()

[Petr Mladek]

Move code from the long pointer() function. We are going to improve
error handling that will make it even more complicated.

This patch does not change the existing behavior.

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 | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 9621b76f6c60..ab59db1c1146 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1887,6 +1887,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 ptr_to_id(buf, end, ptr, 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
@@ -2082,11 +2093,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'G':
 		return flags_string(buf, end, ptr, fmt);
 	case 'O':
-		switch (fmt[1]) {
-		case 'F':
-			return device_node_string(buf, end, ptr, spec, fmt + 1);
-		}
-		break;
+		return kobject_string(buf, end, ptr, spec, fmt);
 	case 'x':
 		return pointer_string(buf, end, ptr, spec);
 	}
-- 
2.13.7

[PATCH v6 7/9] 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 inconsistent and not helpful.
Using WARN() looks like an overkill for this type of error. pr_warn()
is not good either. It would by handled via printk_safe 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.

Note that the changes in test_printf.c revert many changes made
by the commit 4d42c44727a062e233 ("lib/vsprintf: Print time and
date in human readable format via %pt\n"). The format %pt does
not longer produce a hashed pointer.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 lib/test_printf.c | 39 ++++-----------------------------------
 lib/vsprintf.c    | 28 +++++++++++++++++-----------
 2 files changed, 21 insertions(+), 46 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 659b6cc0d483..159bb78b3992 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -250,11 +250,12 @@ plain_format(void)
 #endif	/* BITS_PER_LONG == 64 */
 
 static int __init
-plain_hash_to_buffer(const void *p, char *buf, size_t len)
+plain_hash(void)
 {
+	char buf[PLAIN_BUF_SIZE];
 	int nchars;
 
-	nchars = snprintf(buf, len, "%p", p);
+	nchars = snprintf(buf, sizeof(buf), "%p", PTR);
 
 	if (nchars != PTR_WIDTH)
 		return -1;
@@ -265,20 +266,6 @@ plain_hash_to_buffer(const void *p, char *buf, size_t len)
 		return 0;
 	}
 
-	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;
 
@@ -309,23 +296,6 @@ plain(void)
 }
 
 static void __init
-test_hashed(const char *fmt, const void *p)
-{
-	char buf[PLAIN_BUF_SIZE];
-	int ret;
-
-	/*
-	 * No need to increase failed test counter since this is assumed
-	 * to be called after plain().
-	 */
-	ret = plain_hash_to_buffer(p, buf, PLAIN_BUF_SIZE);
-	if (ret)
-		return;
-
-	test(buf, fmt, p);
-}
-
-static void __init
 symbol_ptr(void)
 {
 }
@@ -462,8 +432,7 @@ struct_rtc_time(void)
 		.tm_year = 118,
 	};
 
-	test_hashed("%pt", &tm);
-
+	test("(%ptR?)", "%pt", &tm);
 	test("2018-11-26T05:35:43", "%ptR", &tm);
 	test("0118-10-26T05:35:43", "%ptRr", &tm);
 	test("05:35:43|2018-11-26", "%ptRt|%ptRd", &tm, &tm);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index ab59db1c1146..d8509f6adda6 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1434,6 +1434,8 @@ 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);
@@ -1456,7 +1458,8 @@ char *ip_addr_string(char *buf, char *end, const void *ptr,
 		}}
 	}
 
-	return ptr_to_id(buf, end, ptr, spec);
+	err_fmt_msg = fmt[0] == 'i' ? "(%pi?)" : "(%pI?)";
+	return string_nocheck(buf, end, err_fmt_msg, spec);
 }
 
 static noinline_for_stack
@@ -1584,7 +1587,7 @@ char *netdev_bits(char *buf, char *end, const void *addr,
 		size = sizeof(netdev_features_t);
 		break;
 	default:
-		return ptr_to_id(buf, end, addr, spec);
+		return string_nocheck(buf, end, "(%pN?)", spec);
 	}
 
 	return special_hex_number(buf, end, num, size);
@@ -1688,7 +1691,7 @@ char *time_and_date(char *buf, char *end, void *ptr, struct printf_spec spec,
 	case 'R':
 		return rtc_str(buf, end, (const struct rtc_time *)ptr, fmt);
 	default:
-		return ptr_to_id(buf, end, ptr, spec);
+		return string_nocheck(buf, end, "(%ptR?)", spec);
 	}
 }
 
@@ -1696,7 +1699,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 string_nocheck(buf, end, "(%pC?)", spec);
+
+	if (!clk)
 		return string(buf, end, NULL, spec);
 
 	switch (fmt[1]) {
@@ -1705,7 +1711,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 ptr_to_id(buf, end, clk, spec);
+		return string_nocheck(buf, end, "(%pC?)", spec);
 #endif
 	}
 }
@@ -1738,7 +1744,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;
@@ -1759,8 +1766,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 string_nocheck(buf, end, "(%pG?)", spec);
 	}
 
 	return format_flags(buf, end, flags, names);
@@ -1816,7 +1822,7 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	str_spec.field_width = -1;
 
 	if (!IS_ENABLED(CONFIG_OF))
-		return string_nocheck(buf, end, "(!OF)", spec);
+		return string_nocheck(buf, end, "(%pOF?)", spec);
 
 	if ((unsigned long)dn < PAGE_SIZE)
 		return string_nocheck(buf, end, "(null)", spec);
@@ -1895,7 +1901,7 @@ static char *kobject_string(char *buf, char *end, void *ptr,
 		return device_node_string(buf, end, ptr, spec, fmt + 1);
 	}
 
-	return ptr_to_id(buf, end, ptr, spec);
+	return string_nocheck(buf, end, "(%pO?)", spec);
 }
 
 /*
@@ -2091,7 +2097,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':
 		return kobject_string(buf, end, ptr, spec, fmt);
 	case 'x':
-- 
2.13.7

Re: [PATCH v6 7/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Andy Shevchenko]

On Fri, Feb 08, 2019 at 04:23:08PM +0100, 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 inconsistent and not helpful.
> Using WARN() looks like an overkill for this type of error. pr_warn()
> is not good either. It would by handled via printk_safe 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.
> 

> Note that the changes in test_printf.c revert many changes made
> by the commit 4d42c44727a062e233 ("lib/vsprintf: Print time and
> date in human readable format via %pt\n"). The format %pt does
> not longer produce a hashed pointer.

It seems in further patch you partially restore back what was brought by that patch.
I would suggest not to touch at least changes for plain_pointer_to_buffer().

Also it would be nice to have a coverage for hashed pointers.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 7/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

On Fri 2019-02-08 19:25:18, Andy Shevchenko wrote:
> On Fri, Feb 08, 2019 at 04:23:08PM +0100, 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 inconsistent and not helpful.
> > Using WARN() looks like an overkill for this type of error. pr_warn()
> > is not good either. It would by handled via printk_safe 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.
> > 
> 
> > Note that the changes in test_printf.c revert many changes made
> > by the commit 4d42c44727a062e233 ("lib/vsprintf: Print time and
> > date in human readable format via %pt\n"). The format %pt does
> > not longer produce a hashed pointer.
> 
> It seems in further patch you partially restore back what was brought by that patch.
> I would suggest not to touch at least changes for plain_pointer_to_buffer().

It is a matter of taste. plain_pointer_to_buffer() is not longer
really needed and makes the code more complicated from my POV.
On the other hand, the removal makes the patch more complicated
and harder to review.


> Also it would be nice to have a coverage for hashed pointers.

The hashing is still tested by plain_hash() and plain_format().

The only remaining format using ptr_to_id() is %pK. But this one
would be tricky to test.

Anyway, I am going to keep plain_pointer_to_buffer() in v7
to make the patch simple. We could clean this later.

Best Regards,
Petr

[PATCH v6 8/9] 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                         |  39 +++++++--
 lib/vsprintf.c                            | 132 ++++++++++++++++++++++--------
 3 files changed, 136 insertions(+), 42 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index a7fae4538946..732943b36828 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 159bb78b3992..2a701e2e5d24 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -211,12 +211,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)
 		return -1;
@@ -239,9 +239,10 @@ plain_format(void)
 #define PTR ((void *)0x456789ab)
 #define PTR_STR "456789ab"
 #define PTR_VAL_NO_CRNG "(ptrval)"
+#define ZEROS ""
 
 static int __init
-plain_format(void)
+plain_format(void *ptr)
 {
 	/* Format is implicitly tested for 32 bit machines by plain_hash() */
 	return 0;
@@ -250,12 +251,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, sizeof(buf), "%p", PTR);
+	nchars = snprintf(buf, sizeof(buf), "%p", ptr);
 
 	if (nchars != PTR_WIDTH)
 		return -1;
@@ -277,18 +278,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++;
@@ -296,6 +297,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)
 {
 }
@@ -540,7 +559,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 d8509f6adda6..3a95b4d1ca2e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -611,12 +611,45 @@ static char *string_nocheck(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_msg(const void *ptr)
+{
+	char byte;
+
+	if (!ptr)
+		return "(null)";
+
+	if (probe_kernel_address(ptr, byte))
+		return "(efault)";
+
+	return NULL;
+}
+
+static int check_pointer(char **buf, char *end, const void *ptr,
+			     struct printf_spec spec)
+{
+	const char *err_msg;
+
+	err_msg = check_pointer_msg(ptr);
+	if (err_msg) {
+		*buf = string_nocheck(*buf, end, err_msg, spec);
+		return -EFAULT;
+	}
+
+	return 0;
+}
+
 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 (check_pointer(&buf, end, s, spec))
+		return buf;
 
 	return string_nocheck(buf, end, s, spec);
 }
@@ -791,6 +824,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 (check_pointer(&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) {
@@ -821,8 +859,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 (check_pointer(&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])) {
@@ -941,6 +983,9 @@ char *resource_string(char *buf, char *end, struct resource *res,
 	int decode = (fmt[0] == 'R') ? 1 : 0;
 	const struct printf_spec *specp;
 
+	if (check_pointer(&buf, end, res, spec))
+		return buf;
+
 	*p++ = '[';
 	if (res->flags & IORESOURCE_IO) {
 		p = string_nocheck(p, pend, "io  ", str_spec);
@@ -1003,9 +1048,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 (check_pointer(&buf, end, addr, spec))
+		return buf;
 
 	switch (fmt[1]) {
 	case 'C':
@@ -1052,6 +1096,9 @@ char *bitmap_string(char *buf, char *end, unsigned long *bitmap,
 	int i, chunksz;
 	bool first = true;
 
+	if (check_pointer(&buf, end, bitmap, spec))
+		return buf;
+
 	/* reused to print numbers */
 	spec = (struct printf_spec){ .flags = SMALL | ZEROPAD, .base = 16 };
 
@@ -1093,6 +1140,9 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap,
 	int cur, rbot, rtop;
 	bool first = true;
 
+	if (check_pointer(&buf, end, bitmap, spec))
+		return buf;
+
 	rbot = cur = find_first_bit(bitmap, nr_bits);
 	while (cur < nr_bits) {
 		rtop = cur;
@@ -1131,6 +1181,9 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
 	char separator;
 	bool reversed = false;
 
+	if (check_pointer(&buf, end, addr, spec))
+		return buf;
+
 	switch (fmt[1]) {
 	case 'F':
 		separator = '-';
@@ -1436,6 +1489,9 @@ char *ip_addr_string(char *buf, char *end, const void *ptr,
 {
 	char *err_fmt_msg;
 
+	if (check_pointer(&buf, end, ptr, spec))
+		return buf;
+
 	switch (fmt[1]) {
 	case '6':
 		return ip6_addr_string(buf, end, ptr, spec, fmt);
@@ -1474,9 +1530,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 (check_pointer(&buf, end, addr, spec))
+		return buf;
 
 	do {
 		switch (fmt[count++]) {
@@ -1522,10 +1577,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 (check_pointer(&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);
 	va_end(va);
@@ -1543,6 +1602,9 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 	const u8 *index = uuid_index;
 	bool uc = false;
 
+	if (check_pointer(&buf, end, addr, spec))
+		return buf;
+
 	switch (*(++fmt)) {
 	case 'L':
 		uc = true;		/* fall-through */
@@ -1581,6 +1643,9 @@ char *netdev_bits(char *buf, char *end, const void *addr,
 	unsigned long long num;
 	int size;
 
+	if (check_pointer(&buf, end, addr, spec))
+		return buf;
+
 	switch (fmt[1]) {
 	case 'F':
 		num = *(const netdev_features_t *)addr;
@@ -1594,11 +1659,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 (check_pointer(&buf, end, addr, spec))
+		return buf;
+
 	switch (fmt[1]) {
 	case 'd':
 		num = *(const dma_addr_t *)addr;
@@ -1650,12 +1719,16 @@ char *time_str(char *buf, char *end, const struct rtc_time *tm, bool r)
 }
 
 static noinline_for_stack
-char *rtc_str(char *buf, char *end, const struct rtc_time *tm, const char *fmt)
+char *rtc_str(char *buf, char *end, const struct rtc_time *tm,
+	      struct printf_spec spec, const char *fmt)
 {
 	bool have_t = true, have_d = true;
 	bool raw = false;
 	int count = 2;
 
+	if (check_pointer(&buf, end, tm, spec))
+		return buf;
+
 	switch (fmt[count]) {
 	case 'd':
 		have_t = false;
@@ -1689,7 +1762,7 @@ char *time_and_date(char *buf, char *end, void *ptr, struct printf_spec spec,
 {
 	switch (fmt[1]) {
 	case 'R':
-		return rtc_str(buf, end, (const struct rtc_time *)ptr, fmt);
+		return rtc_str(buf, end, (const struct rtc_time *)ptr, spec, fmt);
 	default:
 		return string_nocheck(buf, end, "(%ptR?)", spec);
 	}
@@ -1702,8 +1775,8 @@ char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
 	if (!IS_ENABLED(CONFIG_HAVE_CLK))
 		return string_nocheck(buf, end, "(%pC?)", spec);
 
-	if (!clk)
-		return string(buf, end, NULL, spec);
+	if (check_pointer(&buf, end, clk, spec))
+		return buf;
 
 	switch (fmt[1]) {
 	case 'n':
@@ -1750,6 +1823,9 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
 	unsigned long flags;
 	const struct trace_print_flags *names;
 
+	if (check_pointer(&buf, end, flags_ptr, spec))
+		return buf;
+
 	switch (fmt[1]) {
 	case 'p':
 		flags = *(unsigned long *)flags_ptr;
@@ -2021,18 +2097,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 string_nocheck(buf, end, "(null)", spec);
-	}
-
 	switch (*fmt) {
 	case 'F':
 	case 'f':
@@ -2074,13 +2138,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 't':
@@ -2714,11 +2778,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_msg(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.7

Re: [PATCH v6 8/9] vsprintf: Prevent crash when dereferencing invalid pointers

[Sergey Senozhatsky]

On (02/08/19 16:23), 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
> +  * the address to make sure it doesn't fault due to a poorly added printk
> +  * during debugging.
> +  */
> +static const char *check_pointer_msg(const void *ptr)
> +{
> +	char byte;
> +
> +	if (!ptr)
> +		return "(null)";
> +
> +	if (probe_kernel_address(ptr, byte))
> +		return "(efault)";
> +
> +	return NULL;
> +}

Hmm... So the assumption here is that the target buffer always has
at least strlen("(efault)") bytes and, thus, we always can write the
error message to it.

> +static int check_pointer(char **buf, char *end, const void *ptr,
> +			     struct printf_spec spec)
> +{
> +	const char *err_msg;
> +
> +	err_msg = check_pointer_msg(ptr);
> +	if (err_msg) {
> +		*buf = string_nocheck(*buf, end, err_msg, spec);
> +		return -EFAULT;
> +	}
> +
> +	return 0;
> +}

Suppose in my driver I sprintf() pointers to 4-bytes strings and, thus,
have only 5 spare bytes in target buffer. But one of the pointers is
faulty and now sprintf() writes "(efault)" to target buffer which can
hold only 5 bytes.

	-ss

Re: [PATCH v6 8/9] vsprintf: Prevent crash when dereferencing invalid pointers

[Andy Shevchenko]

On Tue, Feb 19, 2019 at 5:35 AM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
> On (02/08/19 16:23), Petr Mladek wrote:

> Hmm... So the assumption here is that the target buffer always has
> at least strlen("(efault)") bytes and, thus, we always can write the
> error message to it.

Same assumption as for pointers, 8 or 16 bytes. Same will happen if
it's not enough room.

> > +static int check_pointer(char **buf, char *end, const void *ptr,
> > +                          struct printf_spec spec)
> > +{
> > +     const char *err_msg;
> > +
> > +     err_msg = check_pointer_msg(ptr);
> > +     if (err_msg) {
> > +             *buf = string_nocheck(*buf, end, err_msg, spec);
> > +             return -EFAULT;
> > +     }
> > +
> > +     return 0;
> > +}
>
> Suppose in my driver I sprintf() pointers to 4-bytes strings and, thus,
> have only 5 spare bytes in target buffer. But one of the pointers is
> faulty and now sprintf() writes "(efault)" to target buffer which can
> hold only 5 bytes.

And if it's not? You will get in either case incomplete information,
but at least with "(e" (or even "(") you might get a clue that it
errornous conditions.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 8/9] vsprintf: Prevent crash when dereferencing invalid pointers

[Sergey Senozhatsky]

On (02/19/19 13:02), Andy Shevchenko wrote:
[..]
> And if it's not? You will get in either case incomplete information,
> but at least with "(e" (or even "(") you might get a clue that it
> errornous conditions.

The thing I'm signaling here is that in some cases we still can
crash the kernel; with the difference that invalid dereference
can now be a memory corruption. Just saying.

	-ss

Re: [PATCH v6 8/9] vsprintf: Prevent crash when dereferencing invalid pointers

[Andy Shevchenko]

On Tue, Feb 19, 2019 at 09:51:15PM +0900, Sergey Senozhatsky wrote:
> On (02/19/19 13:02), Andy Shevchenko wrote:
> [..]
> > And if it's not? You will get in either case incomplete information,
> > but at least with "(e" (or even "(") you might get a clue that it
> > errornous conditions.
> 
> The thing I'm signaling here is that in some cases we still can
> crash the kernel; with the difference that invalid dereference
> can now be a memory corruption. Just saying.

Wouldn't that mean that the culprit in the caller, not in the callee?

(As far as I got your another example with badly called sprintf() which may
 overwrite stack, etc).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 8/9] vsprintf: Prevent crash when dereferencing invalid pointers

[Sergey Senozhatsky]

On (02/19/19 15:49), Andy Shevchenko wrote:
> > On (02/19/19 13:02), Andy Shevchenko wrote:
> > [..]
> > > And if it's not? You will get in either case incomplete information,
> > > but at least with "(e" (or even "(") you might get a clue that it
> > > errornous conditions.
> > 
> > The thing I'm signaling here is that in some cases we still can
> > crash the kernel; with the difference that invalid dereference
> > can now be a memory corruption. Just saying.
> 
> Wouldn't that mean that the culprit in the caller, not in the callee?
> 
> (As far as I got your another example with badly called sprintf() which may
>  overwrite stack, etc).

ipv4 printout case does not look like a caller bug to me: we expect a 15
bytes ipv4 address, allocate a 16 bytes buffer, sprintf() voluntarily
writes 18 bytes. This error reporting is a bit of a dangerous practice;
next year someone might add another specifier and another

	return string(buf, end, "(this data does not look right)", spec);

We probably would want to do something about it. For instance, mandating
that "(error)" string cannot be larger than 8 bytes can be a good starting
point.

	-ss

Re: [PATCH v6 8/9] vsprintf: Prevent crash when dereferencing invalid pointers

[Petr Mladek]

On Tue 2019-02-19 23:15:16, Sergey Senozhatsky wrote:
> On (02/19/19 15:49), Andy Shevchenko wrote:
> > > On (02/19/19 13:02), Andy Shevchenko wrote:
> > > [..]
> > > > And if it's not? You will get in either case incomplete information,
> > > > but at least with "(e" (or even "(") you might get a clue that it
> > > > errornous conditions.
> > > 
> > > The thing I'm signaling here is that in some cases we still can
> > > crash the kernel; with the difference that invalid dereference
> > > can now be a memory corruption. Just saying.
> > 
> > Wouldn't that mean that the culprit in the caller, not in the callee?
> > 
> > (As far as I got your another example with badly called sprintf() which may
> >  overwrite stack, etc).
> 
> ipv4 printout case does not look like a caller bug to me: we expect a 15
> bytes ipv4 address, allocate a 16 bytes buffer, sprintf() voluntarily
> writes 18 bytes. This error reporting is a bit of a dangerous practice;
> next year someone might add another specifier and another
> 
> 	return string(buf, end, "(this data does not look right)", spec);

I hope that this would never pass a review.

> We probably would want to do something about it. For instance, mandating
> that "(error)" string cannot be larger than 8 bytes can be a good starting
> point.

All the warnings are printed by string_nocheck(). We could add one
more variant for printing the warnings, for example:

static int error_string(char **buf, char *end, const char *err_msg,
			     struct printf_spec spec)
{
	/*
	 * Limit the error message when the buffer size is unknown.
	 * It is not full proof but it looks like a reasonable
	 * compromise. The (null) error string never caused a problem.
	 */
	if ((end - *buf > INT_MAX / 2) && !spec.precision) {
		if (spec.field_width)
			spec.precision = spec.field_width;
		else
			spec.precision = 8;
	}

	*buf = string_nocheck(*buf, end, err_msg, spec);
		return -EFAULT;
	}

	return 0;
}

Well, I am not sure that it is worth it. I guess that "(null)"
never caused a problem. And all the new error messages have
8 characters at maximum.

Best Regards,
Petr

[PATCH v6 9/9] 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                            | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 732943b36828..0cbd98206a20 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
 
 Plain Pointers
 --------------
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3a95b4d1ca2e..e51cbc2be540 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1510,7 +1510,7 @@ char *ip_addr_string(char *buf, char *end, const void *ptr,
 		case AF_INET6:
 			return ip6_addr_string_sa(buf, end, &sa->v6, spec, fmt);
 		default:
-			return string_nocheck(buf, end, "(invalid address)", spec);
+			return string_nocheck(buf, end, "(einval)", spec);
 		}}
 	}
 
-- 
2.13.7

Re: [PATCH v6 9/9] vsprintf: Avoid confusion between invalid address and value

[Andy Shevchenko]

On Fri, Feb 08, 2019 at 04:23:10PM +0100, Petr Mladek wrote:
> 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)".

The proper one should be "invalid address family". The proposed change
increases confusion.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 9/9] vsprintf: Avoid confusion between invalid address and value

[Petr Mladek]

On Fri 2019-02-08 19:27:17, Andy Shevchenko wrote:
> On Fri, Feb 08, 2019 at 04:23:10PM +0100, Petr Mladek wrote:
> > 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)".
> 
> The proper one should be "invalid address family". The proposed change
> increases confusion.

I am confused. Is there any error code for "invalid address family"?

EINVAL is standard error code used when a wrong value is passed
as a parameter. In this case, the code is not able to handle
the given address family.

IMHO, the original message "invalid address" has been even more
confusing. Oops would happen if it was invalid. In fact, the value
was invalid.

Best Regards,
Petr

Re: [PATCH v6 9/9] vsprintf: Avoid confusion between invalid address and value

[Andy Shevchenko]

On Tue, Feb 12, 2019 at 04:45:30PM +0100, Petr Mladek wrote:
> On Fri 2019-02-08 19:27:17, Andy Shevchenko wrote:
> > On Fri, Feb 08, 2019 at 04:23:10PM +0100, Petr Mladek wrote:
> > > 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)".
> > 
> > The proper one should be "invalid address family". The proposed change
> > increases confusion.
> 
> I am confused. Is there any error code for "invalid address family"?

I'm not sure.
There is EAFNOSUPPORT. I don't know if it suits better.

> EINVAL is standard error code used when a wrong value is passed
> as a parameter. In this case, the code is not able to handle
> the given address family.

This is possible, but it will produce more generic message.


> IMHO, the original message "invalid address" has been even more
> confusing. Oops would happen if it was invalid. In fact, the value
> was invalid.

I agree with this. "Address" may be treated as "memory address", while in
practice it's a "network address".

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 9/9] vsprintf: Avoid confusion between invalid address and value

[Petr Mladek]

On Wed 2019-02-13 15:54:55, Andy Shevchenko wrote:
> On Tue, Feb 12, 2019 at 04:45:30PM +0100, Petr Mladek wrote:
> > On Fri 2019-02-08 19:27:17, Andy Shevchenko wrote:
> > > On Fri, Feb 08, 2019 at 04:23:10PM +0100, Petr Mladek wrote:
> > > > 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)".
> > > 
> > > The proper one should be "invalid address family". The proposed change
> > > increases confusion.
> > 
> > I am confused. Is there any error code for "invalid address family"?
> 
> I'm not sure.
> There is EAFNOSUPPORT. I don't know if it suits better.

I would not complicate it. EAFNOSUPPORT looks too special,
see below. Also it is controversial here because vsprintf()
does not implement any protocol.


> > EINVAL is standard error code used when a wrong value is passed
> > as a parameter. In this case, the code is not able to handle
> > the given address family.
> 
> This is possible, but it will produce more generic message.

I am not sure that I understand it. We do not pass the error code
anywhere. The patch only changes the string that is shown instead
of the requested value. It is a hint that something is wrong
either with the caller or with the vsprintf() implementation.

I think that it does not make sense to do a big deal from it.
"(einval)" looks informative enough to me.

Best Regards,
Petr

Re: [PATCH v6 9/9] vsprintf: Avoid confusion between invalid address and value

[Andy Shevchenko]

On Thu, Feb 14, 2019 at 09:42:56AM +0100, Petr Mladek wrote:
> On Wed 2019-02-13 15:54:55, Andy Shevchenko wrote:
> > On Tue, Feb 12, 2019 at 04:45:30PM +0100, Petr Mladek wrote:
> > > On Fri 2019-02-08 19:27:17, Andy Shevchenko wrote:
> > > > On Fri, Feb 08, 2019 at 04:23:10PM +0100, Petr Mladek wrote:
> > > > > 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)".
> > > > 
> > > > The proper one should be "invalid address family". The proposed change
> > > > increases confusion.
> > > 
> > > I am confused. Is there any error code for "invalid address family"?
> > 
> > I'm not sure.
> > There is EAFNOSUPPORT. I don't know if it suits better.
> 
> I would not complicate it. EAFNOSUPPORT looks too special,
> see below. Also it is controversial here because vsprintf()
> does not implement any protocol.
> 
> 
> > > EINVAL is standard error code used when a wrong value is passed
> > > as a parameter. In this case, the code is not able to handle
> > > the given address family.
> > 
> > This is possible, but it will produce more generic message.
> 
> I am not sure that I understand it. We do not pass the error code
> anywhere. The patch only changes the string that is shown instead
> of the requested value. It is a hint that something is wrong
> either with the caller or with the vsprintf() implementation.
> 
> I think that it does not make sense to do a big deal from it.
> "(einval)" looks informative enough to me.

OK, let's go with it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 9/9] vsprintf: Avoid confusion between invalid address and value

[Sergey Senozhatsky]

On (02/08/19 16:23), Petr Mladek wrote:
[..]
>  Plain Pointers
>  --------------
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3a95b4d1ca2e..e51cbc2be540 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1510,7 +1510,7 @@ char *ip_addr_string(char *buf, char *end, const void *ptr,
>  		case AF_INET6:
>  			return ip6_addr_string_sa(buf, end, &sa->v6, spec, fmt);
>  		default:
> -			return string_nocheck(buf, end, "(invalid address)", spec);
> +			return string_nocheck(buf, end, "(einval)", spec);
>  		}}

Hmm... The original code looks "a bit" dangerous.

Suppose, in my driver I want to sprintf() IPv4 address. The longest
possible address is 3 * 4 (%d%d%d) + 3 bytes (dots) + terminating NULL.
E.g. 111.111.111.111

So I can allocate a 16-bytes buffer (stack or slab) and accidentally
do an %piS sprintf() on a corrupted in_addr struct:

		char buf[16];
		sprintf(buf, "%piS", in_addr);

forcing sprintf() to write "(invalid address)" to a 16-bytes buffer,
but the thing is - strlen("(invalid address)") > 16.


We might want to take this change out of this series.

	-ss

Re: [PATCH v6 9/9] vsprintf: Avoid confusion between invalid address and value

[Andy Shevchenko]

On Tue, Feb 19, 2019 at 5:07 AM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:

> Suppose, in my driver I want to sprintf() IPv4 address. The longest
> possible address is 3 * 4 (%d%d%d) + 3 bytes (dots) + terminating NULL.
> E.g. 111.111.111.111
>
> So I can allocate a 16-bytes buffer (stack or slab) and accidentally
> do an %piS sprintf() on a corrupted in_addr struct:
>
>                 char buf[16];
>                 sprintf(buf, "%piS", in_addr);
>
> forcing sprintf() to write "(invalid address)" to a 16-bytes buffer,
> but the thing is - strlen("(invalid address)") > 16.

It would print as many characters as buffer allows. In your case above
the use of sprintf() is a bit fragile.

But yes, the error messages should not be longer than 8 / 16 bytes
depending on 32- or 64-bit build we have.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 9/9] vsprintf: Avoid confusion between invalid address and value

[Petr Mladek]

On Tue 2019-02-19 13:06:17, Andy Shevchenko wrote:
> On Tue, Feb 19, 2019 at 5:07 AM Sergey Senozhatsky
> <sergey.senozhatsky.work@gmail.com> wrote:
> 
> > Suppose, in my driver I want to sprintf() IPv4 address. The longest
> > possible address is 3 * 4 (%d%d%d) + 3 bytes (dots) + terminating NULL.
> > E.g. 111.111.111.111
> >
> > forcing sprintf() to write "(invalid address)" to a 16-bytes buffer,
> > but the thing is - strlen("(invalid address)") > 16.

Good catch!

> It would print as many characters as buffer allows. In your case above
> the use of sprintf() is a bit fragile.

Heh, I do not see it used anywhere in the code. Only "%pi6" is used
in a real code and always a safe way.

> But yes, the error messages should not be longer than 8 / 16 bytes
> depending on 32- or 64-bit build we have.

Well, better be on the safe side. I'll move it at the beginning
of the patchset.

Best Regards,
Petr

PS: I am a bit busy with some other things. I'll send v7 later.
I think that it is a material for 5.2. This patchset would deserve
some testing in linux-next and we are already too close to
the 5.1 merge window.

Re: [PATCH v6 9/9] vsprintf: Avoid confusion between invalid address and value

[Sergey Senozhatsky]

On (02/20/19 10:24), Petr Mladek wrote:
> Well, better be on the safe side. I'll move it at the beginning
> of the patchset.
>
> PS: I am a bit busy with some other things. I'll send v7 later.

If this is the only change you are going to make then I'm OK with v6.

	-ss