[PATCH v2 1/9] lib/test_printf: Mark big constant with ULL

[PATCH v2 1/9] lib/test_printf: Mark big constant with ULL

[Andy Shevchenko]

Sparse complains that constant is so bit for unsigned long on 64-bit
architecture.

lib/test_printf.c:217:54: warning: constant 0xffff0123456789ab is so big it is unsigned long
lib/test_printf.c:246:54: warning: constant 0xffff0123456789ab is so big it is unsigned long

To satisfy everyone, mark the constant with ULL.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/test_printf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 71ebfa43ad05..309cf8d7e6d4 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -204,7 +204,7 @@ test_string(void)
 #if BITS_PER_LONG == 64
 
 #define PTR_WIDTH 16
-#define PTR ((void *)0xffff0123456789ab)
+#define PTR ((void *)0xffff0123456789abULL)
 #define PTR_STR "ffff0123456789ab"
 #define ZEROS "00000000"	/* hex 32 zero bits */
 
-- 
2.15.1

[PATCH v2 2/9] lib/vsprintf: Make dec_spec global

[Andy Shevchenko]

There are places where default specification to print decimal numbers
is in use.

Make it global and convert existing users.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7a708f82559..8f29af063d8a 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -693,6 +693,11 @@ char *symbol_string(char *buf, char *end, void *ptr,
 #endif
 }
 
+static const struct printf_spec default_dec_spec = {
+	.base = 10,
+	.precision = -1,
+};
+
 static noinline_for_stack
 char *resource_string(char *buf, char *end, struct resource *res,
 		      struct printf_spec spec, const char *fmt)
@@ -722,11 +727,6 @@ char *resource_string(char *buf, char *end, struct resource *res,
 		.precision = -1,
 		.flags = SMALL | ZEROPAD,
 	};
-	static const struct printf_spec dec_spec = {
-		.base = 10,
-		.precision = -1,
-		.flags = 0,
-	};
 	static const struct printf_spec str_spec = {
 		.field_width = -1,
 		.precision = 10,
@@ -760,10 +760,10 @@ char *resource_string(char *buf, char *end, struct resource *res,
 		specp = &mem_spec;
 	} else if (res->flags & IORESOURCE_IRQ) {
 		p = string(p, pend, "irq ", str_spec);
-		specp = &dec_spec;
+		specp = &default_dec_spec;
 	} else if (res->flags & IORESOURCE_DMA) {
 		p = string(p, pend, "dma ", str_spec);
-		specp = &dec_spec;
+		specp = &default_dec_spec;
 	} else if (res->flags & IORESOURCE_BUS) {
 		p = string(p, pend, "bus ", str_spec);
 		specp = &bus_spec;
@@ -903,9 +903,6 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap,
 	int cur, rbot, rtop;
 	bool first = true;
 
-	/* reused to print numbers */
-	spec = (struct printf_spec){ .base = 10 };
-
 	rbot = cur = find_first_bit(bitmap, nr_bits);
 	while (cur < nr_bits) {
 		rtop = cur;
@@ -920,13 +917,13 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap,
 		}
 		first = false;
 
-		buf = number(buf, end, rbot, spec);
+		buf = number(buf, end, rbot, default_dec_spec);
 		if (rbot < rtop) {
 			if (buf < end)
 				*buf = '-';
 			buf++;
 
-			buf = number(buf, end, rtop, spec);
+			buf = number(buf, end, rtop, default_dec_spec);
 		}
 
 		rbot = cur;
-- 
2.15.1

Re: [PATCH v2 2/9] lib/vsprintf: Make dec_spec global

[Petr Mladek]

On Fri 2018-02-16 23:07:04, Andy Shevchenko wrote:
> There are places where default specification to print decimal numbers
> is in use.
> 
> Make it global and convert existing users.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

I have pushed it into printk.git, branch for-4.18-vsprintf-cleanup.

Best Regards,
Petr

[PATCH v2 3/9] lib/vsprintf: Make strspec global

[Andy Shevchenko]

There are places where default specification to print strings
is in use.

Make it global and convert existing users.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 8f29af063d8a..0c23b006b495 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -693,6 +693,11 @@ char *symbol_string(char *buf, char *end, void *ptr,
 #endif
 }
 
+static const struct printf_spec default_str_spec = {
+	.field_width = -1,
+	.precision = -1,
+};
+
 static const struct printf_spec default_dec_spec = {
 	.base = 10,
 	.precision = -1,
@@ -1461,10 +1466,6 @@ char *format_flags(char *buf, char *end, unsigned long flags,
 					const struct trace_print_flags *names)
 {
 	unsigned long mask;
-	const struct printf_spec strspec = {
-		.field_width = -1,
-		.precision = -1,
-	};
 	const struct printf_spec numspec = {
 		.flags = SPECIAL|SMALL,
 		.field_width = -1,
@@ -1477,7 +1478,7 @@ char *format_flags(char *buf, char *end, unsigned long flags,
 		if ((flags & mask) != mask)
 			continue;
 
-		buf = string(buf, end, names->name, strspec);
+		buf = string(buf, end, names->name, default_str_spec);
 
 		flags &= ~mask;
 		if (flags) {
@@ -1535,22 +1536,18 @@ char *device_node_gen_full_name(const struct device_node *np, char *buf, char *e
 {
 	int depth;
 	const struct device_node *parent = np->parent;
-	static const struct printf_spec strspec = {
-		.field_width = -1,
-		.precision = -1,
-	};
 
 	/* special case for root node */
 	if (!parent)
-		return string(buf, end, "/", strspec);
+		return string(buf, end, "/", default_str_spec);
 
 	for (depth = 0; parent->parent; depth++)
 		parent = parent->parent;
 
 	for ( ; depth >= 0; depth--) {
-		buf = string(buf, end, "/", strspec);
+		buf = string(buf, end, "/", default_str_spec);
 		buf = string(buf, end, device_node_name_for_depth(np, depth),
-			     strspec);
+			     default_str_spec);
 	}
 	return buf;
 }
-- 
2.15.1

Re: [PATCH v2 3/9] lib/vsprintf: Make strspec global

[Petr Mladek]

On Fri 2018-02-16 23:07:05, Andy Shevchenko wrote:
> There are places where default specification to print strings
> is in use.
> 
> Make it global and convert existing users.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

I have pushed it into printk.git, branch for-4.18-vsprintf-cleanup.

Best Regards,
Petr

[PATCH v2 4/9] lib/vsprintf: Make flag_spec global

[Andy Shevchenko]

There are places where default specification to print flags as number
is in use.

Make it global and convert existing users.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 0c23b006b495..c789d265311b 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -698,6 +698,12 @@ static const struct printf_spec default_str_spec = {
 	.precision = -1,
 };
 
+static const struct printf_spec default_flag_spec = {
+	.base = 16,
+	.precision = -1,
+	.flags = SPECIAL | SMALL,
+};
+
 static const struct printf_spec default_dec_spec = {
 	.base = 10,
 	.precision = -1,
@@ -737,11 +743,6 @@ char *resource_string(char *buf, char *end, struct resource *res,
 		.precision = 10,
 		.flags = LEFT,
 	};
-	static const struct printf_spec flag_spec = {
-		.base = 16,
-		.precision = -1,
-		.flags = SPECIAL | SMALL,
-	};
 
 	/* 32-bit res (sizeof==4): 10 chars in dec, 10 in hex ("0x" + 8)
 	 * 64-bit res (sizeof==8): 20 chars in dec, 18 in hex ("0x" + 16) */
@@ -798,7 +799,7 @@ char *resource_string(char *buf, char *end, struct resource *res,
 			p = string(p, pend, " disabled", str_spec);
 	} else {
 		p = string(p, pend, " flags ", str_spec);
-		p = number(p, pend, res->flags, flag_spec);
+		p = number(p, pend, res->flags, default_flag_spec);
 	}
 	*p++ = ']';
 	*p = '\0';
@@ -1466,12 +1467,6 @@ char *format_flags(char *buf, char *end, unsigned long flags,
 					const struct trace_print_flags *names)
 {
 	unsigned long mask;
-	const struct printf_spec numspec = {
-		.flags = SPECIAL|SMALL,
-		.field_width = -1,
-		.precision = -1,
-		.base = 16,
-	};
 
 	for ( ; flags && names->name; names++) {
 		mask = names->mask;
@@ -1489,7 +1484,7 @@ char *format_flags(char *buf, char *end, unsigned long flags,
 	}
 
 	if (flags)
-		buf = number(buf, end, flags, numspec);
+		buf = number(buf, end, flags, default_flag_spec);
 
 	return buf;
 }
-- 
2.15.1

Re: [PATCH v2 4/9] lib/vsprintf: Make flag_spec global

[Petr Mladek]

On Fri 2018-02-16 23:07:06, Andy Shevchenko wrote:
> There are places where default specification to print flags as number
> is in use.
> 
> Make it global and convert existing users.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

I have pushed it into printk.git, branch for-4.18-vsprintf-cleanup.

Best Regards,
Petr

[PATCH v2 5/9] lib/vsprintf: Move pointer_string() upper

[Andy Shevchenko]

As preparatory patch to further clean up.

No functional change.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index c789d265311b..87dbced51b1a 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1347,6 +1347,20 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 	return string(buf, end, uuid, 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);
+}
+
 int kptr_restrict __read_mostly;
 
 static noinline_for_stack
@@ -1634,20 +1648,6 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	return widen_string(buf, buf - buf_start, end, 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;
 
-- 
2.15.1

Re: [PATCH v2 5/9] lib/vsprintf: Move pointer_string() upper

[Petr Mladek]

On Fri 2018-02-16 23:07:07, Andy Shevchenko wrote:
> As preparatory patch to further clean up.
> 
> No functional change.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

I have pushed it into printk.git, branch for-4.18-vsprintf-cleanup.

Best Regards,
Petr

[PATCH v2 6/9] lib/vsprintf: Deduplicate pointer_string()

[Andy Shevchenko]

There is an exact code at the end of ptr_to_id().
Replace it by calling pointer_string() directly.

This is followup to the commit
  ad67b74d2469 ("printk: hash addresses printed with %p").

Cc: Tobin C. Harding <me@tobin.cc>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 87dbced51b1a..9004bbb3d84d 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1367,13 +1367,6 @@ static noinline_for_stack
 char *restricted_pointer(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;
-	}
-
 	switch (kptr_restrict) {
 	case 0:
 		/* Always print %pK values */
@@ -1385,8 +1378,11 @@ char *restricted_pointer(char *buf, char *end, const void *ptr,
 		 * 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 (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
@@ -1411,7 +1407,7 @@ char *restricted_pointer(char *buf, char *end, const void *ptr,
 		break;
 	}
 
-	return number(buf, end, (unsigned long)ptr, spec);
+	return pointer_string(buf, end, ptr, spec);
 }
 
 static noinline_for_stack
@@ -1686,10 +1682,9 @@ early_initcall(initialize_ptr_random);
 static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
 {
 	unsigned long hashval;
-	const int default_width = 2 * sizeof(ptr);
 
 	if (unlikely(!have_filled_random_ptr_key)) {
-		spec.field_width = default_width;
+		spec.field_width = 2 * sizeof(ptr);
 		/* string length must be less than default_width */
 		return string(buf, end, "(ptrval)", spec);
 	}
@@ -1704,15 +1699,7 @@ static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
 #else
 	hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
 #endif
-
-	spec.flags |= SMALL;
-	if (spec.field_width == -1) {
-		spec.field_width = default_width;
-		spec.flags |= ZEROPAD;
-	}
-	spec.base = 16;
-
-	return number(buf, end, hashval, spec);
+	return pointer_string(buf, end, (const void *)hashval, spec);
 }
 
 /*
-- 
2.15.1

Re: [PATCH v2 6/9] lib/vsprintf: Deduplicate pointer_string()

[Petr Mladek]

On Fri 2018-02-16 23:07:08, Andy Shevchenko wrote:
> There is an exact code at the end of ptr_to_id().
> Replace it by calling pointer_string() directly.
> 
> This is followup to the commit
>   ad67b74d2469 ("printk: hash addresses printed with %p").
> 
> Cc: Tobin C. Harding <me@tobin.cc>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

I have pushed it into printk.git, branch for-4.18-vsprintf-cleanup.

Best Regards,
Petr

[PATCH v2 7/9] lib/vsprintf: Replace space with '_' before crng is ready

[Andy Shevchenko]

From: Shunyong Yang <shunyong.yang@hxt-semitech.com>

Before crng is ready, output of "%p" composes of "(ptrval)" and
left padding spaces for alignment as no random address can be
generated. This seems a little strange when default string width
is larger than strlen("(ptrval)").

For example, when irq domain names are built with "%p", the nodes
under /sys/kernel/debug/irq/domains like this on AArch64 system,

[root@y irq]# ls domains/
default                   irqchip@        (ptrval)-2
irqchip@        (ptrval)-4  \_SB_.TCS0.QIC1  \_SB_.TCS0.QIC3
irqchip@        (ptrval)  irqchip@        (ptrval)-3
\_SB_.TCS0.QIC0             \_SB_.TCS0.QIC2

The name "irqchip@        (ptrval)-2" is not so readable in console
output.

This patch replaces space with readable "_" when output needs padding.
Following is the output after applying the patch,

[root@y domains]# ls
default                   irqchip@(____ptrval____)-2
irqchip@(____ptrval____)-4  \_SB_.TCS0.QIC1  \_SB_.TCS0.QIC3
irqchip@(____ptrval____)  irqchip@(____ptrval____)-3  \_SB_.TCS0.QIC0
\_SB_.TCS0.QIC2

There is same problem in some subsystem's dmesg output. Moreover,
someone may call "%p" in a similar case. In addition, the timing of
crng initialization done may vary on different system. So, the change
is made in vsprintf.c.

Cc: Joey Zheng <yu.zheng@hxt-semitech.com>
Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Shunyong Yang <shunyong.yang@hxt-semitech.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 9004bbb3d84d..97be2d07297a 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1681,12 +1681,13 @@ 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, "(ptrval)", spec);
+		return string(buf, end, str, spec);
 	}
 
 #ifdef CONFIG_64BIT
-- 
2.15.1

Re: [此邮件可能存在风险] [PATCH v2 7/9] lib/vsprintf: Replace space with '_' before crng is ready

[Yang, Shunyong]

Hi, Andy,

Many thanks for the change. I am on Chinese New Year travel and slow
response. :-)

Thanks.
Shunyong.

On Fri, 2018-02-16 at 23:07 +0200, Andy Shevchenko wrote:
> From: Shunyong Yang <shunyong.yang@hxt-semitech.com>
> 
> Before crng is ready, output of "%p" composes of "(ptrval)" and
> left padding spaces for alignment as no random address can be
> generated. This seems a little strange when default string width
> is larger than strlen("(ptrval)").
> 
> For example, when irq domain names are built with "%p", the nodes
> under /sys/kernel/debug/irq/domains like this on AArch64 system,
> 
> [root@y irq]# ls domains/
> default                   irqchip@        (ptrval)-2
> irqchip@        (ptrval)-4  \_SB_.TCS0.QIC1  \_SB_.TCS0.QIC3
> irqchip@        (ptrval)  irqchip@        (ptrval)-3
> \_SB_.TCS0.QIC0             \_SB_.TCS0.QIC2
> 
> The name "irqchip@        (ptrval)-2" is not so readable in console
> output.
> 
> This patch replaces space with readable "_" when output needs
> padding.
> Following is the output after applying the patch,
> 
> [root@y domains]# ls
> default                   irqchip@(____ptrval____)-2
> irqchip@(____ptrval____)-4  \_SB_.TCS0.QIC1  \_SB_.TCS0.QIC3
> irqchip@(____ptrval____)  irqchip@(____ptrval____)-3  \_SB_.TCS0.QIC0
> \_SB_.TCS0.QIC2
> 
> There is same problem in some subsystem's dmesg output. Moreover,
> someone may call "%p" in a similar case. In addition, the timing of
> crng initialization done may vary on different system. So, the change
> is made in vsprintf.c.
> 
> Cc: Joey Zheng <yu.zheng@hxt-semitech.com>
> Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Shunyong Yang <shunyong.yang@hxt-semitech.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/vsprintf.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 9004bbb3d84d..97be2d07297a 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1681,12 +1681,13 @@ 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, "(ptrval)", spec);
> +		return string(buf, end, str, spec);
>  	}
>  
>  #ifdef CONFIG_64BIT

Re: [PATCH v2 7/9] lib/vsprintf: Replace space with '_' before crng is ready

[Petr Mladek]

On Fri 2018-02-16 23:07:09, Andy Shevchenko wrote:
> From: Shunyong Yang <shunyong.yang@hxt-semitech.com>
> 
> Before crng is ready, output of "%p" composes of "(ptrval)" and
> left padding spaces for alignment as no random address can be
> generated. This seems a little strange when default string width
> is larger than strlen("(ptrval)").
> 
> For example, when irq domain names are built with "%p", the nodes
> under /sys/kernel/debug/irq/domains like this on AArch64 system,
> 
> [root@y irq]# ls domains/
> default                   irqchip@        (ptrval)-2
> irqchip@        (ptrval)-4  \_SB_.TCS0.QIC1  \_SB_.TCS0.QIC3
> irqchip@        (ptrval)  irqchip@        (ptrval)-3
> \_SB_.TCS0.QIC0             \_SB_.TCS0.QIC2
> 
> The name "irqchip@        (ptrval)-2" is not so readable in console
> output.
> 
> This patch replaces space with readable "_" when output needs padding.
> Following is the output after applying the patch,
> 
> [root@y domains]# ls
> default                   irqchip@(____ptrval____)-2
> irqchip@(____ptrval____)-4  \_SB_.TCS0.QIC1  \_SB_.TCS0.QIC3
> irqchip@(____ptrval____)  irqchip@(____ptrval____)-3  \_SB_.TCS0.QIC0
> \_SB_.TCS0.QIC2
> 
> There is same problem in some subsystem's dmesg output. Moreover,
> someone may call "%p" in a similar case. In addition, the timing of
> crng initialization done may vary on different system. So, the change
> is made in vsprintf.c.
> 
> Cc: Joey Zheng <yu.zheng@hxt-semitech.com>
> Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Shunyong Yang <shunyong.yang@hxt-semitech.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

I have pushed it into printk.git, branch for-4.18-vsprintf-cleanup.

Best Regards,
Petr

[PATCH v2 8/9] lib/vsprintf: Remove useless NULL checks

[Andy Shevchenko]

The pointer can't be NULL since it's first what has been done in the
pointer().

Remove useless checks.

Note we leave check for !CONFIG_HAVE_CLK to make compiler
to optimize code away when possible.

Cc: Petr Mladek <pmladek@suse.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 97be2d07297a..a49da00b79e7 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -819,10 +819,6 @@ 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);
-
 	switch (fmt[1]) {
 	case 'C':
 		separator = ':';
@@ -1258,10 +1254,6 @@ 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 */
-
-
 	do {
 		switch (fmt[count++]) {
 		case 'a':
@@ -1455,7 +1447,7 @@ 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(buf, end, NULL, spec);
 
 	switch (fmt[1]) {
@@ -1580,9 +1572,6 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	if (!IS_ENABLED(CONFIG_OF))
 		return string(buf, end, "(!OF)", spec);
 
-	if ((unsigned long)dn < PAGE_SIZE)
-		return string(buf, end, "(null)", spec);
-
 	/* simple case without anything any more format specifiers */
 	fmt++;
 	if (fmt[0] == '\0' || strcspn(fmt,"fnpPFcC") > 0)
-- 
2.15.1

Re: [PATCH v2 8/9] lib/vsprintf: Remove useless NULL checks

[Petr Mladek]

On Fri 2018-02-16 23:07:10, Andy Shevchenko wrote:
> The pointer can't be NULL since it's first what has been done in the
> pointer().
> 
> Remove useless checks.
> 
> Note we leave check for !CONFIG_HAVE_CLK to make compiler
> to optimize code away when possible.
> 
> Cc: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/vsprintf.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 97be2d07297a..a49da00b79e7 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -819,10 +819,6 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
>  		/* nothing to print */
>  		return buf;
>  
> -	if (ZERO_OR_NULL_PTR(addr))

This macro matches also values <= 16. All these values are handled as NULL
by kfree(). Therefore it would make sense to write "(null)" for them.

But pointer() prints "(null)" only for ptr == 0.

See below.

> -		/* NULL pointer */
> -		return string(buf, end, NULL, spec);
> -
>  	switch (fmt[1]) {
>  	case 'C':
>  		separator = ':';
> @@ -1258,10 +1254,6 @@ 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 */
> -
> -
>  	do {
>  		switch (fmt[count++]) {
>  		case 'a':
> @@ -1455,7 +1447,7 @@ 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(buf, end, NULL, spec);
>  
>  	switch (fmt[1]) {
> @@ -1580,9 +1572,6 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
>  	if (!IS_ENABLED(CONFIG_OF))
>  		return string(buf, end, "(!OF)", spec);
>  
> -	if ((unsigned long)dn < PAGE_SIZE)
> -		return string(buf, end, "(null)", spec);

In this case, "null" was printed for ptr < PAGE_SIZE. The same check
is also in string() function.

Note that it is not only about the printed value. The pointer is later
derefecend. We will start crashing on dn > 0 && dn < PAGE_SIZE.


To be honest, I do not feel experienced enough to decide
about the preferred behavior. On one hand, it is bad when
printk() would crash the kernel. On the other hand, hiding wide
range of values under "(null)" string might confuse people.

Would it make sense to survive and write different strings for
difference intervals? For example?

    "(null)"     for ptr == 0
    "(null-16)"  for ptr > 0 && ptr <= 16
    "(null-pg)"  for prt > 16 && ptr <= PAGE_SIZE

In each case, this patch changes the behavior and it should
be documented in the commit message.

Best Regards,
Petr

Re: [PATCH v2 8/9] lib/vsprintf: Remove useless NULL checks

[Andy Shevchenko]

On Tue, 2018-02-27 at 16:50 +0100, Petr Mladek wrote:
> On Fri 2018-02-16 23:07:10, Andy Shevchenko wrote:
> > The pointer can't be NULL since it's first what has been done in the
> > pointer().
> > 
> > Remove useless checks.
> > 
> > Note we leave check for !CONFIG_HAVE_CLK to make compiler
> > to optimize code away when possible.
> > 
> > Cc: Petr Mladek <pmladek@suse.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  lib/vsprintf.c | 13 +------------
> >  1 file changed, 1 insertion(+), 12 deletions(-)
> > 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 97be2d07297a..a49da00b79e7 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -819,10 +819,6 @@ char *hex_string(char *buf, char *end, u8
> > *addr, struct printf_spec spec,
> >  		/* nothing to print */
> >  		return buf;
> >  
> > -	if (ZERO_OR_NULL_PTR(addr))
> 
> This macro matches also values <= 16.

Yes, I know.

This had been discussed with Rasmus and we agreed that printing a result
of kmalloc(0) is rather weird.

Moreover, in couple of cases I added these checks.
 
> >  	switch (fmt[1]) {
> > @@ -1580,9 +1572,6 @@ char *device_node_string(char *buf, char *end,
> > struct device_node *dn,
> >  	if (!IS_ENABLED(CONFIG_OF))
> >  		return string(buf, end, "(!OF)", spec);
> >  
> > -	if ((unsigned long)dn < PAGE_SIZE)
> > -		return string(buf, end, "(null)", spec);
> 
> In this case, "null" was printed for ptr < PAGE_SIZE. The same check
> is also in string() function.

Do we have a uses cases when invalid (non-NULL) pointer is supplied to
print function?

Those call sites have to be fixed.

> Note that it is not only about the printed value. The pointer is later
> derefecend. We will start crashing on dn > 0 && dn < PAGE_SIZE.

Yes.
So, fix the call sites!

> To be honest, I do not feel experienced enough to decide
> about the preferred behavior. On one hand, it is bad when
> printk() would crash the kernel. On the other hand, hiding wide
> range of values under "(null)" string might confuse people.

> Would it make sense to survive and write different strings for
> difference intervals? For example?
> 
>     "(null)"     for ptr == 0
>     "(null-16)"  for ptr > 0 && ptr <= 16
>     "(null-pg)"  for prt > 16 && ptr <= PAGE_SIZE
> 
> In each case, this patch changes the behavior and it should
> be documented in the commit message.

Personally I strongly disagree with blowing code up in such places for
little or none benefit.

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

Re: [PATCH v2 8/9] lib/vsprintf: Remove useless NULL checks

[Petr Mladek]

On Tue 2018-02-27 19:35:50, Andy Shevchenko wrote:
> On Tue, 2018-02-27 at 16:50 +0100, Petr Mladek wrote:
> > On Fri 2018-02-16 23:07:10, Andy Shevchenko wrote:
> > > The pointer can't be NULL since it's first what has been done in the
> > > pointer().
> > > 
> > > Remove useless checks.
> > > 
> > > Note we leave check for !CONFIG_HAVE_CLK to make compiler
> > > to optimize code away when possible.
> > > 
> > > Cc: Petr Mladek <pmladek@suse.com>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >  lib/vsprintf.c | 13 +------------
> > >  1 file changed, 1 insertion(+), 12 deletions(-)
> > > 
> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > > index 97be2d07297a..a49da00b79e7 100644
> > > --- a/lib/vsprintf.c
> > > +++ b/lib/vsprintf.c
> > > @@ -819,10 +819,6 @@ char *hex_string(char *buf, char *end, u8
> > > *addr, struct printf_spec spec,
> > >  		/* nothing to print */
> > >  		return buf;
> > >  
> > > -	if (ZERO_OR_NULL_PTR(addr))
> > 
> > This macro matches also values <= 16.
> 
> Yes, I know.
> 
> This had been discussed with Rasmus and we agreed that printing a result
> of kmalloc(0) is rather weird.

I see
https://lkml.kernel.org/r/1500546142.29303.133.camel@linux.intel.com
There you suggested to move this check into pointer(). But I do not
see any agreement on this.


> Moreover, in couple of cases I added these checks.
>  
> > >  	switch (fmt[1]) {
> > > @@ -1580,9 +1572,6 @@ char *device_node_string(char *buf, char *end,
> > > struct device_node *dn,
> > >  	if (!IS_ENABLED(CONFIG_OF))
> > >  		return string(buf, end, "(!OF)", spec);
> > >  
> > > -	if ((unsigned long)dn < PAGE_SIZE)
> > > -		return string(buf, end, "(null)", spec);
> > 
> > In this case, "null" was printed for ptr < PAGE_SIZE. The same check
> > is also in string() function.
> 
> Do we have a uses cases when invalid (non-NULL) pointer is supplied to
> print function?
> 
> Those call sites have to be fixed.

I am not aware of any. But this patch will make fixing such locations
more complicated. The kernel would crash and might not show any message.
Is this really what we want?

Note that it will most likely crash in vprintk_emit() on the line

   text_len = vscnprintf(text, sizeof(textbuf), fmt, args);

It will be with logbug_lock() taken. The nested printk() messages
will be stored in per-CPU buffer thanks to printk_safe code.
They might eventually be printed by printk_safe_flush_on_panic()
but it is not guaranteed.


> > Note that it is not only about the printed value. The pointer is later
> > derefecend. We will start crashing on dn > 0 && dn < PAGE_SIZE.
> 
> Yes.
> So, fix the call sites!

It would be easier if printk() was able to show the message
when hitting this place.

I did some archaeology. The first check for PAGE_SIZE was added
by the pre-git commit:

commit 8bcb3ba1dec5749a7f1eed570cb69a20c2e4bd41
Author: Andrew Morton <akpm@osdl.org>
Date:   Tue Oct 21 18:22:28 2003 -0700

    [PATCH] make printk more robust with "null" pointers
    
    Expand printk's traditional handling of null pointers so that anything in the
    first page is considered a null pointer.
    
    This gives us better behaviour when someone (acpi..) accidentally prints a
    string which is embedded in a struct, the pointer to which is null.


IMHO, it would make sense to hanve this check also pointers that are
being deferred.


> > To be honest, I do not feel experienced enough to decide
> > about the preferred behavior. On one hand, it is bad when
> > printk() would crash the kernel. On the other hand, hiding wide
> > range of values under "(null)" string might confuse people.
> 
> > Would it make sense to survive and write different strings for
> > difference intervals? For example?
> > 
> >     "(null)"     for ptr == 0
> >     "(null-16)"  for ptr > 0 && ptr <= 16
> >     "(null-pg)"  for prt > 16 && ptr <= PAGE_SIZE
> > 
> > In each case, this patch changes the behavior and it should
> > be documented in the commit message.
> 
> Personally I strongly disagree with blowing code up in such places for
> little or none benefit.

I do not have strong opinion here. I could imagine that this might
save a day to some people. But I have never encountered such a bug
myself.

To make it clear. Your clean up work makes sense. I just want to point
out that this patch is not as innocent as the commit message suggest.
Also I think that it goes in the wrong direction regarding the
ability to show useful information in a buggy situation.

Best Regards,
Petr

Re: [PATCH v2 8/9] lib/vsprintf: Remove useless NULL checks

[Andy Shevchenko]

On Wed, 2018-02-28 at 11:04 +0100, Petr Mladek wrote:
> On Tue 2018-02-27 19:35:50, Andy Shevchenko wrote:
> > On Tue, 2018-02-27 at 16:50 +0100, Petr Mladek wrote:
> > > On Fri 2018-02-16 23:07:10, Andy Shevchenko wrote:
> > > > The pointer can't be NULL since it's first what has been done in
> > > > the
> > > > pointer().
> > > > 
> > > > Remove useless checks.
> > > > 
> > > > Note we leave check for !CONFIG_HAVE_CLK to make compiler
> > > > to optimize code away when possible.
> > > > 

> I see
> https://lkml.kernel.org/r/1500546142.29303.133.camel@linux.intel.com
> There you suggested to move this check into pointer(). But I do not
> see any agreement on this.

> I am not aware of any. But this patch will make fixing such locations
> more complicated. The kernel would crash and might not show any
> message.

> Is this really what we want?

I never see such, so, I don't know what we want here.

> Note that it will most likely crash in vprintk_emit() on the line
> 
>    text_len = vscnprintf(text, sizeof(textbuf), fmt, args);
> 
> It will be with logbug_lock() taken. The nested printk() messages
> will be stored in per-CPU buffer thanks to printk_safe code.

Yeah, that's bad.


> It would be easier if printk() was able to show the message
> when hitting this place.
> 
> I did some archaeology. The first check for PAGE_SIZE was added
> by the pre-git commit:
> 
> commit 8bcb3ba1dec5749a7f1eed570cb69a20c2e4bd41
> Author: Andrew Morton <akpm@osdl.org>
> Date:   Tue Oct 21 18:22:28 2003 -0700
> 
>     [PATCH] make printk more robust with "null" pointers
>     
>     Expand printk's traditional handling of null pointers so that
> anything in the
>     first page is considered a null pointer.
>     
>     This gives us better behaviour when someone (acpi..) accidentally
> prints a
>     string which is embedded in a struct, the pointer to which is
> null.
> 
> 
> IMHO, it would make sense to hanve this check also pointers that are
> being deferred.

Send a patch to discuss!

> > > To be honest, I do not feel experienced enough to decide
> > > about the preferred behavior. On one hand, it is bad when
> > > printk() would crash the kernel. On the other hand, hiding wide
> > > range of values under "(null)" string might confuse people.
> > > Would it make sense to survive and write different strings for
> > > difference intervals? For example?
> > > 
> > >     "(null)"     for ptr == 0
> > >     "(null-16)"  for ptr > 0 && ptr <= 16
> > >     "(null-pg)"  for prt > 16 && ptr <= PAGE_SIZE
> > > 
> > > In each case, this patch changes the behavior and it should
> > > be documented in the commit message.
> > 
> > Personally I strongly disagree with blowing code up in such places
> > for
> > little or none benefit.
> 
> I do not have strong opinion here. I could imagine that this might
> save a day to some people. But I have never encountered such a bug
> myself.
> 
> To make it clear. Your clean up work makes sense. I just want to point
> out that this patch is not as innocent as the commit message suggest.
> Also I think that it goes in the wrong direction regarding the
> ability to show useful information in a buggy situation.

Send a patch to discuss!

I consider silence as not preventing me doing my way. It seems you are
the first one who looks into this closer than the other(s) [Rasmus].

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

Re: [PATCH v2 8/9] lib/vsprintf: Remove useless NULL checks

[Petr Mladek]

On Wed 2018-02-28 12:42:24, Andy Shevchenko wrote:
> On Wed, 2018-02-28 at 11:04 +0100, Petr Mladek wrote:
> > On Tue 2018-02-27 19:35:50, Andy Shevchenko wrote:
> > > On Tue, 2018-02-27 at 16:50 +0100, Petr Mladek wrote:
> > Note that it will most likely crash in vprintk_emit() on the line
> > 
> >    text_len = vscnprintf(text, sizeof(textbuf), fmt, args);
> > 
> > It will be with logbug_lock() taken. The nested printk() messages
> > will be stored in per-CPU buffer thanks to printk_safe code.
> 
> Yeah, that's bad.
> 
> > IMHO, it would make sense to hanve this check also pointers that are
> > being deferred.
> 
> Send a patch to discuss!

I thought about this more. IMHO, the check for PAGE_SIZE in pointer()
makes perfect sense. It helps to avoid crash and actually see
the message. I am going to send the patch in a minute.

BTW: I am not sure who is going to pass this patchset to Linus.
If nobody is against, I could eventually do so via printk.git.


> > > > To be honest, I do not feel experienced enough to decide
> > > > about the preferred behavior. On one hand, it is bad when
> > > > printk() would crash the kernel. On the other hand, hiding wide
> > > > range of values under "(null)" string might confuse people.
> > > > Would it make sense to survive and write different strings for
> > > > difference intervals? For example?
> > > > 
> > > >     "(null)"     for ptr == 0
> > > >     "(null-16)"  for ptr > 0 && ptr <= 16
> > > >     "(null-pg)"  for prt > 16 && ptr <= PAGE_SIZE
> > > > 
> > > > In each case, this patch changes the behavior and it should
> > > > be documented in the commit message.
> > > 
> > > Personally I strongly disagree with blowing code up in such places
> > > for little or none benefit.
> 
> Send a patch to discuss!

I am not going to do so unless there is an evidence that people are
confused or that the above idea is desired.

Best Regards,
Petr

[PATCH] vsprintf: Make "null" pointer dereference more robust

[Petr Mladek]

%p has many modifiers where the pointer is dereferenced. An invalid
pointer might cause kernel to crash silently.

Note that printk() formats the string 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.

In general, we should do our best to get useful message from printk().
All pointers to the first memory page must be invalid. Let's prevent
the dereference and print "(null)" in this case. This is already done
in many other situations, including "%s" format handling and many
page fault handlers.

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

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7a708f82559..5c2d1f44218a 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1849,7 +1849,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 {
 	const int default_width = 2 * sizeof(void *);
 
-	if (!ptr && *fmt != 'K' && *fmt != 'x') {
+	if ((unsigned long)ptr < PAGE_SIZE && *fmt != 'K' && *fmt != 'x') {
 		/*
 		 * Print (null) with the same width as a pointer so it makes
 		 * tabular output look nice.
-- 
2.13.6

Re: [PATCH] vsprintf: Make "null" pointer dereference more robust

[Andy Shevchenko]

On Fri, 2018-03-02 at 13:53 +0100, Petr Mladek wrote:
> %p has many modifiers where the pointer is dereferenced. An invalid
> pointer might cause kernel to crash silently.
> 
> Note that printk() formats the string 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.
> 
> In general, we should do our best to get useful message from printk().
> All pointers to the first memory page must be invalid. Let's prevent
> the dereference and print "(null)" in this case. This is already done
> in many other situations, including "%s" format handling and many
> page fault handlers.
> 


With such explanation it makes at least clear for the reader why it's
done.

Thanks!

Would you be okay if I take this one as a first in my series and
resubmit the series based on it?

> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  lib/vsprintf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index d7a708f82559..5c2d1f44218a 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1849,7 +1849,7 @@ char *pointer(const char *fmt, char *buf, char
> *end, void *ptr,
>  {
>  	const int default_width = 2 * sizeof(void *);
>  
> -	if (!ptr && *fmt != 'K' && *fmt != 'x') {
> +	if ((unsigned long)ptr < PAGE_SIZE && *fmt != 'K' && *fmt !=
> 'x') {
>  		/*
>  		 * Print (null) with the same width as a pointer so
> it makes
>  		 * tabular output look nice.

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

Re: [PATCH] vsprintf: Make "null" pointer dereference more robust

[Petr Mladek]

On Fri 2018-03-02 16:17:34, Andy Shevchenko wrote:
> On Fri, 2018-03-02 at 13:53 +0100, Petr Mladek wrote:
> > %p has many modifiers where the pointer is dereferenced. An invalid
> > pointer might cause kernel to crash silently.
> > 
> > Note that printk() formats the string 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.
> > 
> > In general, we should do our best to get useful message from printk().
> > All pointers to the first memory page must be invalid. Let's prevent
> > the dereference and print "(null)" in this case. This is already done
> > in many other situations, including "%s" format handling and many
> > page fault handlers.
> > 
> 
> 
> With such explanation it makes at least clear for the reader why it's
> done.
> 
> Thanks!
> 
> Would you be okay if I take this one as a first in my series and
> resubmit the series based on it?

Makes sense. Feel free to go on.

Best Regards,
Petr

Re: [PATCH] vsprintf: Make "null" pointer dereference more robust

[Petr Mladek]

On Fri 2018-03-02 16:17:34, Andy Shevchenko wrote:
> On Fri, 2018-03-02 at 13:53 +0100, Petr Mladek wrote:
> > %p has many modifiers where the pointer is dereferenced. An invalid
> > pointer might cause kernel to crash silently.
> > 
> > Note that printk() formats the string 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.
> > 
> > In general, we should do our best to get useful message from printk().
> > All pointers to the first memory page must be invalid. Let's prevent
> > the dereference and print "(null)" in this case. This is already done
> > in many other situations, including "%s" format handling and many
> > page fault handlers.
> > 
> 
> 
> With such explanation it makes at least clear for the reader why it's
> done.
> 
> Thanks!
> 
> Would you be okay if I take this one as a first in my series and
> resubmit the series based on it?

The original simple patch grew into something bigger. I have it
almost ready. It looks the following way at the moment:

  vsprintf: Shuffle ptr_to_id() code
  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: Avoid confusion between invalid address and value

 Documentation/core-api/printk-formats.rst |   8 +
 lib/test_printf.c                         |  37 ++-
 lib/vsprintf.c                            | 445 ++++++++++++++++++------------
 3 files changed, 307 insertions(+), 183 deletions(-)

I still would like to do some final review and check with a fresh
head after Easter holidays.


Now, I am unsure how to merge it with your patchset. Most of your
patches are independent and should be applied. But there will be
some merge conflicts.

On possibility would be that I take your still valid changes via
printk.git. Then you would even need not send anything. I could
resolve the conflicts when applying the patches.

Or would you prefer to resend your patchset without the controversal
check removal? And wait for my patchset?

Best Regards,
Petr

Re: [PATCH] vsprintf: Make "null" pointer dereference more robust

[Joe Perches]

On Thu, 2018-03-29 at 17:13 +0200, Petr Mladek wrote:
> The original simple patch grew into something bigger. I have it
> almost ready. It looks the following way at the moment:
[]
>   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()

I would like to review these before being applied.

Re: [PATCH] vsprintf: Make "null" pointer dereference more robust

[Rasmus Villemoes]

On 2 March 2018 at 13:53, Petr Mladek <pmladek@suse.com> wrote:
> %p has many modifiers where the pointer is dereferenced. An invalid
> pointer might cause kernel to crash silently.
>
> Note that printk() formats the string 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.

Yeah, it's annoying that we can't reliably WARN for bogus vsprintf() uses.

> In general, we should do our best to get useful message from printk().
> All pointers to the first memory page must be invalid. Let's prevent
> the dereference and print "(null)" in this case. This is already done
> in many other situations, including "%s" format handling and many
> page fault handlers.
>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  lib/vsprintf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index d7a708f82559..5c2d1f44218a 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1849,7 +1849,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  {
>         const int default_width = 2 * sizeof(void *);
>
> -       if (!ptr && *fmt != 'K' && *fmt != 'x') {
> +       if ((unsigned long)ptr < PAGE_SIZE && *fmt != 'K' && *fmt != 'x') {

ISTM that accidentally passing an ERR_PTR would be just as likely as
passing a NULL pointer (or some small offset from one), so if we do
this, shouldn't the test also cover IS_ERR values?

Rasmus

Re: [PATCH] vsprintf: Make "null" pointer dereference more robust

[Andy Shevchenko]

On Mon, 2018-03-05 at 16:16 +0100, Rasmus Villemoes wrote:
> On 2 March 2018 at 13:53, Petr Mladek <pmladek@suse.com> wrote:

> > -       if (!ptr && *fmt != 'K' && *fmt != 'x') {
> > +       if ((unsigned long)ptr < PAGE_SIZE && *fmt != 'K' && *fmt !=
> > 'x') {
> 
> ISTM that accidentally passing an ERR_PTR would be just as likely as
> passing a NULL pointer (or some small offset from one), so if we do
> this, shouldn't the test also cover IS_ERR values?

We (will) have such check in two places, perhaps a helper

static bool is_pointer_valid(void *ptr)
{
  return !IS_ERR(ptr) && (unsigned long)ptr >= PAGE_SIZE;
}

?

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

Re: [PATCH] vsprintf: Make "null" pointer dereference more robust

[Petr Mladek]

On Mon 2018-03-05 16:16:37, Rasmus Villemoes wrote:
> On 2 March 2018 at 13:53, Petr Mladek <pmladek@suse.com> wrote:
> > %p has many modifiers where the pointer is dereferenced. An invalid
> > pointer might cause kernel to crash silently.
> >
> > Note that printk() formats the string 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.
> 
> Yeah, it's annoying that we can't reliably WARN for bogus vsprintf() uses.
> 
> > In general, we should do our best to get useful message from printk().
> > All pointers to the first memory page must be invalid. Let's prevent
> > the dereference and print "(null)" in this case. This is already done
> > in many other situations, including "%s" format handling and many
> > page fault handlers.
> >
> > Signed-off-by: Petr Mladek <pmladek@suse.com>
> > ---
> >  lib/vsprintf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index d7a708f82559..5c2d1f44218a 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1849,7 +1849,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> >  {
> >         const int default_width = 2 * sizeof(void *);
> >
> > -       if (!ptr && *fmt != 'K' && *fmt != 'x') {
> > +       if ((unsigned long)ptr < PAGE_SIZE && *fmt != 'K' && *fmt != 'x') {
> 
> ISTM that accidentally passing an ERR_PTR would be just as likely as
> passing a NULL pointer (or some small offset from one), so if we do
> this, shouldn't the test also cover IS_ERR values?

It would make perfect sense to catch IS_ERR_PTR(). Derefenrecing
such pointer cause crash. But it might be pretty confusing to print
"(null)" in this case.

I would handle this in separate patch and print "(err)" or so.
Any volunteer to prepare the patch?

Best Regards,
Petr

Re: [PATCH] vsprintf: Make "null" pointer dereference more robust

[Andy Shevchenko]

On Tue, 2018-03-06 at 10:25 +0100, Petr Mladek wrote:
> On Mon 2018-03-05 16:16:37, Rasmus Villemoes wrote:
> > On 2 March 2018 at 13:53, Petr Mladek <pmladek@suse.com> wrote:

> > > -       if (!ptr && *fmt != 'K' && *fmt != 'x') {
> > > +       if ((unsigned long)ptr < PAGE_SIZE && *fmt != 'K' && *fmt
> > > != 'x') {
> > 
> > ISTM that accidentally passing an ERR_PTR would be just as likely as
> > passing a NULL pointer (or some small offset from one), so if we do
> > this, shouldn't the test also cover IS_ERR values?
> 
> It would make perfect sense to catch IS_ERR_PTR(). Derefenrecing
> such pointer cause crash. But it might be pretty confusing to print
> "(null)" in this case.
> 
> I would handle this in separate patch and print "(err)" or so.
> Any volunteer to prepare the patch?

As I pointed out, we have already such check for %s in binary printf().
And it goes for "(null)". I'm not sure if changing that might break
something.

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

Re: [PATCH] vsprintf: Make "null" pointer dereference more robust

[Petr Mladek]

On Tue 2018-03-06 11:56:25, Andy Shevchenko wrote:
> On Tue, 2018-03-06 at 10:25 +0100, Petr Mladek wrote:
> > On Mon 2018-03-05 16:16:37, Rasmus Villemoes wrote:
> > > On 2 March 2018 at 13:53, Petr Mladek <pmladek@suse.com> wrote:
> 
> > > > -       if (!ptr && *fmt != 'K' && *fmt != 'x') {
> > > > +       if ((unsigned long)ptr < PAGE_SIZE && *fmt != 'K' && *fmt
> > > > != 'x') {
> > > 
> > > ISTM that accidentally passing an ERR_PTR would be just as likely as
> > > passing a NULL pointer (or some small offset from one), so if we do
> > > this, shouldn't the test also cover IS_ERR values?
> > 
> > It would make perfect sense to catch IS_ERR_PTR(). Derefenrecing
> > such pointer cause crash. But it might be pretty confusing to print
> > "(null)" in this case.
> > 
> > I would handle this in separate patch and print "(err)" or so.
> > Any volunteer to prepare the patch?
> 
> As I pointed out, we have already such check for %s in binary printf().
> And it goes for "(null)". I'm not sure if changing that might break
> something.

I have missed this.

Anyway, I have discussed this with my colleagues. Different people had
different opinions. But I liked the following.

If we are changing things, let's do it properly. The range
(-PAGE_SIZE,+PAGE_SIZE) is just a small subset of invalid pointers.
Let's try to catch more of them by reading one byte using
probe_kernel_read(). It would return -FAULT if we are not able
to read the address but it would not crash.

Then we clearly need a new message when dereferencing invalid
poitners that are not pure NULL. I propose (efault).

I believe that the error message is not ABI. Otherwise we would
never be able to fix this. Anyway, we would not know if we did
not try it. And I think that this is worth the risk.

Below is my RFC patch. It is rather a concept to see if it would
work. I send it now because others seem to be working on different
approaches. I believe that this is the right direction. I hope
that it does not conflict much with your patches. I deliberately
touched only the locations that are supposed to stay.


>From 3aae11b504637aa29027949709482f4570cb8bec Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@suse.com>
Date: Wed, 7 Mar 2018 16:27:24 +0100
Subject: [RFC] vsprintf: Prevent crash when dereferencing invalid pointers

We already prevent crash when derefencing some obviously broken
pointers. 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() formats the string 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.

In general, we should do our best to get useful message from printk().
This patch tries to find a wide range of invalid strings using
probe_kernel_read(). Also it makes the handling unified. We print:

   + (null) only when pure NULL pointer is dereferenced
   + (efault) in all other cases

Note that we could not print the exact pointer value from security reasons.
Developers need print the pointer using %px to get the real value.

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

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7a708f82559..c1d8de016081 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -520,6 +520,19 @@ char *number(char *buf, char *end, unsigned long long num,
 	return buf;
 }
 
+static const char *check_pointer_access(const void *ptr)
+{
+	unsigned char byte;
+
+	if (!ptr)
+		return "(null)";
+
+	if (probe_kernel_read(&byte, ptr, 1))
+		return "(efault)";
+
+	return 0;
+}
+
 static noinline_for_stack
 char *special_hex_number(char *buf, char *end, unsigned long long num, int size)
 {
@@ -586,9 +599,11 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 {
 	int len = 0;
 	size_t lim = spec.precision;
+	const char *err_msg;
 
-	if ((unsigned long)s < PAGE_SIZE)
-		s = "(null)";
+	err_msg = check_pointer_access(s);
+	if (err_msg)
+		s = err_msg;
 
 	while (lim--) {
 		char c = *s++;
@@ -1848,15 +1863,19 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	      struct printf_spec spec)
 {
 	const int default_width = 2 * sizeof(void *);
+	const char *err_msg = NULL;
+
+	if (*fmt != 'K' && *fmt != 'x')
+		err_msg = check_pointer_access(ptr);
 
-	if (!ptr && *fmt != 'K' && *fmt != 'x') {
+	if (err_msg) {
 		/*
-		 * Print (null) with the same width as a pointer so it makes
-		 * tabular output look nice.
+		 * Print the error message 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(buf, end, "(null)", spec);
+		return string(buf, end, err_msg, spec);
 	}
 
 	switch (*fmt) {
@@ -2571,11 +2590,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] vsprintf: Make "null" pointer dereference more robust

[Andy Shevchenko]

On Wed, 2018-03-07 at 16:52 +0100, Petr Mladek wrote:
> On Tue 2018-03-06 11:56:25, Andy Shevchenko wrote:

> Anyway, I have discussed this with my colleagues. Different people had
> different opinions. But I liked the following.

I discussed as well, and...

> We already prevent crash when derefencing some obviously broken
> pointers. 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).

> In general, we should do our best to get useful message from printk().
> This patch tries to find a wide range of invalid strings using
> probe_kernel_read(). Also it makes the handling unified. We print:

...they are considering not crashing is a bad idea from debugging point
of view.

So, what is can be done is to:
 - print "(null)" only for null pointers
 - print error codes for IS_ERR() part
 - crash on everything else

which partially what does my patch 1.

> Note that we could not print the exact pointer value from security
> reasons.

If it's invalid we need to fix the code, not to hide a problem, right?

> Developers need print the pointer using %px to get the real value.

But how developer will know (w/o traceback) where to look for?

> +	if (probe_kernel_read(&byte, ptr, 1))
> +		return "(efault)";

There is couple of flaws here:

 - If we asked to print 0 bytes of the value of pointer or something
from extension (%*ph, %*pE, etc), we don't know if pointer valid or not,
because we are going to print nothing. So, for now there is a
ZERO_OR_NULL_PTR() check for them, but in reality I wouldn't know what
the best to do in such case. So, the question is what we would like to
know more: the pointer is invalid, or the spec.width is 0 and caller
doesn't care in this case?

 - For IS_ERR() case it might be better to print an actual value of err,
(4 chars + parens + 2 chars left, like (e:dddd) or similar.
Unfortunately it doesn't scale good (ffff is a last error value we may
print). So, perhaps printing as %p in this case is a good enough and
scalable.

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

Re: [PATCH] vsprintf: Make "null" pointer dereference more robust

[Linus Torvalds]

On Wed, Mar 7, 2018 at 7:52 AM, Petr Mladek <pmladek@suse.com> wrote:
> If we are changing things, let's do it properly. The range
> (-PAGE_SIZE,+PAGE_SIZE) is just a small subset of invalid pointers.
> Let's try to catch more of them by reading one byte using
> probe_kernel_read(). It would return -FAULT if we are not able
> to read the address but it would not crash.
>
> Then we clearly need a new message when dereferencing invalid
> poitners that are not pure NULL. I propose (efault).

NO! Absolutely f*cking NOT!

"probe_kernel_read()" is really complicated. It takes a *fault* for chrissake!

Guess what happens now to any crash report if it uses %p and there is
anything wrong with the VM?

Yes, yes, it disables page faults, but that only means that we won't
go all the way into the generic VM. We'll still take the fauly, still
do vmalloc fault filling, still do a *lot* of potentially really
complicated things.

Guys, stop this idiocy. printk() needs to be *simple* and *reliable*, not fancy.

Plus, you just made %p be an excellent leak of some very sensitive
information, like "where is the kernel mapped" etc.

So not only did you make it fundamentally more complex and fragile,
you actually made it more of a potential security issue too.

> Below is my RFC patch.

NAK NAK NAK.

Seriously. Stop this crap. Get over yourself guys.

This whole NULL discussion has been one huge pile of unbelievable *SHIT*.

The first few bytes in the address space are special, because NULL
pointers are often offset by structure offsets.  They are special
because NULL is special.

THEY ARE NOT SPECIAL BECAUSE IT CAUSES FAULTS.

The top "small negative numbers" are special, because the kernel
extensively uses the ERR_PTR model, and that is how we encode it.

THEY ARE NOT SPECIAL BECAUSE THEY CAUSE FAULTS.

Thinking that "this address causes faults is special" is moronic.

Actually testing - in something as crore and critical as printk -
whether a fault happens is beyond the pale.

                   Linus

Re: [PATCH] vsprintf: Make "null" pointer dereference more robust

[Petr Mladek]

On Wed 2018-03-07 10:34:17, Linus Torvalds wrote:
> On Wed, Mar 7, 2018 at 7:52 AM, Petr Mladek <pmladek@suse.com> wrote:
> > If we are changing things, let's do it properly. The range
> > (-PAGE_SIZE,+PAGE_SIZE) is just a small subset of invalid pointers.
> > Let's try to catch more of them by reading one byte using
> > probe_kernel_read(). It would return -FAULT if we are not able
> > to read the address but it would not crash.
> >
> > Then we clearly need a new message when dereferencing invalid
> > poitners that are not pure NULL. I propose (efault).
> 
> "probe_kernel_read()" is really complicated. It takes a *fault* for chrissake!
> 
> Guess what happens now to any crash report if it uses %p and there is
> anything wrong with the VM?

This patch does _not_ affect plain %p, %px, and %pK!

It affects %s and %p* modifiers that need to read data from the
given address.


> Yes, yes, it disables page faults, but that only means that we won't
> go all the way into the generic VM. We'll still take the fauly, still
> do vmalloc fault filling, still do a *lot* of potentially really
> complicated things.

But the faulty way would happen anyway when vsnprintf() tried to
access the data at the given address.


> Guys, stop this idiocy. printk() needs to be *simple* and *reliable*, not fancy.

This patch is primary about printk() reliability. We want a message
instead of a silent crash.

I am open for better ideas how to get the fault-related messages out
when logbug_lock is taken. At the moment we have them in printk_safe
per-CPU buffer. The only chance to see them is crashdump or
printk_safe_flush_on_panic() called in single CPU mode.


> Plus, you just made %p be an excellent leak of some very sensitive
> information, like "where is the kernel mapped" etc.

We might call panic() after lockbuf_lock is released. This would help
to see the message and prevent crash-free hunting for kernel location.

Best Regards,
Petr

Re: [PATCH] vsprintf: Make "null" pointer dereference more robust

[Linus Torvalds]

On Thu, Mar 8, 2018 at 6:18 AM, Petr Mladek <pmladek@suse.com> wrote:
> On Wed 2018-03-07 10:34:17, Linus Torvalds wrote:
>>
>> Guess what happens now to any crash report if it uses %p and there is
>> anything wrong with the VM?
>
> This patch does _not_ affect plain %p, %px, and %pK!

Umm. Look again. It _does_ affect plain %p.

You're correct that it doesn't affect %px and %pK, since those never
printed out (null) in the first place.

> It affects %s and %p* modifiers that need to read data from the
> given address.

_If_ that was what the patch did, it would be fine.

But it isn't.

It not only affects %p, but it also affects %pS and friends (sSfFB),
that do not access the location (well, on some architectures those
might, to dereference a function descriptor, but then they will check
the address range).

So that patch really is completely broken for the reasons I outlined.

Now, if it was fixed to what you apparently *intended* to do, then
that would be ok.

            Linus

Re: [PATCH] vsprintf: Make "null" pointer dereference more robust

[Linus Torvalds]

On Thu, Mar 8, 2018 at 8:45 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Umm. Look again. It _does_ affect plain %p.
>
> You're correct that it doesn't affect %px and %pK, since those never
> printed out (null) in the first place.
>
> It not only affects %p, but it also affects %pS and friends (sSfFB),

Looking around at the x86 panic thing, %p doesn't matter that much,
but %p[sSfFB] really do.

We use %pS/%pB to print out the instruction pointer. And a fault might
be due to the instruction pointer being bad.

And then we very much need to see the value, which the current
%pS-and-friends falls back to.

So printing <efault> would actually be horrible, in addition to the
extra page fault being wrong. In fact, _only_ NULL itself needs to be
printed as (null), because we'd care if it's 0 or 8 or something.

The other ones? The ones that would actually fault (%pI and friends)
would not matter.

The hex dumping one _might_ actually be useful if it got a buffer with
'probe_kernel_read()' and stopped half-way on problems.   Maybe. The
others I can't imagine really care. efault or hex address.

             Linus

Re: [PATCH] vsprintf: Make "null" pointer dereference more robust

[Petr Mladek]

On Thu 2018-03-08 09:26:11, Linus Torvalds wrote:
> On Thu, Mar 8, 2018 at 8:45 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Umm. Look again. It _does_ affect plain %p.
> >
> > You're correct that it doesn't affect %px and %pK, since those never
> > printed out (null) in the first place.
> >
> > It not only affects %p, but it also affects %pS and friends (sSfFB),
> 
> Looking around at the x86 panic thing, %p doesn't matter that much,
> but %p[sSfFB] really do.
> 
> We use %pS/%pB to print out the instruction pointer. And a fault might
> be due to the instruction pointer being bad.
> 
> And then we very much need to see the value, which the current
> %pS-and-friends falls back to.
> 
> So printing <efault> would actually be horrible, in addition to the
> extra page fault being wrong. In fact, _only_ NULL itself needs to be
> printed as (null), because we'd care if it's 0 or 8 or something.
> 
> The other ones? The ones that would actually fault (%pI and friends)
> would not matter.

This all makes perfect sense. And I actually intended to do it this
way. Unfortunately, I sent the first RFC too fast without updating
the check for %p* specifiers. I am sorry for the confusion.


> The hex dumping one _might_ actually be useful if it got a buffer with
> 'probe_kernel_read()' and stopped half-way on problems.   Maybe. The
> others I can't imagine really care. efault or hex address.

I will try to keep it simple and check only the 1st byte for now.
It should prevent most of the possible silent crashes.

Here is the 2nd attempt. It never calls probe_kernel_read() when
the data need not be accessed:


>From 1c7123fbb8d098822b8f59c032e1ac79dbb6bf1a Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@suse.com>
Date: Wed, 7 Mar 2018 16:27:24 +0100
Subject: [PATCH v2] vsprintf: Prevent crash when dereferencing invalid pointers

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.

In general, we want to get a message from printk() than a silent crash.
This patch adds a check using probe_kernel_read().

The check is used _only_ in situations where we would really crash. This is
why this patch adds a white list of %p* specifiers that do the dereference.
Note that "%p" might be followed by any character. Only few of them are
valid specifiers and only few specifiers need to access the data.

Also it makes the handling unified. We print:

   + (null) when pure NULL pointer is dereferenced
   + (efault) when an invalid address is dereferenced
   + pointer address otherwise

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>
---
 lib/vsprintf.c | 41 ++++++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7a708f82559..36fa8a15c169 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -520,6 +520,19 @@ char *number(char *buf, char *end, unsigned long long num,
 	return buf;
 }
 
+static const char *check_pointer_access(const void *ptr)
+{
+	unsigned char byte;
+
+	if (!ptr)
+		return "(null)";
+
+	if (probe_kernel_read(&byte, ptr, 1))
+		return "(efault)";
+
+	return 0;
+}
+
 static noinline_for_stack
 char *special_hex_number(char *buf, char *end, unsigned long long num, int size)
 {
@@ -586,9 +599,11 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 {
 	int len = 0;
 	size_t lim = spec.precision;
+	const char *err_msg;
 
-	if ((unsigned long)s < PAGE_SIZE)
-		s = "(null)";
+	err_msg = check_pointer_access(s);
+	if (err_msg)
+		s = err_msg;
 
 	while (lim--) {
 		char c = *s++;
@@ -1847,16 +1862,22 @@ static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	      struct printf_spec spec)
 {
+	static const char data_access_fmt[] = "RrhbMmIiEUVNadCDgGO";
 	const int default_width = 2 * sizeof(void *);
+	const char *err_msg = NULL;
+
+	/* Prevent silent crash when this is called under logbuf_lock. */
+	if (strchr(data_access_fmt, *fmt) != NULL)
+		err_msg = check_pointer_access(ptr);
 
-	if (!ptr && *fmt != 'K' && *fmt != 'x') {
+	if (err_msg) {
 		/*
-		 * Print (null) with the same width as a pointer so it makes
-		 * tabular output look nice.
+		 * Print the error message 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(buf, end, "(null)", spec);
+		return string(buf, end, err_msg, spec);
 	}
 
 	switch (*fmt) {
@@ -2571,11 +2592,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] vsprintf: Make "null" pointer dereference more robust

[Linus Torvalds]

On Fri, Mar 9, 2018 at 7:01 AM, Petr Mladek <pmladek@suse.com> wrote:
> Also it makes the handling unified. We print:
>
>    + (null) when pure NULL pointer is dereferenced
>    + (efault) when an invalid address is dereferenced
>    + pointer address otherwise

This is still fundamentally completely wrong.

It never prints "pointer address", and if it were to do that, it would be wrong.

It should never ever trigger for an address operation, only for the
"we will get _data_ from the ponter".

The strchr thing is also completely broken, and in a very subtle way.
"strchr(string, 0)" is special, and the Open Group states

  "The terminating null byte is considered to be part of the string"

so a NUL character will *always* return success, which is actually
completely wrong for this case, because now it does that whole crazy
<efault> thing for %p that it shouldn't do.

Not that I actually verified that our strchr() follows the actual
rules anyway - I personally consider "strchr(string, 0)" to not really
be "special", but be a bug. Either way, the comment is wrong, but the
code is also wrong.

                 Linus

[PATCH v3] 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.

In general, we want to get a message from printk() than a silent crash.
This patch adds a check using probe_kernel_read().

The check is used _only_ in situations where we would really crash. This is
why this patch adds a white list of %p* specifiers that need to read data
from the pointer. Note that "%p" might be followed by any character. Only
few of them are valid specifiers and only few specifiers need to access
the data.

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>
---
Changes against v2:

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

Changes against v1:

	+ do not check access for plain %p
	+ more clear commit message

 Documentation/core-api/printk-formats.rst |  7 ++++++
 lib/test_printf.c                         | 39 +++++++++++++++++++++++------
 lib/vsprintf.c                            | 41 ++++++++++++++++++++++++-------
 3 files changed, 70 insertions(+), 17 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 934559b3c130..d3bbca732ed6 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, you might get two special
+values::
+
+	(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 71ebfa43ad05..61c05a352d79 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -207,14 +207,15 @@ test_string(void)
 #define PTR ((void *)0xffff0123456789ab)
 #define PTR_STR "ffff0123456789ab"
 #define ZEROS "00000000"	/* hex 32 zero bits */
+#define SPACE "        "	/* 32 space 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 +228,8 @@ plain_format(void)
 #define PTR_WIDTH 8
 #define PTR ((void *)0x456789ab)
 #define PTR_STR "456789ab"
+#define ZEROS ""
+#define SPACE ""
 
 static int __init
 plain_format(void)
@@ -238,12 +241,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 +259,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 +278,24 @@ plain(void)
 }
 
 static void __init
+null_pointer(void)
+{
+	plain(NULL);
+	test(ZEROS "00000000", "%px", NULL);
+	test(SPACE "  (null)", "%pC", NULL);
+}
+
+#define PTR_INVALID ((void *)0x000000ab)
+
+static void __init
+invalid_pointer(void)
+{
+	plain(PTR_INVALID);
+	test(ZEROS "000000ab", "%px", PTR_INVALID);
+	test(SPACE "(efault)", "%pC", PTR_INVALID);
+}
+
+static void __init
 symbol_ptr(void)
 {
 }
@@ -497,7 +518,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 d7a708f82559..54b985143e07 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -520,6 +520,19 @@ char *number(char *buf, char *end, unsigned long long num,
 	return buf;
 }
 
+static const char *check_pointer_access(const void *ptr)
+{
+	unsigned char byte;
+
+	if (!ptr)
+		return "(null)";
+
+	if (probe_kernel_read(&byte, ptr, 1))
+		return "(efault)";
+
+	return 0;
+}
+
 static noinline_for_stack
 char *special_hex_number(char *buf, char *end, unsigned long long num, int size)
 {
@@ -586,9 +599,11 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 {
 	int len = 0;
 	size_t lim = spec.precision;
+	const char *err_msg;
 
-	if ((unsigned long)s < PAGE_SIZE)
-		s = "(null)";
+	err_msg = check_pointer_access(s);
+	if (err_msg)
+		s = err_msg;
 
 	while (lim--) {
 		char c = *s++;
@@ -1847,16 +1862,22 @@ static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	      struct printf_spec spec)
 {
+	static const char data_access_fmt[] = "RrhbMmIiEUVNadCDgGO";
 	const int default_width = 2 * sizeof(void *);
+	const char *err_msg = NULL;
+
+	/* Prevent silent crash when this is called under logbuf_lock. */
+	if (*fmt && strchr(data_access_fmt, *fmt) != NULL)
+		err_msg = check_pointer_access(ptr);
 
-	if (!ptr && *fmt != 'K' && *fmt != 'x') {
+	if (err_msg) {
 		/*
-		 * Print (null) with the same width as a pointer so it makes
-		 * tabular output look nice.
+		 * Print the error message 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(buf, end, "(null)", spec);
+		return string(buf, end, err_msg, spec);
 	}
 
 	switch (*fmt) {
@@ -2571,11 +2592,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 v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Rasmus Villemoes]

On 2018-03-14 15:09, Petr Mladek wrote:

> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 71ebfa43ad05..61c05a352d79 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -207,14 +207,15 @@ test_string(void)
> @@ -256,18 +259,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 +278,24 @@ plain(void)
>  }

Thanks for adding tests. Please increment total_tests for each test case.

>  static void __init
> +null_pointer(void)
> +{
> +	plain(NULL);
> +	test(ZEROS "00000000", "%px", NULL);
> +	test(SPACE "  (null)", "%pC", NULL);
> +}

Hm, %pC depends on CONFIG_CLOCK, not that we ever reach clock() with
these invalid pointers, but perhaps clearer to choose one whose
behaviour is not config-dependent.

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index d7a708f82559..54b985143e07 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -520,6 +520,19 @@ char *number(char *buf, char *end, unsigned long long num,
>  	return buf;
>  }
>  
> +static const char *check_pointer_access(const void *ptr)
> +{
> +	unsigned char byte;
> +
> +	if (!ptr)
> +		return "(null)";
> +
> +	if (probe_kernel_read(&byte, ptr, 1))
> +		return "(efault)";
> +
> +	return 0;
> +}

return NULL;

> +
>  static noinline_for_stack
>  char *special_hex_number(char *buf, char *end, unsigned long long num, int size)
>  {
> @@ -586,9 +599,11 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
>  {
>  	int len = 0;
>  	size_t lim = spec.precision;
> +	const char *err_msg;
>  
> -	if ((unsigned long)s < PAGE_SIZE)
> -		s = "(null)";
> +	err_msg = check_pointer_access(s);
> +	if (err_msg)
> +		s = err_msg;
>  
>  	while (lim--) {
>  		char c = *s++;
> @@ -1847,16 +1862,22 @@ static noinline_for_stack
>  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  	      struct printf_spec spec)
>  {
> +	static const char data_access_fmt[] = "RrhbMmIiEUVNadCDgGO";
>  	const int default_width = 2 * sizeof(void *);
> +	const char *err_msg = NULL;
> +
> +	/* Prevent silent crash when this is called under logbuf_lock. */
> +	if (*fmt && strchr(data_access_fmt, *fmt) != NULL)
> +		err_msg = check_pointer_access(ptr);

Can we please make this more readable and maintainable in case other
fancy %p* stuff is added. The extensions which do dereference ptr
outnumber those which don't, and a new one is also likely to fall in
that category. Something like

static bool extension_dereferences_ptr(const char *fmt)
{
  switch(*fmt) {
  case 'x':
  case 'K':
  case 'F':
  case 'f':
  case 'S':
  case 's':
  case 'B':
    return false;
  default:
    return isalnum(*fmt);
  }
}

which you could spell isalnum(*fmt) && !strchr("xKFfSsB", *fmt), but
having a switch is a closer match to the subsequent dispatching (and
would allow a more fine-grained approach should the answer depend on
fmt[1] as well).

Question: probe_kernel_read seems to allow (mapped) userspace addresses.
Is that really what we want? Sure, some %p* just format the pointed-to
bytes directly (as an IP address or raw hex dump or whatnot), but some
(e.g. %pD, and %pV could be particularly fun) do another dereference.
I'm not saying it would be easy for an attacker to get a userpointer
passed to %pV, but there's a lot of places that end up calling vsnprintf
(not just printk and friends). Isn't there some cheap address comparison
one can do to rule that out completely?

Rasmus

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Petr Mladek]

On Wed 2018-03-14 23:12:36, Rasmus Villemoes wrote:
> On 2018-03-14 15:09, Petr Mladek wrote:
> 
> > diff --git a/lib/test_printf.c b/lib/test_printf.c
> > index 71ebfa43ad05..61c05a352d79 100644
> > --- a/lib/test_printf.c
> > +++ b/lib/test_printf.c
> > @@ -207,14 +207,15 @@ test_string(void)
> > @@ -256,18 +259,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 +278,24 @@ plain(void)
> >  }
> 
> Thanks for adding tests. Please increment total_tests for each test case.

Good point! Will do in v4.

> >  static void __init
> > +null_pointer(void)
> > +{
> > +	plain(NULL);
> > +	test(ZEROS "00000000", "%px", NULL);
> > +	test(SPACE "  (null)", "%pC", NULL);
> > +}
> 
> Hm, %pC depends on CONFIG_CLOCK, not that we ever reach clock() with
> these invalid pointers, but perhaps clearer to choose one whose
> behaviour is not config-dependent.

It is not a real problem, but I could select another "random" one for v4.

BTW: I chosen "%pC" because clock was a basic word that anyone could
understand ;-) Otherwise, "%pC" is always handled and the purpose of the
specifier is to access the data. IMHO, it does not make sense to make
it too complicated and avoid the access check when CONFIG_CLOCK is not
enabled.

The only specifier that is optionally handled is "%pg". And I think
that it is wrong. It is strange when 'g' is sometimes handled as
specifier and sometimes part of the output string. Well, it is
not a big deal. Who would want to print something like "hello, %pgroup"?

Anyway, you made me to look at clock() more closely. The
implementation is questionable:

  + it always printks "(null)" when CONFIG_HAVE_CLK is not enabled.
    This might hide an error.

  + it prints the address for plain "%pC" and "%pCn" when
    CONFIG_COMMON_CLK is not enabled. We should rather print
    the pointer hash or some error string.

Andy Shevchenko plans to do some clean up on top of this patch.
We could sort it there.


> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index d7a708f82559..54b985143e07 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -520,6 +520,19 @@ char *number(char *buf, char *end, unsigned long long num,
> >  	return buf;
> >  }
> >  
> > +static const char *check_pointer_access(const void *ptr)
> > +{
> > +	unsigned char byte;
> > +
> > +	if (!ptr)
> > +		return "(null)";
> > +
> > +	if (probe_kernel_read(&byte, ptr, 1))
> > +		return "(efault)";
> > +
> > +	return 0;
> > +}
> 
> return NULL;

Good catch! This is an artifact from an older variant where it returned int.

> > +
> >  static noinline_for_stack
> >  char *special_hex_number(char *buf, char *end, unsigned long long num, int size)
> >  {
> > @@ -1847,16 +1862,22 @@ static noinline_for_stack
> >  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> >  	      struct printf_spec spec)
> >  {
> > +	static const char data_access_fmt[] = "RrhbM mIiEU VNadC DgGO";
> >  	const int default_width = 2 * sizeof(void *);
> > +	const char *err_msg = NULL;
> > +
> > +	/* Prevent silent crash when this is called under logbuf_lock. */
> > +	if (*fmt && strchr(data_access_fmt, *fmt) != NULL)
> > +		err_msg = check_pointer_access(ptr);
> 
> Can we please make this more readable and maintainable in case other
> fancy %p* stuff is added. The extensions which do dereference ptr
> outnumber those which don't, and a new one is also likely to fall in
> that category. Something like
> 
> static bool extension_dereferences_ptr(const char *fmt)
> {
>   switch(*fmt) {
>   case 'x':
>   case 'K':
>   case 'F':
>   case 'f':
>   case 'S':
>   case 's':
>   case 'B':
>     return false;
>   default:
>     return isalnum(*fmt);
>   }
> }
> 
> which you could spell isalnum(*fmt) && !strchr("xKFfSsB", *fmt), but
> having a switch is a closer match to the subsequent dispatching (and
> would allow a more fine-grained approach should the answer depend on
> fmt[1] as well).

The thing is that *fmt points to the original format. It might be
something like: "print%ponscreen". It will be handled as "%p" because
'o' is not valid specifier. But your function would return true.

To be honest, I cannot imagine reasonable real-life example where
the above implementation might fail. On the other hand, there are
currently 19 specifiers where we do the dereference. It means the
your simplified approach has 36 false positives. People might be
creative, ... So I prefer to avoid false positives.


> Question: probe_kernel_read seems to allow (mapped) userspace addresses.
> Is that really what we want? Sure, some %p* just format the pointed-to
> bytes directly (as an IP address or raw hex dump or whatnot), but some
> (e.g. %pD, and %pV could be particularly fun) do another dereference.
> I'm not saying it would be easy for an attacker to get a userpointer
> passed to %pV, but there's a lot of places that end up calling vsnprintf
> (not just printk and friends). Isn't there some cheap address comparison
> one can do to rule that out completely?

Good point. The following should do the job:

static const char *check_pointer_access(const void *ptr)
{
	char c;

	if (!ptr)
		return "(null)";

	if ((unsigned long)ptr < TASK_SIZE || probe_kernel_address(ptr, c))
		return "(efault)";

	return 0;
}

Best Regards,
Petr

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Steven Rostedt]

On Thu, 15 Mar 2018 16:07:54 +0100
Petr Mladek <pmladek@suse.com> wrote:

> Good point. The following should do the job:
> 
> static const char *check_pointer_access(const void *ptr)
> {
> 	char c;
> 
> 	if (!ptr)
> 		return "(null)";
> 
> 	if ((unsigned long)ptr < TASK_SIZE || probe_kernel_address(ptr, c))

Please don't.

-- Steve

> 		return "(efault)";
> 
> 	return 0;
> }

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Steven Rostedt]

On Wed, 14 Mar 2018 23:12:36 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> Question: probe_kernel_read seems to allow (mapped) userspace addresses.
> Is that really what we want? Sure, some %p* just format the pointed-to
> bytes directly (as an IP address or raw hex dump or whatnot), but some
> (e.g. %pD, and %pV could be particularly fun) do another dereference.
> I'm not saying it would be easy for an attacker to get a userpointer
> passed to %pV, but there's a lot of places that end up calling vsnprintf
> (not just printk and friends). Isn't there some cheap address comparison
> one can do to rule that out completely?

We allow it today right? Why should we stop it now. For debugging I
will sometimes add printk()s to write out content in userspace. Since
the kernel maps all memory in its own space, there's nothing we are
protecting by not letting the kernel read userspace but be OK letting
it read anything in kernel space.

-- Steve

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Sergey Senozhatsky]

On (03/14/18 15:09), Petr Mladek wrote:
> Changes against v2:
> 
> 	+ fix handling with strchr(string, '\0'); happens with
> 	  %p at the very end of the string
> 	+ even more clear commit message
> 	+ update Documentation/core-api/printk-formats.rst
> 	+ add check into lib/test_printf.c
> 
> Changes against v1:
> 
> 	+ do not check access for plain %p
> 	+ more clear commit message

Did I miss v1 and v2?

	-ss

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Sergey Senozhatsky]

On (03/14/18 15:09), Petr Mladek wrote:
[..]
> +static const char *check_pointer_access(const void *ptr)
> +{
> +	unsigned char byte;
> +
> +	if (!ptr)
> +		return "(null)";
> +
> +	if (probe_kernel_read(&byte, ptr, 1))
                                        ^^^^^
Why one byte? 			     sizeof(ptr)?

[..]
>  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  	      struct printf_spec spec)
>  {
> +	static const char data_access_fmt[] = "RrhbMmIiEUVNadCDgGO";
>  	const int default_width = 2 * sizeof(void *);
> +	const char *err_msg = NULL;
> +
> +	/* Prevent silent crash when this is called under logbuf_lock. */
> +	if (*fmt && strchr(data_access_fmt, *fmt) != NULL)
> +		err_msg = check_pointer_access(ptr);

Agree with Rasmus, I think switch() is easier.

	-ss

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Sergey Senozhatsky]

On (03/15/18 16:58), Sergey Senozhatsky wrote:
> On (03/14/18 15:09), Petr Mladek wrote:
> [..]
> > +static const char *check_pointer_access(const void *ptr)
> > +{
> > +	unsigned char byte;
> > +
> > +	if (!ptr)
> > +		return "(null)";
> > +
> > +	if (probe_kernel_read(&byte, ptr, 1))
>                                         ^^^^^
> Why one byte? 			     sizeof(ptr)?

I think there is a shorter version - probe_kernel_address(),
which, seems, was designed for this particular kind of stuff.

	void *p;

	if (probe_kernel_address(ptr, p))
		....

	-ss

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Steven Rostedt]

On Thu, 15 Mar 2018 17:03:09 +0900
Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:

> On (03/15/18 16:58), Sergey Senozhatsky wrote:
> > On (03/14/18 15:09), Petr Mladek wrote:
> > [..]  
> > > +static const char *check_pointer_access(const void *ptr)
> > > +{
> > > +	unsigned char byte;
> > > +
> > > +	if (!ptr)
> > > +		return "(null)";
> > > +
> > > +	if (probe_kernel_read(&byte, ptr, 1))  
> >                                         ^^^^^
> > Why one byte? 			     sizeof(ptr)?  
> 
> I think there is a shorter version - probe_kernel_address(),
> which, seems, was designed for this particular kind of stuff.
> 
> 	void *p;
> 
> 	if (probe_kernel_address(ptr, p))
> 		....
>

Agreed.

-- Steve

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Sergey Senozhatsky]

On (03/15/18 13:01), Steven Rostedt wrote:
> > > > +static const char *check_pointer_access(const void *ptr)
> > > > +{
> > > > +	unsigned char byte;
> > > > +
> > > > +	if (!ptr)
> > > > +		return "(null)";
> > > > +
> > > > +	if (probe_kernel_read(&byte, ptr, 1))  
> > >                                         ^^^^^
> > > Why one byte? 			     sizeof(ptr)?  
> > 
> > I think there is a shorter version - probe_kernel_address(),
> > which, seems, was designed for this particular kind of stuff.
> > 
> > 	void *p;
> > 
> > 	if (probe_kernel_address(ptr, p))
> > 		....
> >
> 
> Agreed.

Hm, may be sizeof(ptr) still won't suffice. It would be great if we
could always look at spec.field_width, which can be up to 2 * sizeof(void *),
and then just probe_kernel_read(spec.field_width). E.g., %b/%bl prints out a
bitmap, accessing max_t(int, spec.field_width, 0) bits, which is good. But,
for instance, %U (uuid printout) doesn't look at spec.field_width, and reads
in 16 bytes from the given memory address. Then we have ipv4/ipv6, mac, etc.
So I think that checking just 1 byte or sizeof(ptr) is not really enough if
we want to fix vsprintf. What do you think?

	-ss

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Linus Torvalds]

On Thu, Mar 15, 2018 at 6:18 PM, Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> Hm, may be sizeof(ptr) still won't suffice. It would be great if we
> could always look at spec.field_width, which can be up to 2 * sizeof(void *),
> and then just probe_kernel_read(spec.field_width). E.g., %b/%bl prints out a
> bitmap, accessing max_t(int, spec.field_width, 0) bits, which is good. But,
> for instance, %U (uuid printout) doesn't look at spec.field_width, and reads
> in 16 bytes from the given memory address. Then we have ipv4/ipv6, mac, etc.
> So I think that checking just 1 byte or sizeof(ptr) is not really enough if
> we want to fix vsprintf. What do you think?

Honestly, I think it would be better to move the whole logic to the
functions that actually do the printout.

Then you can do it right, and you don't need to have the strchr() either.

There really isn't any commonality between the different versions.
field_width is meaningless, since it's about the size of the _printed_
field, not the size in memory.

Would it be a few more lines? Yes. But it would also clarify the code
and get all the cases right. Look at hex_string() for example, and
imagine fetching a byte at a time and just getting the corner cases
automatically right.

And if you don't care, just do a

       const char *err = check_pointer_access(ptr);
       if (err)
                return string(buf, end, err, spec);

at the top of each function that dereferences things. Some of them
already have the equivalent of that (the afore-mentioned hex_string:

        if (ZERO_OR_NULL_PTR(addr))
                /* NULL pointer */
                return string(buf, end, NULL, spec);

and it certainly is neither a lot of extra code, nor subtle or complex
to just do that (except using "err = check_pointer_access(ptr);").

               Linus

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Sergey Senozhatsky]

On (03/15/18 18:35), Linus Torvalds wrote:
> On Thu, Mar 15, 2018 at 6:18 PM, Sergey Senozhatsky
> <sergey.senozhatsky.work@gmail.com> wrote:
> >
> > Hm, may be sizeof(ptr) still won't suffice. It would be great if we
> > could always look at spec.field_width, which can be up to 2 * sizeof(void *),
> > and then just probe_kernel_read(spec.field_width). E.g., %b/%bl prints out a
> > bitmap, accessing max_t(int, spec.field_width, 0) bits, which is good. But,
> > for instance, %U (uuid printout) doesn't look at spec.field_width, and reads
> > in 16 bytes from the given memory address. Then we have ipv4/ipv6, mac, etc.
> > So I think that checking just 1 byte or sizeof(ptr) is not really enough if
> > we want to fix vsprintf. What do you think?
> 
> Honestly, I think it would be better to move the whole logic to the
> functions that actually do the printout.
> 
> Then you can do it right, and you don't need to have the strchr() either.
>
> There really isn't any commonality between the different versions.
> field_width is meaningless, since it's about the size of the _printed_
> field, not the size in memory.

Agreed!

> Would it be a few more lines? Yes. But it would also clarify the code
> and get all the cases right. Look at hex_string() for example, and
> imagine fetching a byte at a time and just getting the corner cases
> automatically right.

So, basically, what I tried to say - any byte past the first sizeof(ptr)
bytes or past the first byte that we check_access() can cause problems,
which this patch is trying to address. As an example, FORMAT_TYPE_STR
case

	printk("%.*s\n", p->buf)
	 vsnprintf()
	  string()

Where ->buf is a _nearly always_ properly nul terminated char buf[128]
array in struct foo. So moving that check_access() to every function that
does printout sounds good to me, as well as checking every byte we access
[assuming that we want to cure vsprintf], not just the first one or the
first sizeof(ptr) bytes.

	-ss

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Petr Mladek]

On Fri 2018-03-16 14:53:46, Sergey Senozhatsky wrote:
> On (03/15/18 18:35), Linus Torvalds wrote:
> > On Thu, Mar 15, 2018 at 6:18 PM, Sergey Senozhatsky
> > <sergey.senozhatsky.work@gmail.com> wrote:
> > >
> > > Hm, may be sizeof(ptr) still won't suffice. It would be great if we
> > > could always look at spec.field_width, which can be up to 2 * sizeof(void *),
> > > and then just probe_kernel_read(spec.field_width). E.g., %b/%bl prints out a
> > > bitmap, accessing max_t(int, spec.field_width, 0) bits, which is good. But,
> > > for instance, %U (uuid printout) doesn't look at spec.field_width, and reads
> > > in 16 bytes from the given memory address. Then we have ipv4/ipv6, mac, etc.
> > > So I think that checking just 1 byte or sizeof(ptr) is not really enough if
> > > we want to fix vsprintf. What do you think?
> > 
> > Honestly, I think it would be better to move the whole logic to the
> > functions that actually do the printout.
>
> > Would it be a few more lines? Yes. But it would also clarify the code
> > and get all the cases right. Look at hex_string() for example, and
> > imagine fetching a byte at a time and just getting the corner cases
> > automatically right.

I am learning every day. I like this idea and am happy that
it is acceptable to others.


> So, basically, what I tried to say - any byte past the first sizeof(ptr)
> bytes or past the first byte that we check_access() can cause problems,
> which this patch is trying to address. As an example, FORMAT_TYPE_STR
> case
> 
> 	printk("%.*s\n", p->buf)
> 	 vsnprintf()
> 	  string()
> 
> Where ->buf is a _nearly always_ properly nul terminated char buf[128]
> array in struct foo. So moving that check_access() to every function that
> does printout sounds good to me, as well as checking every byte we access
> [assuming that we want to cure vsprintf], not just the first one or the
> first sizeof(ptr) bytes.

I am not sure if it is worth it. I think that we would catch 99% of
problems by checking the first byte.

This patch was motivated by a code clean up rather than bug reports.
The original patch removed two more strict checks and kept only
the check for pure NULL. I suggested that it was the wrong way to
go...

I do not want to go suddenly to the other extreme. I suggest to start
with simple check for the first byte and see how often it helps
in the real life. We could always extend it later.

Best Regards,
Petr

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Steven Rostedt]

On Fri, 16 Mar 2018 09:55:56 +0100
Petr Mladek <pmladek@suse.com> wrote:

> I am not sure if it is worth it. I think that we would catch 99% of
> problems by checking the first byte.

Then it should be commented as such. Something like:

 /*
  * This is not a fool-proof test. 99.9% 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.
  */

> 
> This patch was motivated by a code clean up rather than bug reports.
> The original patch removed two more strict checks and kept only
> the check for pure NULL. I suggested that it was the wrong way to
> go...
> 
> I do not want to go suddenly to the other extreme. I suggest to start
> with simple check for the first byte and see how often it helps
> in the real life. We could always extend it later.

Fair enough. If this is just code clean up, then sure, we don't need to
cover all cases. But it should definitely be commented about why this
is added, and if in the future we really do want this to be more
robust, then we can extend it.

-- Steve

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Sergey Senozhatsky]

On (03/16/18 09:55), Petr Mladek wrote:
[..]
> I am not sure if it is worth it. I think that we would catch 99% of
> problems by checking the first byte.
> 
> This patch was motivated by a code clean up rather than bug reports.

OK. Then I think we really need this "the patch is just good enough" line
in the commit message and a big comment in the source code.

Another idea (just an idea) - for some pointers we know the address range
we are going to access and can check the first and the last byte. E.g. for
UUID it's check_access(ptr) and check_access(ptr + len), and so on. Won't
work for string() in general case, tho.

	-ss

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Andy Shevchenko]

On Thu, 2018-03-15 at 16:58 +0900, Sergey Senozhatsky wrote:
> On (03/14/18 15:09), Petr Mladek wrote:
> 


> >  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> >  	      struct printf_spec spec)
> >  {
> > +	static const char data_access_fmt[] =
> > "RrhbMmIiEUVNadCDgGO";
> >  	const int default_width = 2 * sizeof(void *);
> > +	const char *err_msg = NULL;
> > +
> > +	/* Prevent silent crash when this is called under
> > logbuf_lock. */
> > +	if (*fmt && strchr(data_access_fmt, *fmt) != NULL)
> > +		err_msg = check_pointer_access(ptr);
> 
> Agree with Rasmus, I think switch() is easier.
> 

One more to the same.

Though need to add that we also have to append / update some comment to
keep these lists synchronized (in Documentation, around pointer() and
here)

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

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Andy Shevchenko]

On Wed, 2018-03-14 at 15:09 +0100, 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).

I still think that printing a hex value of the error code is much better
than some odd "(efault)".


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

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Petr Mladek]

On Thu 2018-03-15 15:09:03, Andy Shevchenko wrote:
> On Wed, 2018-03-14 at 15:09 +0100, 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).
> 
> I still think that printing a hex value of the error code is much better
> than some odd "(efault)".

Do you mean (err:0e)? Google gives rather confusing answers for this.

I am not super excited about (efault). But it seems to be less
cryptic and the style is more similar to (null).

Best Regards,
Petr

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Andy Shevchenko]

On Thu, 2018-03-15 at 16:26 +0100, Petr Mladek wrote:
> On Thu 2018-03-15 15:09:03, Andy Shevchenko wrote:
> > On Wed, 2018-03-14 at 15:09 +0100, 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).
> > 
> > I still think that printing a hex value of the error code is much
> > better
> > than some odd "(efault)".
> 
> Do you mean (err:0e)? Google gives rather confusing answers for this.

More like "(0xHHHH)" (we have already more than 512 error code numbers.


> I am not super excited about (efault). But it seems to be less
> cryptic and the style is more similar to (null).
> 
> Best Regards,
> Petr

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

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Petr Mladek]

On Fri 2018-03-16 20:19:35, Andy Shevchenko wrote:
> On Thu, 2018-03-15 at 16:26 +0100, Petr Mladek wrote:
> > On Thu 2018-03-15 15:09:03, Andy Shevchenko wrote:
> > > On Wed, 2018-03-14 at 15:09 +0100, 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).
> > > 
> > > I still think that printing a hex value of the error code is much
> > > better
> > > than some odd "(efault)".
> > 
> > Do you mean (err:0e)? Google gives rather confusing answers for this.
> 
> More like "(0xHHHH)" (we have already more than 512 error code numbers.

Hmm, I have never seen the error code in this form. Also google gives
rather confusing results when searching, for example for "(0x000E)".

Best Regards,
Petr

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Andy Shevchenko]

On Thu, 2018-03-29 at 16:53 +0200, Petr Mladek wrote:
> On Fri 2018-03-16 20:19:35, Andy Shevchenko wrote:
> > On Thu, 2018-03-15 at 16:26 +0100, Petr Mladek wrote:
> > > On Thu 2018-03-15 15:09:03, Andy Shevchenko wrote:

> > > > I still think that printing a hex value of the error code is
> > > > much
> > > > better
> > > > than some odd "(efault)".
> > > 
> > > Do you mean (err:0e)? Google gives rather confusing answers for
> > > this.
> > 
> > More like "(0xHHHH)" (we have already more than 512 error code
> > numbers.
> 
> Hmm, I have never seen the error code in this form.

We have limited space to print it and error numbers currently can be up
to 0xfff (4095). So, I have no better idea how to squeeze them while
thinking that "(efault)" is much harder to parse in case of error
pointer.

>  Also google gives
> rather confusing results when searching, for example for "(0x000E)".

It's not primarily for google, though yeah, people would google for
error messages...

Another question is what the format: decimal versus hex for errors.
Maybe just "(-DDDDD)"?

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

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Sergey Senozhatsky]

On (04/02/18 17:15), Andy Shevchenko wrote:
> > 
> > Hmm, I have never seen the error code in this form.
> 
> We have limited space to print it and error numbers currently can be up
> to 0xfff (4095). So, I have no better idea how to squeeze them while
> thinking that "(efault)" is much harder to parse in case of error

'efault' looks to me like a misspelled 'default', for some reason.

	-ss

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Petr Mladek]

On Tue 2018-04-03 10:12:37, Sergey Senozhatsky wrote:
> On (04/02/18 17:15), Andy Shevchenko wrote:
> > > 
> > > Hmm, I have never seen the error code in this form.
> > 
> > We have limited space to print it and error numbers currently can be up
> > to 0xfff (4095). So, I have no better idea how to squeeze them while
> > thinking that "(efault)" is much harder to parse in case of error
> 
> 'efault' looks to me like a misspelled 'default', for some reason.

I wonder if (-efault) would help a bit.

Even better might be (-EFAULT). But then it would be better to use
(NULL). It already was but it was explicitly changed to the lowercase
variant by the commit 0f4f81dce93774a447da3c ("vsprintf: factorize
"(null)" string").

Best Regards,
Petr

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Andy Shevchenko]

On Tue, 2018-04-03 at 13:52 +0200, Petr Mladek wrote:
> On Tue 2018-04-03 10:12:37, Sergey Senozhatsky wrote:
> > On (04/02/18 17:15), Andy Shevchenko wrote:
> > > > 
> > > > Hmm, I have never seen the error code in this form.
> > > 
> > > We have limited space to print it and error numbers currently can
> > > be up
> > > to 0xfff (4095). So, I have no better idea how to squeeze them
> > > while
> > > thinking that "(efault)" is much harder to parse in case of error
> > 
> > 'efault' looks to me like a misspelled 'default', for some reason.
> 
> I wonder if (-efault) would help a bit.

It's 9 characters, not going to satisfy sizeof(void *) * 2 on 32-bit
systems.

> Even better might be (-EFAULT). But then it would be better to use
> (NULL). It already was but it was explicitly changed to the lowercase
> variant by the commit 0f4f81dce93774a447da3c ("vsprintf: factorize
> "(null)" string").

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

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Sergey Senozhatsky]

On (04/03/18 13:52), Petr Mladek wrote:
> On Tue 2018-04-03 10:12:37, Sergey Senozhatsky wrote:
> > On (04/02/18 17:15), Andy Shevchenko wrote:
> > > > 
> > > > Hmm, I have never seen the error code in this form.
> > > 
> > > We have limited space to print it and error numbers currently can be up
> > > to 0xfff (4095). So, I have no better idea how to squeeze them while
> > > thinking that "(efault)" is much harder to parse in case of error
> > 
> > 'efault' looks to me like a misspelled 'default', for some reason.
> 
> I wonder if (-efault) would help a bit.

Dunno. If the pointer is invalid and -EFAULTS then I guess we are not
leaking anything critical and may be can just print it out. May be I'm
wrong.

	-ss

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Petr Mladek]

On Mon 2018-04-02 17:15:23, Andy Shevchenko wrote:
> On Thu, 2018-03-29 at 16:53 +0200, Petr Mladek wrote:
> > On Fri 2018-03-16 20:19:35, Andy Shevchenko wrote:
> > > On Thu, 2018-03-15 at 16:26 +0100, Petr Mladek wrote:
> > > > On Thu 2018-03-15 15:09:03, Andy Shevchenko wrote:
> 
> > > > > I still think that printing a hex value of the error code is
> > > > > much
> > > > > better
> > > > > than some odd "(efault)".
> > > > 
> > > > Do you mean (err:0e)? Google gives rather confusing answers for
> > > > this.
> > > 
> > > More like "(0xHHHH)" (we have already more than 512 error code
> > > numbers.
> > 
> > Hmm, I have never seen the error code in this form.
> 
> We have limited space to print it and error numbers currently can be up
> to 0xfff (4095). So, I have no better idea how to squeeze them while
> thinking that "(efault)" is much harder to parse in case of error
> pointer.

But this will not be used instead of address value. It is used in situations
where we print the information that is stored at the address, for example,
string, IP address, dentry name.

> >  Also google gives
> > rather confusing results when searching, for example for "(0x000E)".
> 
> It's not primarily for google, though yeah, people would google for
> error messages...
> 
> Another question is what the format: decimal versus hex for errors.
> Maybe just "(-DDDDD)"?

This still looks confusing and google does not help.

Best Regards,
Petr

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Andy Shevchenko]

On Tue, 2018-04-03 at 13:46 +0200, Petr Mladek wrote:
> On Mon 2018-04-02 17:15:23, Andy Shevchenko wrote:
> > On Thu, 2018-03-29 at 16:53 +0200, Petr Mladek wrote:
> > > On Fri 2018-03-16 20:19:35, Andy Shevchenko wrote:
> > > > On Thu, 2018-03-15 at 16:26 +0100, Petr Mladek wrote:
> > > > > On Thu 2018-03-15 15:09:03, Andy Shevchenko wrote:
> > > > > > I still think that printing a hex value of the error code is
> > > > > > much
> > > > > > better
> > > > > > than some odd "(efault)".
> > > > > 
> > > > > Do you mean (err:0e)? Google gives rather confusing answers
> > > > > for
> > > > > this.
> > > > 
> > > > More like "(0xHHHH)" (we have already more than 512 error code
> > > > numbers.
> > > 
> > > Hmm, I have never seen the error code in this form.
> > 
> > We have limited space to print it and error numbers currently can be
> > up
> > to 0xfff (4095). So, I have no better idea how to squeeze them while
> > thinking that "(efault)" is much harder to parse in case of error
> > pointer.
> 
> But this will not be used instead of address value. It is used in
> situations
> where we print the information that is stored at the address, for
> example,
> string, IP address, dentry name.

We have a lot of API functions which returns:
-ERR_PTR
NULL
struct foo *

There is no guarantee that one of that API won't be used as a supplier
for printf().

You can't dereference ERR_PTR value, but anything else except the actual
error value is worse than value itself...

> 
> > >  Also google gives
> > > rather confusing results when searching, for example for
> > > "(0x000E)".
> > 
> > It's not primarily for google, though yeah, people would google for
> > error messages...
> > 
> > Another question is what the format: decimal versus hex for errors.
> > Maybe just "(-DDDDD)"?
> 
> This still looks confusing and google does not help.

...then we have a last option just to print a value as a pointer
address.

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

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Petr Mladek]

On Tue 2018-04-03 14:54:18, Andy Shevchenko wrote:
> On Tue, 2018-04-03 at 13:46 +0200, Petr Mladek wrote:
> > On Mon 2018-04-02 17:15:23, Andy Shevchenko wrote:
> > > On Thu, 2018-03-29 at 16:53 +0200, Petr Mladek wrote:
> > > > On Fri 2018-03-16 20:19:35, Andy Shevchenko wrote:
> > > > > On Thu, 2018-03-15 at 16:26 +0100, Petr Mladek wrote:
> > > > > > On Thu 2018-03-15 15:09:03, Andy Shevchenko wrote:
> > > > > > > I still think that printing a hex value of the error code is
> > > > > > > much
> > > > > > > better
> > > > > > > than some odd "(efault)".
> > > > > > 
> > > > > > Do you mean (err:0e)? Google gives rather confusing answers
> > > > > > for
> > > > > > this.
> > > > > 
> > > > > More like "(0xHHHH)" (we have already more than 512 error code
> > > > > numbers.
> > > > 
> > > > Hmm, I have never seen the error code in this form.
> > > 
> > > We have limited space to print it and error numbers currently can be
> > > up
> > > to 0xfff (4095). So, I have no better idea how to squeeze them while
> > > thinking that "(efault)" is much harder to parse in case of error
> > > pointer.
> > 
> > But this will not be used instead of address value. It is used in
> > situations
> > where we print the information that is stored at the address, for
> > example,
> > string, IP address, dentry name.
> 
> We have a lot of API functions which returns:
> -ERR_PTR
> NULL
> struct foo *
> 
> There is no guarantee that one of that API won't be used as a supplier
> for printf().

OK, I think that I have finally understood it. You would like to
detect ERR_PTR values and handle them specially? I mean to show
the value?

But then we would need to distinguish three types of errors,
something like:

 + (null)       for pure NULL address
 + (e:XXXX)     for address in IS_ERR_VALUE() range
 + (efault)     for any other invalid address

Then people might want to see values also from the first 4096 bytes.
This is getting too complicated. I am not sure if it is worth it.


> You can't dereference ERR_PTR value, but anything else except the actual
> error value is worse than value itself...

Yes and no, see below.

> > 
> > > >  Also google gives
> > > > rather confusing results when searching, for example for
> > > > "(0x000E)".
> > > 
> > > It's not primarily for google, though yeah, people would google for
> > > error messages...
> > > 
> > > Another question is what the format: decimal versus hex for errors.
> > > Maybe just "(-DDDDD)"?
> > 
> > This still looks confusing and google does not help.
> 
> ...then we have a last option just to print a value as a pointer
> address.

We could not print the real address from security reasons. The hashed
pointer value is not much helpful. IMHO, a common error string is
easier to spot or search for.

Best Regards,
Petr

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Andy Shevchenko]

On Tue, 2018-04-03 at 15:13 +0200, Petr Mladek wrote:
> On Tue 2018-04-03 14:54:18, Andy Shevchenko wrote:
> > On Tue, 2018-04-03 at 13:46 +0200, Petr Mladek wrote:
> > > On Mon 2018-04-02 17:15:23, Andy Shevchenko wrote:
> > > > On Thu, 2018-03-29 at 16:53 +0200, Petr Mladek wrote:
> > > > > On Fri 2018-03-16 20:19:35, Andy Shevchenko wrote:
> > > > > > On Thu, 2018-03-15 at 16:26 +0100, Petr Mladek wrote:
> > > > > > > On Thu 2018-03-15 15:09:03, Andy Shevchenko wrote:
> > > > > > > > I still think that printing a hex value of the error
> > > > > > > > code is
> > > > > > > > much
> > > > > > > > better
> > > > > > > > than some odd "(efault)".
> > > > > > > 
> > > > > > > Do you mean (err:0e)? Google gives rather confusing
> > > > > > > answers
> > > > > > > for
> > > > > > > this.
> > > > > > 
> > > > > > More like "(0xHHHH)" (we have already more than 512 error
> > > > > > code
> > > > > > numbers.
> > > > > 
> > > > > Hmm, I have never seen the error code in this form.
> > > > 
> > > > We have limited space to print it and error numbers currently
> > > > can be
> > > > up
> > > > to 0xfff (4095). So, I have no better idea how to squeeze them
> > > > while
> > > > thinking that "(efault)" is much harder to parse in case of
> > > > error
> > > > pointer.
> > > 
> > > But this will not be used instead of address value. It is used in
> > > situations
> > > where we print the information that is stored at the address, for
> > > example,
> > > string, IP address, dentry name.
> > 
> > We have a lot of API functions which returns:
> > -ERR_PTR
> > NULL
> > struct foo *
> > 
> > There is no guarantee that one of that API won't be used as a
> > supplier
> > for printf().
> 
> OK, I think that I have finally understood it. You would like to
> detect ERR_PTR values and handle them specially? I mean to show
> the value?
> 
> But then we would need to distinguish three types of errors,
> something like:
> 
>  + (null)       for pure NULL address
>  + (e:XXXX)     for address in IS_ERR_VALUE() range

// Just IS_ERR(). IS_ERR_VALUE() is not meant to be used widely

>  + (efault)     for any other invalid address
> 
> Then people might want to see values also from the first 4096 bytes.
> This is getting too complicated.

No, it's not. (null) case is already in kernel, you came with (efault),
but IS_ERR() case or any other case like it is just printing of standard
pointer value. See in the code where special_hex_number() is called.

>  I am not sure if it is worth it.

Your patch will hide values for error codes. Not good for debugging.

> 
> 
> > You can't dereference ERR_PTR value, but anything else except the
> > actual
> > error value is worse than value itself...
> 
> Yes and no, see below.

Yes, there is no "no".

> 
> > > 
> > > > >  Also google gives
> > > > > rather confusing results when searching, for example for
> > > > > "(0x000E)".
> > > > 
> > > > It's not primarily for google, though yeah, people would google
> > > > for
> > > > error messages...
> > > > 
> > > > Another question is what the format: decimal versus hex for
> > > > errors.
> > > > Maybe just "(-DDDDD)"?
> > > 
> > > This still looks confusing and google does not help.
> > 
> > ...then we have a last option just to print a value as a pointer
> > address.
> 
> We could not print the real address from security reasons. The hashed
> pointer value is not much helpful. IMHO, a common error string is
> easier to spot or search for.

Did you read what I'm writing? How on the earth the pointer in the range
of -1...-4095 would be a security issue?!

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

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[Petr Mladek]

On Tue 2018-04-03 16:40:58, Andy Shevchenko wrote:
> On Tue, 2018-04-03 at 15:13 +0200, Petr Mladek wrote:
> > On Tue 2018-04-03 14:54:18, Andy Shevchenko wrote:
> > > On Tue, 2018-04-03 at 13:46 +0200, Petr Mladek wrote:
> > > > On Mon 2018-04-02 17:15:23, Andy Shevchenko wrote:
> > > We have a lot of API functions which returns:
> > > -ERR_PTR
> > > NULL
> > > struct foo *
> > > 
> > > There is no guarantee that one of that API won't be used as a
> > > supplier
> > > for printf().
> > 
> > OK, I think that I have finally understood it. You would like to
> > detect ERR_PTR values and handle them specially? I mean to show
> > the value?
> > 
> > But then we would need to distinguish three types of errors,
> > something like:
> > 
> >  + (null)       for pure NULL address
> >  + (e:XXXX)     for address in IS_ERR_VALUE() range
> 
> // Just IS_ERR(). IS_ERR_VALUE() is not meant to be used widely

IS_ERR() is just a wrapper above IS_ERR_VALUE(). The range is important
here and it is the same for both.


> >  + (efault)     for any other invalid address
> > 
> > Then people might want to see values also from the first 4096 bytes.
> > This is getting too complicated.
> 
> No, it's not. (null) case is already in kernel, you came with (efault),
> but IS_ERR() case or any other case like it is just printing of standard
> pointer value. See in the code where special_hex_number() is called.
> 
> >  I am not sure if it is worth it.
> 
> Your patch will hide values for error codes. Not good for debugging.

It does it in situation where we mostly silently crashed so
far. Therefore I do not see this as a big regression.

If we hanle IS_ERR() a special way, it will mean more code and more types
of error messages. People might be confused by the difference between
(e:000e) and (efault).

Yes, it might help to distinguish this situation in some cases.
But typically the information about invalid address access should be
enough. People will either see the problem from the code or they would
need to add more debug messages anyway.



> > > You can't dereference ERR_PTR value, but anything else except the
> > > > 
> > > > > >  Also google gives
> > > > > > rather confusing results when searching, for example for
> > > > > > "(0x000E)".
> > > > > 
> > > > > It's not primarily for google, though yeah, people would google
> > > > > for
> > > > > error messages...
> > > > > 
> > > > > Another question is what the format: decimal versus hex for
> > > > > errors.
> > > > > Maybe just "(-DDDDD)"?
> > > > 
> > > > This still looks confusing and google does not help.
> > > 
> > > ...then we have a last option just to print a value as a pointer
> > > address.
> > 
> > We could not print the real address from security reasons. The hashed
> > pointer value is not much helpful. IMHO, a common error string is
> > easier to spot or search for.
> 
> Did you read what I'm writing? How on the earth the pointer in the range
> of -1...-4095 would be a security issue?!

Please, read your mails again. You never wrote that you were talking
about handling error codes specially. I mean this thread starting at
https://lkml.kernel.org/r/20180314140947.rs3b6i5gguzzu5wi@pathway.suse.cz

Most of the time I though that you were talking about displaying the
single error code -EFAULT for any invalid address or showing any
invalid address directly. I am not a psychic.

OK, you mentioned this in the early mail at
https://lkml.kernel.org/r/1520446689.10722.493.camel@linux.intel.com
But it was in a bit different context. Also it was right before the mail
from Linus who wrote:

"Guys, stop this idiocy. printk() needs to be *simple* and *reliable*,
not fancy."

He then wrote many times that NULL might be considered special.
But that we should really keep it simple.

Best Regards,
Petr

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[kbuild test robot]

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

Hi Petr,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc5 next-20180314]
[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-crash-when-dereferencing-invalid-pointers/20180315-214100
config: i386-randconfig-x019-201810 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 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:273:8: error: too many arguments to function 'plain_format'
     err = plain_format(ptr);
           ^~~~~~~~~~~~
   lib/test_printf.c:235:1: note: declared here
    plain_format(void)
    ^~~~~~~~~~~~

vim +/plain_format +273 lib/test_printf.c

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

---
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: 30041 bytes --]

Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

[kbuild test robot]

Hi Petr,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v4.16-rc5 next-20180314]
[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-crash-when-dereferencing-invalid-pointers/20180315-214100
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> lib/vsprintf.c:533:16: sparse: Using plain integer as NULL pointer
   lib/vsprintf.c:762:18: sparse: Variable length array is used.
   lib/vsprintf.c:765:38: sparse: cannot size expression
   lib/vsprintf.c:1532:23: sparse: incorrect type in assignment (different base types) @@    expected unsigned long [unsigned] [assigned] flags @@    got  long [unsigned] [assigned] flags @@
   lib/vsprintf.c:1532:23:    expected unsigned long [unsigned] [assigned] flags
   lib/vsprintf.c:1532:23:    got restricted gfp_t [usertype] <noident>

vim +533 lib/vsprintf.c

   522	
   523	static const char *check_pointer_access(const void *ptr)
   524	{
   525		unsigned char byte;
   526	
   527		if (!ptr)
   528			return "(null)";
   529	
   530		if (probe_kernel_read(&byte, ptr, 1))
   531			return "(efault)";
   532	
 > 533		return 0;
   534	}
   535	

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

[PATCH 1/2] vsprintf: distinguish between (null), (err) and (invalid) pointer derefs

[Adam Borowski]

Attempting to print an object pointed to by a bad (usually ERR_PTR) pointer
is a not so surprising error.  Our code handles them inconsistently:
 * two places print (null) if ptr<PAGE_SIZE
 * one place prints (null) if abs(ptr)<PAGE_SIZE
 * one place prints (null) only if !ptr

Obviously, saying (null) for a small but non-0 value is misleading.
Thus, let's print:
 * (null) for exactly 0
 * (err) if last page && abs(ptr)<=MAX_ERRNO
 * (invalid) otherwise

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
 lib/vsprintf.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7a708f82559..1c2c3cc5a321 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -47,6 +47,8 @@
 #include <linux/string_helpers.h>
 #include "kstrtox.h"
 
+#define BAD_PTR_STRING(x) (!(x) ? "(null)" : IS_ERR(x) ? "(err)" : "(invalid)")
+
 /**
  * simple_strtoull - convert a string to an unsigned long long
  * @cp: The start of the string
@@ -588,7 +590,7 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 	size_t lim = spec.precision;
 
 	if ((unsigned long)s < PAGE_SIZE)
-		s = "(null)";
+		s = BAD_PTR_STRING(s);
 
 	while (lim--) {
 		char c = *s++;
@@ -1582,7 +1584,7 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 		return string(buf, end, "(!OF)", spec);
 
 	if ((unsigned long)dn < PAGE_SIZE)
-		return string(buf, end, "(null)", spec);
+		return string(buf, end, BAD_PTR_STRING(dn), spec);
 
 	/* simple case without anything any more format specifiers */
 	fmt++;
@@ -1851,12 +1853,12 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 
 	if (!ptr && *fmt != 'K' && *fmt != 'x') {
 		/*
-		 * Print (null) with the same width as a pointer so it makes
-		 * tabular output look nice.
+		 * Print (null)/etc 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(buf, end, "(null)", spec);
+		return string(buf, end, BAD_PTR_STRING(ptr), spec);
 	}
 
 	switch (*fmt) {
@@ -2575,7 +2577,7 @@ int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args)
 
 			if ((unsigned long)save_str > (unsigned long)-PAGE_SIZE
 					|| (unsigned long)save_str < PAGE_SIZE)
-				save_str = "(null)";
+				save_str = BAD_PTR_STRING(save_str);
 			len = strlen(save_str) + 1;
 			if (str + len < end)
 				memcpy(str, save_str, len);
-- 
2.16.2

[PATCH 2/2] vsprintf: don't dereference pointers to the first or last page

[Adam Borowski]

As old code to avoid so is inconsistent, let's unify it within a single
macro.

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
 lib/vsprintf.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 1c2c3cc5a321..4914dac03f0a 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -47,6 +47,8 @@
 #include <linux/string_helpers.h>
 #include "kstrtox.h"
 
+#define IS_BAD_PTR(x) ((unsigned long)(x) >= (unsigned long)-PAGE_SIZE \
+				|| (unsigned long)(x) < PAGE_SIZE)
 #define BAD_PTR_STRING(x) (!(x) ? "(null)" : IS_ERR(x) ? "(err)" : "(invalid)")
 
 /**
@@ -589,7 +591,7 @@ char *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)
+	if (IS_BAD_PTR(s))
 		s = BAD_PTR_STRING(s);
 
 	while (lim--) {
@@ -1583,7 +1585,7 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	if (!IS_ENABLED(CONFIG_OF))
 		return string(buf, end, "(!OF)", spec);
 
-	if ((unsigned long)dn < PAGE_SIZE)
+	if (IS_BAD_PTR(dn))
 		return string(buf, end, BAD_PTR_STRING(dn), spec);
 
 	/* simple case without anything any more format specifiers */
@@ -1851,7 +1853,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 {
 	const int default_width = 2 * sizeof(void *);
 
-	if (!ptr && *fmt != 'K' && *fmt != 'x') {
+	if (IS_BAD_PTR(ptr) && *fmt != 'K' && *fmt != 'x') {
 		/*
 		 * Print (null)/etc with the same width as a pointer so it
 		 * makes tabular output look nice.
@@ -2575,8 +2577,7 @@ int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args)
 			const char *save_str = va_arg(args, char *);
 			size_t len;
 
-			if ((unsigned long)save_str > (unsigned long)-PAGE_SIZE
-					|| (unsigned long)save_str < PAGE_SIZE)
+			if (IS_BAD_PTR(save_ptr))
 				save_str = BAD_PTR_STRING(save_str);
 			len = strlen(save_str) + 1;
 			if (str + len < end)
-- 
2.16.2

Re: [PATCH 2/2] vsprintf: don't dereference pointers to the first or last page

[Andy Shevchenko]

On Tue, 2018-03-06 at 19:11 +0100, Adam Borowski wrote:
> As old code to avoid so is inconsistent, let's unify it within a
> single
> macro.
> 
>  
> +#define IS_BAD_PTR(x) ((unsigned long)(x) >= (unsigned long)-
> PAGE_SIZE \
> +				|| (unsigned long)(x) < PAGE_SIZE)

Oh, no.

First of all, why it's a macro?

Next, what prevents us to do it in place using IS_ERR() instead? (Btw, I
have a patch for that, not published yet)

>  #define BAD_PTR_STRING(x) (!(x) ? "(null)" : IS_ERR(x) ? "(err)" :
> "(invalid)")
>  
>  /**
> @@ -589,7 +591,7 @@ char *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)
> +	if (IS_BAD_PTR(s))
>  		s = BAD_PTR_STRING(s);

I don't think it's a good idea to change current behaviour.

> @@ -1583,7 +1585,7 @@ char *device_node_string(char *buf, char *end,
> struct device_node *dn,
>  	if (!IS_ENABLED(CONFIG_OF))
>  		return string(buf, end, "(!OF)", spec);
>  
> -	if ((unsigned long)dn < PAGE_SIZE)
> +	if (IS_BAD_PTR(dn))
>  		return string(buf, end, BAD_PTR_STRING(dn), spec);

This makes no sense. Explained in comment against patch 1.

>  
>  	/* simple case without anything any more format specifiers */
> @@ -1851,7 +1853,7 @@ char *pointer(const char *fmt, char *buf, char
> *end, void *ptr,
>  {
>  	const int default_width = 2 * sizeof(void *);
>  
> -	if (!ptr && *fmt != 'K' && *fmt != 'x') {
> +	if (IS_BAD_PTR(ptr) && *fmt != 'K' && *fmt != 'x') {
>  		/*
>  		 * Print (null)/etc with the same width as a pointer
> so it
>  		 * makes tabular output look nice.
> 

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

Re: [PATCH 1/2] vsprintf: distinguish between (null), (err) and (invalid) pointer derefs

[Andy Shevchenko]

On Tue, 2018-03-06 at 19:11 +0100, Adam Borowski wrote:

Thanks for the patch, my comments below.

> Attempting to print an object pointed to by a bad (usually ERR_PTR)
> pointer
> is a not so surprising error.  Our code handles them inconsistently:
>  * two places print (null) if ptr<PAGE_SIZE
>  * one place prints (null) if abs(ptr)<PAGE_SIZE
>  * one place prints (null) only if !ptr
> 
> Obviously, saying (null) for a small but non-0 value is misleading.
> Thus, let's print:
>  * (null) for exactly 0
>  * (err) if last page && abs(ptr)<=MAX_ERRNO
>  * (invalid) otherwise
> 

First of all, this patch is much more arguable than the other one in
your small series.

"(invalid)" is invalid. Hint: there is a nice comment in the code why.

I'm in principle not putting explanation here to insist people to
eventually _read and understand_ the code before doing anything.

Some comments below.
 
> +#define BAD_PTR_STRING(x) (!(x) ? "(null)" : IS_ERR(x) ? "(err)" :
> "(invalid)")

It looks ugly.

>  /**
>   * simple_strtoull - convert a string to an unsigned long long
>   * @cp: The start of the string
> @@ -588,7 +590,7 @@ char *string(char *buf, char *end, const char *s,
> struct printf_spec spec)
>  	size_t lim = spec.precision;
>  
>  	if ((unsigned long)s < PAGE_SIZE)
> -		s = "(null)";
> +		s = BAD_PTR_STRING(s);

It doesn't make any sense before your patch 2.
 
>  	if ((unsigned long)dn < PAGE_SIZE)
> -		return string(buf, end, "(null)", spec);
> +		return string(buf, end, BAD_PTR_STRING(dn), spec);

It simple doesn't make sense.

The idea is to do it below, in the pointer.
These certain lines are going to be removed by my patch.

> -		return string(buf, end, "(null)", spec);
> +		return string(buf, end, BAD_PTR_STRING(ptr), spec);

Doesn't make sense before your patch 2.

>  			if ((unsigned long)save_str > (unsigned
> long)-PAGE_SIZE
>  					|| (unsigned long)save_str <
> PAGE_SIZE)
> -				save_str = "(null)";
> +				save_str = BAD_PTR_STRING(save_str);

This is perhaps one valid change in such situation.

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

Re: [PATCH 1/2] vsprintf: distinguish between (null), (err) and (invalid) pointer derefs

[Adam Borowski]

On Wed, Mar 07, 2018 at 03:17:19PM +0200, Andy Shevchenko wrote:
> On Tue, 2018-03-06 at 19:11 +0100, Adam Borowski wrote:
> 
> Thanks for the patch, my comments below.

(Review snipped.)

It looks pretty obvious that it'd take a lot less of your time to roll new
patch[es] from scratch than to try to educate me wrt how you'd want to see
it done; thus I'll sit out this one.


Meow!
-- 
⢀⣴⠾⠻⢶⣦⠀ 
⣾⠁⢠⠒⠀⣿⡁ A dumb species has no way to open a tuna can.
⢿⡄⠘⠷⠚⠋⠀ A smart species invents a can opener.
⠈⠳⣄⠀⠀⠀⠀ A master species delegates.

Re: [PATCH 1/2] vsprintf: distinguish between (null), (err) and (invalid) pointer derefs

[Andy Shevchenko]

On Tue, 2018-03-06 at 19:11 +0100, Adam Borowski wrote:
> Attempting to print an object pointed to by a bad (usually ERR_PTR)
> pointer
> is a not so surprising error.  Our code handles them inconsistently:
>  * two places print (null) if ptr<PAGE_SIZE
>  * one place prints (null) if abs(ptr)<PAGE_SIZE
>  * one place prints (null) only if !ptr
> 
> Obviously, saying (null) for a small but non-0 value is misleading.
> Thus, let's print:
>  * (null) for exactly 0
>  * (err) if last page && abs(ptr)<=MAX_ERRNO
>  * (invalid) otherwise

Ah, and last but not least thing.
Where are the test cases?

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

Re: [PATCH v2 8/9] lib/vsprintf: Remove useless NULL checks

[Andy Shevchenko]

On Fri, 2018-03-02 at 13:51 +0100, Petr Mladek wrote:

> BTW: I am not sure who is going to pass this patchset to Linus.
> If nobody is against, I could eventually do so via printk.git.

Usually Andrew Morton takes care.
But perhaps it's a time to unload Andrew in this part at least?

Would you agree to be a maintainer of lib/vsprinf.c and
lib/test_printf.c ?

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

Re: [PATCH v2 8/9] lib/vsprintf: Remove useless NULL checks

[Petr Mladek]

On Fri 2018-03-02 16:15:06, Andy Shevchenko wrote:
> On Fri, 2018-03-02 at 13:51 +0100, Petr Mladek wrote:
> 
> > BTW: I am not sure who is going to pass this patchset to Linus.
> > If nobody is against, I could eventually do so via printk.git.
> 
> Usually Andrew Morton takes care.
> But perhaps it's a time to unload Andrew in this part at least?

I think that Andrew took these patches as the last resort.

> Would you agree to be a maintainer of lib/vsprinf.c and
> lib/test_printf.c ?

If nobody is against, I could do so.

Best Regards,
Petr

Re: [PATCH v2 8/9] lib/vsprintf: Remove useless NULL checks

[Andy Shevchenko]

On Wed, 2018-02-28 at 11:04 +0100, Petr Mladek wrote:
> On Tue 2018-02-27 19:35:50, Andy Shevchenko wrote:
> > On Tue, 2018-02-27 at 16:50 +0100, Petr Mladek wrote:
> > > On Fri 2018-02-16 23:07:10, Andy Shevchenko wrote:
> > > > 
> > > 

> > > This macro matches also values <= 16.
> > 
> > Yes, I know.
> > 
> > This had been discussed with Rasmus and we agreed that printing a
> > result
> > of kmalloc(0) is rather weird.
> 
> I see
> https://lkml.kernel.org/r/1500546142.29303.133.camel@linux.intel.com
> There you suggested to move this check into pointer(). But I do not
> see any agreement on this.
> 

Btw, I'm pretty sure that the checks like this or another one with
PAGE_SIZE is cargo cult programming rather than imaging possible so
weird use cases.

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

Re: [PATCH v2 8/9] lib/vsprintf: Remove useless NULL checks

[Andy Shevchenko]

+Cc: Pantelis, author of %pOF extension
(I leave a lot of the message from Petr to give you a bit of context)

On Wed, 2018-02-28 at 11:04 +0100, Petr Mladek wrote:
> On Tue 2018-02-27 19:35:50, Andy Shevchenko wrote:
> > On Tue, 2018-02-27 at 16:50 +0100, Petr Mladek wrote:
> > > On Fri 2018-02-16 23:07:10, Andy Shevchenko wrote:
> > > > The pointer can't be NULL since it's first what has been done in
> > > > the
> > > > pointer().
> > > > 
> > > > Remove useless checks.
> > > > 
> > > > Note we leave check for !CONFIG_HAVE_CLK to make compiler
> > > > to optimize code away when possible.
 
> > > > -	if (ZERO_OR_NULL_PTR(addr))
> > > 
> > > This macro matches also values <= 16.
> > 
> > Yes, I know.
> > 
> > This had been discussed with Rasmus and we agreed that printing a
> > result
> > of kmalloc(0) is rather weird.
> 
> I see
> https://lkml.kernel.org/r/1500546142.29303.133.camel@linux.intel.com
> There you suggested to move this check into pointer(). But I do not
> see any agreement on this.

> > > >  	switch (fmt[1]) {
> > > > @@ -1580,9 +1572,6 @@ char *device_node_string(char *buf, char
> > > > *end,
> > > > struct device_node *dn,
> > > >  	if (!IS_ENABLED(CONFIG_OF))
> > > >  		return string(buf, end, "(!OF)", spec);
> > > >  
> > > > -	if ((unsigned long)dn < PAGE_SIZE)
> > > > -		return string(buf, end, "(null)", spec);
> > > 
> > > In this case, "null" was printed for ptr < PAGE_SIZE. The same
> > > check
> > > is also in string() function.
> > 
> > Do we have a uses cases when invalid (non-NULL) pointer is supplied
> > to
> > print function?
> > 
> > Those call sites have to be fixed.

Pantelis, is this check necessary? What are the use cases of node
pointer being < PAGE_SIZE?

And main question, can it be just (re-)moved to simple NULL check?

See below the originate of the PAGE_SIZE check what Petr found.

> I am not aware of any. But this patch will make fixing such locations
> more complicated. The kernel would crash and might not show any
> message.
> Is this really what we want?
> 
> Note that it will most likely crash in vprintk_emit() on the line
> 
>    text_len = vscnprintf(text, sizeof(textbuf), fmt, args);
> 
> It will be with logbug_lock() taken. The nested printk() messages
> will be stored in per-CPU buffer thanks to printk_safe code.
> They might eventually be printed by printk_safe_flush_on_panic()
> but it is not guaranteed.
> 
> 
> > > Note that it is not only about the printed value. The pointer is
> > > later
> > > derefecend. We will start crashing on dn > 0 && dn < PAGE_SIZE.
> > 
> > Yes.
> > So, fix the call sites!
> 
> It would be easier if printk() was able to show the message
> when hitting this place.
> 
> I did some archaeology. The first check for PAGE_SIZE was added
> by the pre-git commit:
> 
> commit 8bcb3ba1dec5749a7f1eed570cb69a20c2e4bd41
> Author: Andrew Morton <akpm@osdl.org>
> Date:   Tue Oct 21 18:22:28 2003 -0700
> 
>     [PATCH] make printk more robust with "null" pointers
>     
>     Expand printk's traditional handling of null pointers so that
> anything in the
>     first page is considered a null pointer.
>     
>     This gives us better behaviour when someone (acpi..) accidentally
> prints a
>     string which is embedded in a struct, the pointer to which is
> null.
> 
> 
> IMHO, it would make sense to hanve this check also pointers that are
> being deferred.
> 
> 
> > > To be honest, I do not feel experienced enough to decide
> > > about the preferred behavior. On one hand, it is bad when
> > > printk() would crash the kernel. On the other hand, hiding wide
> > > range of values under "(null)" string might confuse people.
> > > Would it make sense to survive and write different strings for
> > > difference intervals? For example?
> > > 
> > >     "(null)"     for ptr == 0
> > >     "(null-16)"  for ptr > 0 && ptr <= 16
> > >     "(null-pg)"  for prt > 16 && ptr <= PAGE_SIZE
> > > 
> > > In each case, this patch changes the behavior and it should
> > > be documented in the commit message.
> > 
> > Personally I strongly disagree with blowing code up in such places
> > for
> > little or none benefit.
> 
> I do not have strong opinion here. I could imagine that this might
> save a day to some people. But I have never encountered such a bug
> myself.
> 
> To make it clear. Your clean up work makes sense. I just want to point
> out that this patch is not as innocent as the commit message suggest.
> Also I think that it goes in the wrong direction regarding the
> ability to show useful information in a buggy situation.

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

[PATCH v2 9/9] lib/vsprintf: Mark expected switch fall-through

[Andy Shevchenko]

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a49da00b79e7..28d7aca6a805 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2081,6 +2081,7 @@ int format_decode(const char *fmt, struct printf_spec *spec)
 
 	case 'x':
 		spec->flags |= SMALL;
+		/* fall through */
 
 	case 'X':
 		spec->base = 16;
@@ -3035,8 +3036,10 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 			break;
 		case 'i':
 			base = 0;
+			/* fall through */
 		case 'd':
 			is_sign = true;
+			/* fall through */
 		case 'u':
 			break;
 		case '%':
-- 
2.15.1

Re: [PATCH v2 9/9] lib/vsprintf: Mark expected switch fall-through

[Petr Mladek]

On Fri 2018-02-16 23:07:11, Andy Shevchenko wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

I have pushed it into printk.git, branch for-4.18-vsprintf-cleanup.

Best Regards,
Petr

Re: [PATCH v2 1/9] lib/test_printf: Mark big constant with ULL

[Luc Van Oostenryck]

On Fri, Feb 16, 2018 at 11:07:03PM +0200, Andy Shevchenko wrote:
> Sparse complains that constant is so bit for unsigned long on 64-bit
> architecture.
> 
> lib/test_printf.c:217:54: warning: constant 0xffff0123456789ab is so big it is unsigned long
> lib/test_printf.c:246:54: warning: constant 0xffff0123456789ab is so big it is unsigned long
> 
> To satisfy everyone, mark the constant with ULL.

It should be 'UL' not 'ULL' since for architectures a pointer and
a unsigned long have the ame size while on 32bit archs, long long
are (or may?) 64bit.

-- Luc Van Oostenryck

Re: [PATCH v2 1/9] lib/test_printf: Mark big constant with ULL

[Andy Shevchenko]

On Sun, Feb 18, 2018 at 2:58 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> On Fri, Feb 16, 2018 at 11:07:03PM +0200, Andy Shevchenko wrote:
>> Sparse complains that constant is so bit for unsigned long on 64-bit
>> architecture.
>>
>> lib/test_printf.c:217:54: warning: constant 0xffff0123456789ab is so big it is unsigned long
>> lib/test_printf.c:246:54: warning: constant 0xffff0123456789ab is so big it is unsigned long
>>
>> To satisfy everyone, mark the constant with ULL.
>
> It should be 'UL' not 'ULL' since for architectures a pointer and
> a unsigned long have the ame size while on 32bit archs, long long
> are (or may?) 64bit.

Perhaps, I'll try next week. Though ULL works fine as well.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/9] lib/test_printf: Mark big constant with ULL

[Andy Shevchenko]

On Sun, 2018-02-18 at 13:58 +0100, Luc Van Oostenryck wrote:
> On Fri, Feb 16, 2018 at 11:07:03PM +0200, Andy Shevchenko wrote:
> > Sparse complains that constant is so bit for unsigned long on 64-bit
> > architecture.
> > 
> > lib/test_printf.c:217:54: warning: constant 0xffff0123456789ab is so
> > big it is unsigned long
> > lib/test_printf.c:246:54: warning: constant 0xffff0123456789ab is so
> > big it is unsigned long
> > 
> > To satisfy everyone, mark the constant with ULL.
> 
> It should be 'UL' not 'ULL' since for architectures a pointer and
> a unsigned long have the ame size while on 32bit archs, long long
> are (or may?) 64bit.

Yes, UL works as well.

Andrew, tell me if I need to send an update (followup) or a new version.

Btw, I ran test_printf suite on both 32- and 64-bit code, everything
passed. So, if anyone notices a regression, please, create a test case
that we may run.

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

Re: [PATCH v2 1/9] lib/test_printf: Mark big constant with ULL

[Petr Mladek]

On Mon 2018-02-19 17:24:22, Andy Shevchenko wrote:
> On Sun, 2018-02-18 at 13:58 +0100, Luc Van Oostenryck wrote:
> > On Fri, Feb 16, 2018 at 11:07:03PM +0200, Andy Shevchenko wrote:
> > > Sparse complains that constant is so bit for unsigned long on 64-bit
> > > architecture.
> > > 
> > > lib/test_printf.c:217:54: warning: constant 0xffff0123456789ab is so
> > > big it is unsigned long
> > > lib/test_printf.c:246:54: warning: constant 0xffff0123456789ab is so
> > > big it is unsigned long
> > > 
> > > To satisfy everyone, mark the constant with ULL.
> > 
> > It should be 'UL' not 'ULL' since for architectures a pointer and
> > a unsigned long have the ame size while on 32bit archs, long long
> > are (or may?) 64bit.
> 
> Yes, UL works as well.

I have updated the patch and pushed it into printk.git,
branch for-4.18-vsprintf-cleanup.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v2 1/9] lib/test_printf: Mark big constant with ULL

[Tobin C. Harding]

On Fri, Feb 16, 2018 at 11:07:03PM +0200, Andy Shevchenko wrote:
...

Hi Andy,

What tree does this set apply to please?  I tried mainline rc1 and
next-20180216.  Happy to see some code duplication removal from
vsprintf.c :)

thanks,
Tobin.

Re: [PATCH v2 1/9] lib/test_printf: Mark big constant with ULL

[Andy Shevchenko]

On Sun, Feb 18, 2018 at 11:52 PM, Tobin C. Harding <me@tobin.cc> wrote:
> On Fri, Feb 16, 2018 at 11:07:03PM +0200, Andy Shevchenko wrote:

> What tree does this set apply to please?  I tried mainline rc1 and
> next-20180216.  Happy to see some code duplication removal from
> vsprintf.c :)

IIRC latest next, i.e. 20180217.


-- 
With Best Regards,
Andy Shevchenko