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

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

[Petr Mladek]

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

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

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

Changes against v3:

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

Changes against v2:

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

Changes against v1:

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


Petr Mladek (9):
  vsprintf: Shuffle 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                            | 451 ++++++++++++++++++------------
 3 files changed, 313 insertions(+), 183 deletions(-)

-- 
2.13.6

[PATCH v4 1/9] vsprintf: Shuffle ptr_to_id() code

[Petr Mladek]

This patch just moves ptr_to_id() implementation up in the source
file. We will want to call it earlier.

This patch does not change the existing behavior.

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

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7a708f82559..c0c677c6a833 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -602,6 +602,73 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 	return widen_string(buf, len, end, spec);
 }
 
+static bool have_filled_random_ptr_key __read_mostly;
+static siphash_key_t ptr_key __read_mostly;
+
+static void fill_random_ptr_key(struct random_ready_callback *unused)
+{
+	get_random_bytes(&ptr_key, sizeof(ptr_key));
+	/*
+	 * have_filled_random_ptr_key==true is dependent on get_random_bytes().
+	 * ptr_to_id() needs to see have_filled_random_ptr_key==true
+	 * after get_random_bytes() returns.
+	 */
+	smp_mb();
+	WRITE_ONCE(have_filled_random_ptr_key, true);
+}
+
+static struct random_ready_callback random_ready = {
+	.func = fill_random_ptr_key
+};
+
+static int __init initialize_ptr_random(void)
+{
+	int ret = add_random_ready_callback(&random_ready);
+
+	if (!ret) {
+		return 0;
+	} else if (ret == -EALREADY) {
+		fill_random_ptr_key(&random_ready);
+		return 0;
+	}
+
+	return ret;
+}
+early_initcall(initialize_ptr_random);
+
+/* Maps a pointer to a 32 bit unique identifier. */
+static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
+{
+	unsigned long hashval;
+	const int default_width = 2 * sizeof(ptr);
+
+	if (unlikely(!have_filled_random_ptr_key)) {
+		spec.field_width = default_width;
+		/* string length must be less than default_width */
+		return string(buf, end, "(ptrval)", spec);
+	}
+
+#ifdef CONFIG_64BIT
+	hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
+	/*
+	 * Mask off the first 32 bits, this makes explicit that we have
+	 * modified the address (and 32 bits is plenty for a unique ID).
+	 */
+	hashval = hashval & 0xffffffff;
+#else
+	hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
+#endif
+
+	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);
+}
+
 static noinline_for_stack
 char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_spec spec,
 		  const char *fmt)
@@ -1659,73 +1726,6 @@ char *pointer_string(char *buf, char *end, const void *ptr,
 	return number(buf, end, (unsigned long int)ptr, spec);
 }
 
-static bool have_filled_random_ptr_key __read_mostly;
-static siphash_key_t ptr_key __read_mostly;
-
-static void fill_random_ptr_key(struct random_ready_callback *unused)
-{
-	get_random_bytes(&ptr_key, sizeof(ptr_key));
-	/*
-	 * have_filled_random_ptr_key==true is dependent on get_random_bytes().
-	 * ptr_to_id() needs to see have_filled_random_ptr_key==true
-	 * after get_random_bytes() returns.
-	 */
-	smp_mb();
-	WRITE_ONCE(have_filled_random_ptr_key, true);
-}
-
-static struct random_ready_callback random_ready = {
-	.func = fill_random_ptr_key
-};
-
-static int __init initialize_ptr_random(void)
-{
-	int ret = add_random_ready_callback(&random_ready);
-
-	if (!ret) {
-		return 0;
-	} else if (ret == -EALREADY) {
-		fill_random_ptr_key(&random_ready);
-		return 0;
-	}
-
-	return ret;
-}
-early_initcall(initialize_ptr_random);
-
-/* Maps a pointer to a 32 bit unique identifier. */
-static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
-{
-	unsigned long hashval;
-	const int default_width = 2 * sizeof(ptr);
-
-	if (unlikely(!have_filled_random_ptr_key)) {
-		spec.field_width = default_width;
-		/* string length must be less than default_width */
-		return string(buf, end, "(ptrval)", spec);
-	}
-
-#ifdef CONFIG_64BIT
-	hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
-	/*
-	 * Mask off the first 32 bits, this makes explicit that we have
-	 * modified the address (and 32 bits is plenty for a unique ID).
-	 */
-	hashval = hashval & 0xffffffff;
-#else
-	hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
-#endif
-
-	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);
-}
-
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
-- 
2.13.6

[PATCH v4 2/9] vsprintf: Consistent %pK handling for kptr_restrict == 0

[Petr Mladek]

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

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

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

CPU0                            CPU1

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

    number()

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

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index c0c677c6a833..9ade2c4f21ba 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -637,7 +637,8 @@ static int __init initialize_ptr_random(void)
 early_initcall(initialize_ptr_random);
 
 /* Maps a pointer to a 32 bit unique identifier. */
-static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
+static char *ptr_to_id(char *buf, char *end,
+		       const void *ptr, struct printf_spec spec)
 {
 	unsigned long hashval;
 	const int default_width = 2 * sizeof(ptr);
@@ -1426,8 +1427,8 @@ char *restricted_pointer(char *buf, char *end, const void *ptr,
 
 	switch (kptr_restrict) {
 	case 0:
-		/* Always print %pK values */
-		break;
+		/* Handle as %p, hash and do _not_ leak addresses. */
+		return ptr_to_id(buf, end, ptr, spec);
 	case 1: {
 		const struct cred *cred;
 
@@ -1931,8 +1932,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			return buf;
 		}
 	case 'K':
-		if (!kptr_restrict)
-			break;
 		return restricted_pointer(buf, end, ptr, spec);
 	case 'N':
 		return netdev_bits(buf, end, ptr, fmt);
-- 
2.13.6

Re: [PATCH v4 2/9] vsprintf: Consistent %pK handling for kptr_restrict == 0

[Sergey Senozhatsky]

On (04/04/18 10:58), Petr Mladek wrote:
>
> restricted_pointer() pretends that it prints the address when kptr_restrict
> is set to zero. But it is never called in this situation. Instead,
> pointer() falls back to ptr_to_id() and hashes the pointer.
> 
> This patch removes the potential confusion. klp_restrict is checked only
> in restricted_pointer().
> 
> It should actually fix a small race when the address might get printed
> unhashed:

Early morning, didn't have my coffee yet [like really didn't].

But I don't see how you "fix" a race. "echo 0" might still be called
later than switch().

[..]
> @@ -1426,8 +1427,8 @@ char *restricted_pointer(char *buf, char *end, const void *ptr,
>  
>  	switch (kptr_restrict) {
>  	case 0:
> -		/* Always print %pK values */
> -		break;
> +		/* Handle as %p, hash and do _not_ leak addresses. */
> +		return ptr_to_id(buf, end, ptr, spec);

>From "Always print pK values" to "Always print hashed values"... Do we need
%pK then? You probably need to update printk-formats.rst as well.

	-ss

Re: [PATCH v4 2/9] vsprintf: Consistent %pK handling for kptr_restrict == 0

[Petr Mladek]

On Thu 2018-04-05 08:10:14, Sergey Senozhatsky wrote:
> On (04/04/18 10:58), Petr Mladek wrote:
> >
> > restricted_pointer() pretends that it prints the address when kptr_restrict
> > is set to zero. But it is never called in this situation. Instead,
> > pointer() falls back to ptr_to_id() and hashes the pointer.
> > 
> > This patch removes the potential confusion. klp_restrict is checked only
> > in restricted_pointer().
> > 
> > It should actually fix a small race when the address might get printed
> > unhashed:
> 
> Early morning, didn't have my coffee yet [like really didn't].
> 
> But I don't see how you "fix" a race. "echo 0" might still be called
> later than switch().

I assume that switch() is safe enough and we always have to take one
path there. The old code allowed to print the original pointer without
the capability check (case 0). The new code does:

    case 0: print hashed pointer
    case 1: keep original pointer only with sufficient privileges
    case 2:
    default: set NULL

The old code expected that it would never use case 0. But it was
possible because of the race.


> [..]
> > @@ -1426,8 +1427,8 @@ char *restricted_pointer(char *buf, char *end, const void *ptr,
> >  
> >  	switch (kptr_restrict) {
> >  	case 0:
> > -		/* Always print %pK values */
> > -		break;
> > +		/* Handle as %p, hash and do _not_ leak addresses. */
> > +		return ptr_to_id(buf, end, ptr, spec);
> 
> >From "Always print pK values" to "Always print hashed values"... Do we need
> %pK then? You probably need to update printk-formats.rst as well.

It is already correctly documented in Documentation/sysctl/kernel.txt.
printk-formats.rst uses reference to it.

Best Regards,
Petr

Re: [PATCH v4 2/9] vsprintf: Consistent %pK handling for kptr_restrict == 0

[Andy Shevchenko]

On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:
> restricted_pointer() pretends that it prints the address when
> kptr_restrict
> is set to zero. But it is never called in this situation. Instead,
> pointer() falls back to ptr_to_id() and hashes the pointer.
> 
> This patch removes the potential confusion. klp_restrict is checked
> only
> in restricted_pointer().
> 


>  /* Maps a pointer to a 32 bit unique identifier. */
> -static char *ptr_to_id(char *buf, char *end, void *ptr, struct
> printf_spec spec)
> +static char *ptr_to_id(char *buf, char *end,
> +		       const void *ptr, struct printf_spec spec)

I don't think this change belongs to the patch.


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

Re: [PATCH v4 2/9] vsprintf: Consistent %pK handling for kptr_restrict == 0

[Petr Mladek]

On Thu 2018-04-05 16:04:45, Andy Shevchenko wrote:
> On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:
> > restricted_pointer() pretends that it prints the address when
> > kptr_restrict
> > is set to zero. But it is never called in this situation. Instead,
> > pointer() falls back to ptr_to_id() and hashes the pointer.
> > 
> > This patch removes the potential confusion. klp_restrict is checked
> > only
> > in restricted_pointer().
> > 
> 
> 
> >  /* Maps a pointer to a 32 bit unique identifier. */
> > -static char *ptr_to_id(char *buf, char *end, void *ptr, struct
> > printf_spec spec)
> > +static char *ptr_to_id(char *buf, char *end,
> > +		       const void *ptr, struct printf_spec spec)
> 
> I don't think this change belongs to the patch.

The const should have been there from the beginning. I have found it
because this patch added a call to ptr_to_id() which had the const
and compiler warned about cast problems.

IMHO, it is rather cosmetic change.

Best Regards,
Petr

Re: [PATCH v4 2/9] vsprintf: Consistent %pK handling for kptr_restrict == 0

[Andy Shevchenko]

On Thu, 2018-04-05 at 16:46 +0200, Petr Mladek wrote:
> On Thu 2018-04-05 16:04:45, Andy Shevchenko wrote:
> > On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:
> > > restricted_pointer() pretends that it prints the address when
> > > kptr_restrict
> > > is set to zero. But it is never called in this situation. Instead,
> > > pointer() falls back to ptr_to_id() and hashes the pointer.
> > > 
> > > This patch removes the potential confusion. klp_restrict is
> > > checked
> > > only
> > > in restricted_pointer().
> > > 
> > 
> > 
> > >  /* Maps a pointer to a 32 bit unique identifier. */
> > > -static char *ptr_to_id(char *buf, char *end, void *ptr, struct
> > > printf_spec spec)
> > > +static char *ptr_to_id(char *buf, char *end,
> > > +		       const void *ptr, struct printf_spec spec)
> > 
> > I don't think this change belongs to the patch.
> 
> The const should have been there from the beginning. I have found it
> because this patch added a call to ptr_to_id() which had the const
> and compiler warned about cast problems.

So, why not to do a separate patch with clear intention?

> IMHO, it is rather cosmetic change.

>From my experience I'm afraid of cosmetic changes in the patches which
might focus out attention on real fix.

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

Re: [PATCH v4 2/9] vsprintf: Consistent %pK handling for kptr_restrict == 0

[Petr Mladek]

On Sat 2018-04-07 17:08:18, Andy Shevchenko wrote:
> On Thu, 2018-04-05 at 16:46 +0200, Petr Mladek wrote:
> > On Thu 2018-04-05 16:04:45, Andy Shevchenko wrote:
> > > On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:
> > > > restricted_pointer() pretends that it prints the address when
> > > > kptr_restrict
> > > > is set to zero. But it is never called in this situation. Instead,
> > > > pointer() falls back to ptr_to_id() and hashes the pointer.
> > > > 
> > > > This patch removes the potential confusion. klp_restrict is
> > > > checked
> > > > only
> > > > in restricted_pointer().
> > > > 
> > > 
> > > 
> > > >  /* Maps a pointer to a 32 bit unique identifier. */
> > > > -static char *ptr_to_id(char *buf, char *end, void *ptr, struct
> > > > printf_spec spec)
> > > > +static char *ptr_to_id(char *buf, char *end,
> > > > +		       const void *ptr, struct printf_spec spec)
> > > 
> > > I don't think this change belongs to the patch.
> > 
> > The const should have been there from the beginning. I have found it
> > because this patch added a call to ptr_to_id() which had the const
> > and compiler warned about cast problems.
> 
> So, why not to do a separate patch with clear intention?

If you insist I could do it as separate patch.


> > IMHO, it is rather cosmetic change.
> 
> >From my experience I'm afraid of cosmetic changes in the patches which
> might focus out attention on real fix.

I would understand this if it was part of a large patch that changed
complex chain of functions. But this patch touched 5 lines. The const
is added into static function that is almost leaf and was called
only from a single location.

Best Regards,
Petr

Re: [PATCH v4 2/9] vsprintf: Consistent %pK handling for kptr_restrict == 0

[Andy Shevchenko]

On Mon, 2018-04-09 at 14:05 +0200, Petr Mladek wrote:
> On Sat 2018-04-07 17:08:18, Andy Shevchenko wrote:
> > On Thu, 2018-04-05 at 16:46 +0200, Petr Mladek wrote:
> > > On Thu 2018-04-05 16:04:45, Andy Shevchenko wrote:
> > > > On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:

> > > > >  /* Maps a pointer to a 32 bit unique identifier. */
> > > > > -static char *ptr_to_id(char *buf, char *end, void *ptr,
> > > > > struct
> > > > > printf_spec spec)
> > > > > +static char *ptr_to_id(char *buf, char *end,
> > > > > +		       const void *ptr, struct printf_spec
> > > > > spec)
> > > > 
> > > > I don't think this change belongs to the patch.
> > > 
> > > The const should have been there from the beginning. I have found
> > > it
> > > because this patch added a call to ptr_to_id() which had the const
> > > and compiler warned about cast problems.
> > 
> > So, why not to do a separate patch with clear intention?
> 
> If you insist I could do it as separate patch.

I believe, that logical changes are easier to review and understand when
it's not mixed with other changes.

> > > IMHO, it is rather cosmetic change.
> > > From my experience I'm afraid of cosmetic changes in the patches
> > > which
> > 
> > might focus out attention on real fix.
> 
> I would understand this if it was part of a large patch that changed
> complex chain of functions. But this patch touched 5 lines. The const
> is added into static function that is almost leaf and was called
> only from a single location.

But it's up to you.

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

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

[Petr Mladek]

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

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

This patch does not change the existing behavior.

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

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 9ade2c4f21ba..dd71738d7a09 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -582,14 +582,11 @@ char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
 }
 
 static noinline_for_stack
-char *string(char *buf, char *end, const char *s, struct printf_spec spec)
+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)
-		s = "(null)";
-
 	while (lim--) {
 		char c = *s++;
 		if (!c)
@@ -602,6 +599,16 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 	return widen_string(buf, len, end, spec);
 }
 
+static noinline_for_stack
+char *string(char *buf, char *end, const char *s,
+		   struct printf_spec spec)
+{
+	if ((unsigned long)s < PAGE_SIZE)
+		s = "(null)";
+
+	return __string(buf, end, s, spec);
+}
+
 static bool have_filled_random_ptr_key __read_mostly;
 static siphash_key_t ptr_key __read_mostly;
 
@@ -646,7 +653,7 @@ static char *ptr_to_id(char *buf, char *end,
 	if (unlikely(!have_filled_random_ptr_key)) {
 		spec.field_width = default_width;
 		/* string length must be less than default_width */
-		return string(buf, end, "(ptrval)", spec);
+		return __string(buf, end, "(ptrval)", spec);
 	}
 
 #ifdef CONFIG_64BIT
@@ -755,7 +762,7 @@ char *symbol_string(char *buf, char *end, void *ptr,
 	else
 		sprint_symbol_no_offset(sym, value);
 
-	return string(buf, end, sym, spec);
+	return __string(buf, end, sym, spec);
 #else
 	return special_hex_number(buf, end, value, sizeof(void *));
 #endif
@@ -821,27 +828,27 @@ char *resource_string(char *buf, char *end, struct resource *res,
 
 	*p++ = '[';
 	if (res->flags & IORESOURCE_IO) {
-		p = string(p, pend, "io  ", str_spec);
+		p = __string(p, pend, "io  ", str_spec);
 		specp = &io_spec;
 	} else if (res->flags & IORESOURCE_MEM) {
-		p = string(p, pend, "mem ", str_spec);
+		p = __string(p, pend, "mem ", str_spec);
 		specp = &mem_spec;
 	} else if (res->flags & IORESOURCE_IRQ) {
-		p = string(p, pend, "irq ", str_spec);
+		p = __string(p, pend, "irq ", str_spec);
 		specp = &dec_spec;
 	} else if (res->flags & IORESOURCE_DMA) {
-		p = string(p, pend, "dma ", str_spec);
+		p = __string(p, pend, "dma ", str_spec);
 		specp = &dec_spec;
 	} else if (res->flags & IORESOURCE_BUS) {
-		p = string(p, pend, "bus ", str_spec);
+		p = __string(p, pend, "bus ", str_spec);
 		specp = &bus_spec;
 	} else {
-		p = string(p, pend, "??? ", str_spec);
+		p = __string(p, pend, "??? ", str_spec);
 		specp = &mem_spec;
 		decode = 0;
 	}
 	if (decode && res->flags & IORESOURCE_UNSET) {
-		p = string(p, pend, "size ", str_spec);
+		p = __string(p, pend, "size ", str_spec);
 		p = number(p, pend, resource_size(res), *specp);
 	} else {
 		p = number(p, pend, res->start, *specp);
@@ -852,21 +859,21 @@ char *resource_string(char *buf, char *end, struct resource *res,
 	}
 	if (decode) {
 		if (res->flags & IORESOURCE_MEM_64)
-			p = string(p, pend, " 64bit", str_spec);
+			p = __string(p, pend, " 64bit", str_spec);
 		if (res->flags & IORESOURCE_PREFETCH)
-			p = string(p, pend, " pref", str_spec);
+			p = __string(p, pend, " pref", str_spec);
 		if (res->flags & IORESOURCE_WINDOW)
-			p = string(p, pend, " window", str_spec);
+			p = __string(p, pend, " window", str_spec);
 		if (res->flags & IORESOURCE_DISABLED)
-			p = string(p, pend, " disabled", str_spec);
+			p = __string(p, pend, " disabled", str_spec);
 	} else {
-		p = string(p, pend, " flags ", str_spec);
+		p = __string(p, pend, " flags ", str_spec);
 		p = number(p, pend, res->flags, flag_spec);
 	}
 	*p++ = ']';
 	*p = '\0';
 
-	return string(buf, end, sym, spec);
+	return __string(buf, end, sym, spec);
 }
 
 static noinline_for_stack
@@ -1037,7 +1044,7 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
 	}
 	*p = '\0';
 
-	return string(buf, end, mac_addr, spec);
+	return __string(buf, end, mac_addr, spec);
 }
 
 static noinline_for_stack
@@ -1200,7 +1207,7 @@ char *ip6_addr_string(char *buf, char *end, const u8 *addr,
 	else
 		ip6_string(ip6_addr, addr, fmt);
 
-	return string(buf, end, ip6_addr, spec);
+	return __string(buf, end, ip6_addr, spec);
 }
 
 static noinline_for_stack
@@ -1211,7 +1218,7 @@ char *ip4_addr_string(char *buf, char *end, const u8 *addr,
 
 	ip4_string(ip4_addr, addr, fmt);
 
-	return string(buf, end, ip4_addr, spec);
+	return __string(buf, end, ip4_addr, spec);
 }
 
 static noinline_for_stack
@@ -1273,7 +1280,7 @@ char *ip6_addr_string_sa(char *buf, char *end, const struct sockaddr_in6 *sa,
 	}
 	*p = '\0';
 
-	return string(buf, end, ip6_addr, spec);
+	return __string(buf, end, ip6_addr, spec);
 }
 
 static noinline_for_stack
@@ -1308,7 +1315,7 @@ char *ip4_addr_string_sa(char *buf, char *end, const struct sockaddr_in *sa,
 	}
 	*p = '\0';
 
-	return string(buf, end, ip4_addr, spec);
+	return __string(buf, end, ip4_addr, spec);
 }
 
 static noinline_for_stack
@@ -1409,7 +1416,7 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 
 	*p = 0;
 
-	return string(buf, end, uuid, spec);
+	return __string(buf, end, uuid, spec);
 }
 
 int kptr_restrict __read_mostly;
@@ -1437,7 +1444,7 @@ char *restricted_pointer(char *buf, char *end, const void *ptr,
 		 * because its test for CAP_SYSLOG would be meaningless.
 		 */
 		if (in_irq() || in_serving_softirq() || in_nmi())
-			return string(buf, end, "pK-error", spec);
+			return __string(buf, end, "pK-error", spec);
 
 		/*
 		 * Only print the real pointer value if the current
@@ -1613,13 +1620,13 @@ char *device_node_gen_full_name(const struct device_node *np, char *buf, char *e
 
 	/* special case for root node */
 	if (!parent)
-		return string(buf, end, "/", strspec);
+		return __string(buf, end, "/", strspec);
 
 	for (depth = 0; parent->parent; depth++)
 		parent = parent->parent;
 
 	for ( ; depth >= 0; depth--) {
-		buf = string(buf, end, "/", strspec);
+		buf = __string(buf, end, "/", strspec);
 		buf = string(buf, end, device_node_name_for_depth(np, depth),
 			     strspec);
 	}
@@ -1647,10 +1654,10 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	str_spec.field_width = -1;
 
 	if (!IS_ENABLED(CONFIG_OF))
-		return string(buf, end, "(!OF)", spec);
+		return __string(buf, end, "(!OF)", spec);
 
 	if ((unsigned long)dn < PAGE_SIZE)
-		return string(buf, end, "(null)", spec);
+		return __string(buf, end, "(null)", spec);
 
 	/* simple case without anything any more format specifiers */
 	fmt++;
@@ -1686,7 +1693,7 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 			tbuf[2] = of_node_check_flag(dn, OF_POPULATED) ? 'P' : '-';
 			tbuf[3] = of_node_check_flag(dn, OF_POPULATED_BUS) ? 'B' : '-';
 			tbuf[4] = 0;
-			buf = string(buf, end, tbuf, str_spec);
+			buf = __string(buf, end, tbuf, str_spec);
 			break;
 		case 'c':	/* major compatible string */
 			ret = of_property_read_string(dn, "compatible", &p);
@@ -1697,10 +1704,10 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 			has_mult = false;
 			of_property_for_each_string(dn, "compatible", prop, p) {
 				if (has_mult)
-					buf = string(buf, end, ",", str_spec);
-				buf = string(buf, end, "\"", str_spec);
+					buf = __string(buf, end, ",", str_spec);
+				buf = __string(buf, end, "\"", str_spec);
 				buf = string(buf, end, p, str_spec);
-				buf = string(buf, end, "\"", str_spec);
+				buf = __string(buf, end, "\"", str_spec);
 
 				has_mult = true;
 			}
@@ -1857,7 +1864,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		 */
 		if (spec.field_width == -1)
 			spec.field_width = default_width;
-		return string(buf, end, "(null)", spec);
+		return __string(buf, end, "(null)", spec);
 	}
 
 	switch (*fmt) {
@@ -1913,7 +1920,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			case AF_INET6:
 				return ip6_addr_string_sa(buf, end, &sa->v6, spec, fmt);
 			default:
-				return string(buf, end, "(invalid address)", spec);
+				return __string(buf, end, "(invalid address)", spec);
 			}}
 		}
 		break;
-- 
2.13.6

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

[Rasmus Villemoes]

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

Please leave string() alone, except for moving the < PAGE_SIZE check to
a new helper checked_string (feel free to find a better name), and use
checked_string for handling %s and possibly the few other cases where
we're passing a user-supplied pointer. That avoids cluttering the entire
file with double-underscore calls, and e.g. in the %pO case, it's easier
to understand why one uses two different *string() helpers if the name
of one somehow conveys how it is different from the other.

Rasmus

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

[Petr Mladek]

On Thu 2018-04-05 15:30:51, Rasmus Villemoes wrote:
> On 2018-04-04 10:58, Petr Mladek wrote:
> > We are going to check the address using probe_kernel_address(). It will
> > be more expensive and it does not make sense for well known address.
> > 
> > This patch splits the string() function. The variant without the check
> > is then used on locations that handle string constants or strings defined
> > as local variables.
> > 
> > This patch does not change the existing behavior.
> 
> Please leave string() alone, except for moving the < PAGE_SIZE check to
> a new helper checked_string (feel free to find a better name), and use
> checked_string for handling %s and possibly the few other cases where
> we're passing a user-supplied pointer. That avoids cluttering the entire
> file with double-underscore calls, and e.g. in the %pO case, it's easier
> to understand why one uses two different *string() helpers if the name
> of one somehow conveys how it is different from the other.

I understand your reasoning. I thought about exactly this as well.
My problem is that string() will then be unsafe. It might be dangerous
when porting patches.

This is why I wanted a different name for the variant without the
check. But I was not able to come up with anything short and clear
at the same time.

Is _string() really that bad? I think that it is a rather common
practice to use _func() for functions that are less safe than func()
variants. People should use _func() variants with care and this is
what we want here.

In addition, it is an internal API. IMHO, only few people do changes
there. They will get used to it quickly. Which is not true for people
that might need to port patches.

Best Regards,
Petr

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

[Andy Shevchenko]

On Fri, 2018-04-06 at 11:15 +0200, Petr Mladek wrote:
> On Thu 2018-04-05 15:30:51, Rasmus Villemoes wrote:
> > On 2018-04-04 10:58, Petr Mladek wrote:
> > > We are going to check the address using probe_kernel_address(). It
> > > will
> > > be more expensive and it does not make sense for well known
> > > address.
> > > 
> > > This patch splits the string() function. The variant without the
> > > check
> > > is then used on locations that handle string constants or strings
> > > defined
> > > as local variables.
> > > 
> > > This patch does not change the existing behavior.
> > 
> > Please leave string() alone, except for moving the < PAGE_SIZE check
> > to
> > a new helper checked_string (feel free to find a better name), and
> > use
> > checked_string for handling %s and possibly the few other cases
> > where
> > we're passing a user-supplied pointer. That avoids cluttering the
> > entire
> > file with double-underscore calls, and e.g. in the %pO case, it's
> > easier
> > to understand why one uses two different *string() helpers if the
> > name
> > of one somehow conveys how it is different from the other.
> 
> I understand your reasoning. I thought about exactly this as well.
> My problem is that string() will then be unsafe. It might be dangerous
> when porting patches.

I agree with Rasmus, and your argument here from my point of view kinda
weak. Are we really going to backport this patches? Why? We lived w/o
them for a long time. What's changed now?

> Is _string() really that bad? 

I would think so.

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

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

[Petr Mladek]

On Sat 2018-04-07 17:12:35, Andy Shevchenko wrote:
> On Fri, 2018-04-06 at 11:15 +0200, Petr Mladek wrote:
> > On Thu 2018-04-05 15:30:51, Rasmus Villemoes wrote:
> > > On 2018-04-04 10:58, Petr Mladek wrote:
> > > > We are going to check the address using probe_kernel_address(). It
> > > > will
> > > > be more expensive and it does not make sense for well known
> > > > address.
> > > > 
> > > > This patch splits the string() function. The variant without the
> > > > check
> > > > is then used on locations that handle string constants or strings
> > > > defined
> > > > as local variables.
> > > > 
> > > > This patch does not change the existing behavior.
> > > 
> > > Please leave string() alone, except for moving the < PAGE_SIZE check
> > > to
> > > a new helper checked_string (feel free to find a better name), and
> > > use
> > > checked_string for handling %s and possibly the few other cases
> > > where
> > > we're passing a user-supplied pointer. That avoids cluttering the
> > > entire
> > > file with double-underscore calls, and e.g. in the %pO case, it's
> > > easier
> > > to understand why one uses two different *string() helpers if the
> > > name
> > > of one somehow conveys how it is different from the other.
> > 
> > I understand your reasoning. I thought about exactly this as well.
> > My problem is that string() will then be unsafe. It might be dangerous
> > when porting patches.
> 
> I agree with Rasmus, and your argument here from my point of view kinda
> weak. Are we really going to backport this patches? Why? We lived w/o
> them for a long time. What's changed now?

Someone might have out-of-tree patch that adds yet another format
specifier. It might call string() that checks for (null) now but
it it won't if we rename it as you suggest. People used to safe
string() might miss this when the patch is send upstream for
inclusion, ...


> > Is _string() really that bad? 
> 
> I would think so.

OK, I am going to use valid_string() instead of _string().
Feel free to suggest anything better. Just please no bike-shedding.

Best Regards,
Petr

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

[Andy Shevchenko]

On Mon, 2018-04-09 at 14:19 +0200, Petr Mladek wrote:
> On Sat 2018-04-07 17:12:35, Andy Shevchenko wrote:
> > On Fri, 2018-04-06 at 11:15 +0200, Petr Mladek wrote:
> > > On Thu 2018-04-05 15:30:51, Rasmus Villemoes wrote:
> > > > On 2018-04-04 10:58, Petr Mladek wrote:

> > > > > 
> > > > Please leave string() alone, except for moving the < PAGE_SIZE
> > > > check
> > > > to
> > > > a new helper checked_string (feel free to find a better name),
> > > > and
> > > > use
> > > > checked_string for handling %s and possibly the few other cases
> > > > where
> > > > we're passing a user-supplied pointer. That avoids cluttering
> > > > the
> > > > entire
> > > > file with double-underscore calls, and e.g. in the %pO case,
> > > > it's
> > > > easier
> > > > to understand why one uses two different *string() helpers if
> > > > the
> > > > name
> > > > of one somehow conveys how it is different from the other.
> > > 
> > > I understand your reasoning. I thought about exactly this as well.
> > > My problem is that string() will then be unsafe. It might be
> > > dangerous
> > > when porting patches.
> > 
> > I agree with Rasmus, and your argument here from my point of view
> > kinda
> > weak. Are we really going to backport this patches? Why? We lived
> > w/o
> > them for a long time. What's changed now?
> 
> Someone might have out-of-tree patch that adds yet another format
> specifier. It might call string() that checks for (null) now but
> it it won't if we rename it as you suggest. People used to safe
> string() might miss this when the patch is send upstream for
> inclusion, ...

This is even weaker argument. Sorry, but I don't care about out-of-tree
core patches. If they have them, they are doing completely wrong, or the
patches are crappy.

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

[PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

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

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

The best solution seems to be in flags_string(). It does not print any
misleading value. Instead it calls WARN_ONCE() describing the unknown
specifier. Therefore it clearly shows the problem and helps to find it.

Note that WARN_ONCE() used to cause recursive printk(). But it is safe
now because vscnprintf() is called in printk_safe context from
vprintk_emit().

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

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index dd71738d7a09..5a0d01846a11 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1484,9 +1484,9 @@ char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt)
 		size = sizeof(netdev_features_t);
 		break;
 	default:
-		num = (unsigned long)addr;
-		size = sizeof(unsigned long);
-		break;
+		WARN_ONCE(1, "Unsupported pointer format specifier: %%pN%c\n",
+			  fmt[1]);
+		return buf;
 	}
 
 	return special_hex_number(buf, end, num, size);
@@ -1517,7 +1517,12 @@ 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)) {
+		WARN_ONCE(1, "Unsupported pointer format specifier: %%pC\n");
+		return buf;
+	}
+
+	if (!clk)
 		return string(buf, end, NULL, spec);
 
 	switch (fmt[1]) {
@@ -1529,7 +1534,8 @@ char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
 #ifdef CONFIG_COMMON_CLK
 		return string(buf, end, __clk_get_name(clk), spec);
 #else
-		return special_hex_number(buf, end, (unsigned long)clk, sizeof(unsigned long));
+		WARN_ONCE(1, "Unsupported pointer format specifier: %%pC\n");
+		return buf;
 #endif
 	}
 }
@@ -1593,7 +1599,8 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)
 		names = gfpflag_names;
 		break;
 	default:
-		WARN_ONCE(1, "Unsupported flags modifier: %c\n", fmt[1]);
+		WARN_ONCE(1, "Unsupported pointer format specifier: %%pG%c\n",
+			  fmt[1]);
 		return buf;
 	}
 
-- 
2.13.6

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Rasmus Villemoes]

On 2018-04-04 10:58, Petr Mladek wrote:
> There are few printk formats that make sense only with two or more
> specifiers. Also some specifiers make sense only when a kernel feature
> is enabled.
> 
> The handling of unknown specifiers is strange, inconsistent, and
> even leaking the address. For example, netdev_bits() prints the
> non-hashed pointer value or clock() prints "(null)".
> 
> The best solution seems to be in flags_string(). It does not print any
> misleading value. Instead it calls WARN_ONCE() describing the unknown
> specifier. Therefore it clearly shows the problem and helps to find it.
>

I'm not sure it's actually worth WARNing about the unknown variants
since we have static analysis (at least checkpatch and smatch) that
should catch that. Even just git grep -1 -E '%p"$' finds %pt and %po
which should get fixed before somebody claims those extensions.

But, I don't disagree with trying to fix up the inconsistency, and
certainly not with fixing netdev_bits(), but it seems you've missed that
e.g. the "case: 'g'" is completely compiled out for !CONFIG_BLOCK.
There's also %pOF which is effectively disabled for !CONFIG_OF (which
obviously makes sense), but with yet a different fallback behaviour.

Hm. I think we should somehow distinguish between the cases of "%po" and
"%pNX", i.e. specifiers/variants that are always bogus, and the cases of
a %pOF or %pC that somehow happens even though nobody should have a
struct device_node* or struct clk* to pass.

Rasmus

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Joe Perches]

On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> Even just git grep -1 -E '%p"$' finds %pt and %po
> which should get fixed before somebody claims those extensions.

Neither %pt nor %po is used in a vsprintf
in the kernel.

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Joe Perches]

On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > Even just git grep -1 -E '%p"$' finds %pt and %po
> > which should get fixed before somebody claims those extensions.
> 
> Neither %pt nor %po is used in a vsprintf
> in the kernel.

Nope, you are right, both are defectively used in the
kernel via string concatenation.

Also there's a missing space in a concatenation adjacent.

---
 arch/s390/kernel/perf_cpum_sf.c           | 4 +---
 drivers/scsi/megaraid/megaraid_sas_base.c | 7 ++-----
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/arch/s390/kernel/perf_cpum_sf.c b/arch/s390/kernel/perf_cpum_sf.c
index 1c9ddd7aa5ec..458b8f321043 100644
--- a/arch/s390/kernel/perf_cpum_sf.c
+++ b/arch/s390/kernel/perf_cpum_sf.c
@@ -212,9 +212,7 @@ static int realloc_sampling_buffer(struct sf_buffer *sfb,
 	 * the sampling buffer origin.
 	 */
 	if (sfb->sdbt != get_next_sdbt(tail)) {
-		debug_sprintf_event(sfdbg, 3, "realloc_sampling_buffer: "
-				    "sampling buffer is not linked: origin=%p"
-				    "tail=%p\n",
+		debug_sprintf_event(sfdbg, 3, "%s: sampling buffer is not linked: origin=%p tail=%p\n",
 				    (void *) sfb->sdbt, (void *) tail);
 		return -EINVAL;
 	}
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index b89c6e6c0589..56f1c8132664 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -3546,14 +3546,11 @@ megasas_internal_reset_defer_cmds(struct megasas_instance *instance)
 	for (i = 0; i < max_cmd; i++) {
 		cmd = instance->cmd_list[i];
 		if (cmd->sync_cmd == 1 || cmd->scmd) {
-			dev_notice(&instance->pdev->dev, "moving cmd[%d]:%p:%d:%p"
-					"on the defer queue as internal\n",
+			dev_notice(&instance->pdev->dev, "moving cmd[%d]:%p:%d:%p on the defer queue as internal\n",
 				defer_index, cmd, cmd->sync_cmd, cmd->scmd);
 
 			if (!list_empty(&cmd->list)) {
-				dev_notice(&instance->pdev->dev, "ERROR while"
-					" moving this cmd:%p, %d %p, it was"
-					"discovered on some list?\n",
+				dev_notice(&instance->pdev->dev, "ERROR while moving this cmd:%p, %d %p, it was discovered on some list?\n",
 					cmd, cmd->sync_cmd, cmd->scmd);
 
 				list_del_init(&cmd->list);

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

On Thu 2018-04-05 16:55:35, Joe Perches wrote:
> On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> > On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > > Even just git grep -1 -E '%p"$' finds %pt and %po
> > > which should get fixed before somebody claims those extensions.
> > 
> > Neither %pt nor %po is used in a vsprintf
> > in the kernel.
> 
> Nope, you are right, both are defectively used in the
> kernel via string concatenation.

Ah, I see.

> Also there's a missing space in a concatenation adjacent.


> ---
>  arch/s390/kernel/perf_cpum_sf.c           | 4 +---
>  drivers/scsi/megaraid/megaraid_sas_base.c | 7 ++-----
>  2 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/s390/kernel/perf_cpum_sf.c b/arch/s390/kernel/perf_cpum_sf.c
> index 1c9ddd7aa5ec..458b8f321043 100644
> --- a/arch/s390/kernel/perf_cpum_sf.c
> +++ b/arch/s390/kernel/perf_cpum_sf.c
> @@ -212,9 +212,7 @@ static int realloc_sampling_buffer(struct sf_buffer *sfb,
>  	 * the sampling buffer origin.
>  	 */
>  	if (sfb->sdbt != get_next_sdbt(tail)) {
> -		debug_sprintf_event(sfdbg, 3, "realloc_sampling_buffer: "
> -				    "sampling buffer is not linked: origin=%p"
> -				    "tail=%p\n",
> +		debug_sprintf_event(sfdbg, 3, "%s: sampling buffer is not linked: origin=%p tail=%p\n",
					       ^^
I guess that you wanted to add __func__ as the next parameter?

Otherwise, the changes make perfect sense. Are you going to send
them as proper patches, please?

Best Regards,
Petr

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Rasmus Villemoes]

On 2018-04-06 13:43, Petr Mladek wrote:
> On Thu 2018-04-05 16:55:35, Joe Perches wrote:
>> On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
>>> On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
>>>> Even just git grep -1 -E '%p"$' finds %pt and %po
>>>> which should get fixed before somebody claims those extensions.
>>>
>>> Neither %pt nor %po is used in a vsprintf
>>> in the kernel.
>>
>> Nope, you are right, both are defectively used in the
>> kernel via string concatenation.
> 
> Ah, I see.

Yes, that was the point of that particular grep pattern with context. I
sent patches three years ago and several pings, but despite an email
telling me it would be picked up "for the next release" nothing has
happened, so I've just been waiting for someone to implement a %po.

> Otherwise, the changes make perfect sense. Are you going to send
> them as proper patches, please?

Good luck. Unless Petr will be taking them through the printk tree?

Rasmus

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Joe Perches]

On Fri, 2018-04-06 at 15:17 +0200, Rasmus Villemoes wrote:
> On 2018-04-06 13:43, Petr Mladek wrote:
> > On Thu 2018-04-05 16:55:35, Joe Perches wrote:
> > > On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> > > > On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > > > > Even just git grep -1 -E '%p"$' finds %pt and %po
> > > > > which should get fixed before somebody claims those extensions.
> > > > 
> > > > Neither %pt nor %po is used in a vsprintf
> > > > in the kernel.
> > > 
> > > Nope, you are right, both are defectively used in the
> > > kernel via string concatenation.
> > 
> > Ah, I see.
> 
> Yes, that was the point of that particular grep pattern with context. I
> sent patches three years ago and several pings, but despite an email
> telling me it would be picked up "for the next release" nothing has
> happened, so I've just been waiting for someone to implement a %po.
> 
> > Otherwise, the changes make perfect sense. Are you going to send
> > them as proper patches, please?
> 
> Good luck. Unless Petr will be taking them through the printk tree?

As the trivial maintainer seems to use a leaky bucket algorithm
for most trivial patches, I'll just retry and send actual
patches through Andrew Morton as patches through his quilt tree
seem to get upstream faster.

And thanks Petr for noticing the missing __func__, I was just
mindlessly editing those two files as a placeholder for where
the missing spaces in the %p concatenations exist.

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

On Fri 2018-04-06 07:27:19, Joe Perches wrote:
> On Fri, 2018-04-06 at 15:17 +0200, Rasmus Villemoes wrote:
> > On 2018-04-06 13:43, Petr Mladek wrote:
> > > On Thu 2018-04-05 16:55:35, Joe Perches wrote:
> > > > On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> > > > > On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > > > > > Even just git grep -1 -E '%p"$' finds %pt and %po
> > > > > > which should get fixed before somebody claims those extensions.
> > > > > 
> > > > > Neither %pt nor %po is used in a vsprintf
> > > > > in the kernel.
> > > > 
> > > > Nope, you are right, both are defectively used in the
> > > > kernel via string concatenation.
> > > 
> > > Ah, I see.
> > 
> > Yes, that was the point of that particular grep pattern with context. I
> > sent patches three years ago and several pings, but despite an email
> > telling me it would be picked up "for the next release" nothing has
> > happened, so I've just been waiting for someone to implement a %po.
> > 
> > > Otherwise, the changes make perfect sense. Are you going to send
> > > them as proper patches, please?
> > 
> > Good luck. Unless Petr will be taking them through the printk tree?
> 
> As the trivial maintainer seems to use a leaky bucket algorithm
> for most trivial patches, I'll just retry and send actual
> patches through Andrew Morton as patches through his quilt tree
> seem to get upstream faster.

I could take them via printk.git. But Andrew is a good choice
as well.

Best Regards,
Petr

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Andy Shevchenko]

On Fri, 2018-04-06 at 15:17 +0200, Rasmus Villemoes wrote:
> On 2018-04-06 13:43, Petr Mladek wrote:
> > On Thu 2018-04-05 16:55:35, Joe Perches wrote:
> > > On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> > > > On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > > > > Even just git grep -1 -E '%p"$' finds %pt and %po
> > > > > which should get fixed before somebody claims those
> > > > > extensions.
> > > > 
> > > > Neither %pt nor %po is used in a vsprintf
> > > > in the kernel.
> > > 
> > > Nope, you are right, both are defectively used in the
> > > kernel via string concatenation.
> > 
> > Ah, I see.
> 
> Yes, that was the point of that particular grep pattern with context.
> I
> sent patches three years ago and several pings, but despite an email
> telling me it would be picked up "for the next release" nothing has
> happened, so I've just been waiting for someone to implement a %po.

It used to be for SCSI subsystem until Martin gets involved.
My record is 6 years for the patch made the upstream.

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

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Sergey Senozhatsky]

On (04/05/18 16:55), Joe Perches wrote:
> On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> > On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > > Even just git grep -1 -E '%p"$' finds %pt and %po
> > > which should get fixed before somebody claims those extensions.
> > 
> > Neither %pt nor %po is used in a vsprintf
> > in the kernel.
> 
> Nope, you are right, both are defectively used in the
> kernel via string concatenation.
> 
> Also there's a missing space in a concatenation adjacent.

Can we tweak checkpatch to catch such things?


Hm... *Probably* also wouldn't hurt if checkpatch can require at least
one character after pointer format specifiers:

	printk("string %p"          vs       printk("string %p "
	       " Object\n", ptr);                   "Object\n", ptr);

Especially if we can have a "potential" %px, like here

               dev_vdbg(&md->input->dev,
                       "%s: *axis=%02X(%d) size=%d max=%08X xy_data=%p"
                       " xy_data[%d]=%02X(%d) bofs=%d\n",

or here

       dev_vdbg(&md->input->dev,
               "%s: *axis=%02X(%d) size=%d max=%08X xy_data=%p"
               " xy_data[%d]=%02X(%d)\n",

Opinions?

	-ss

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Joe Perches]

On Sat, 2018-04-07 at 08:52 +0900, Sergey Senozhatsky wrote:
> On (04/05/18 16:55), Joe Perches wrote:
> > On Thu, 2018-04-05 at 16:45 -0700, Joe Perches wrote:
> > > On Thu, 2018-04-05 at 16:25 +0200, Rasmus Villemoes wrote:
> > > > Even just git grep -1 -E '%p"$' finds %pt and %po
> > > > which should get fixed before somebody claims those extensions.
> > > 
> > > Neither %pt nor %po is used in a vsprintf
> > > in the kernel.
> > 
> > Nope, you are right, both are defectively used in the
> > kernel via string concatenation.
> > 
> > Also there's a missing space in a concatenation adjacent.
> 
> Can we tweak checkpatch to catch such things?

Not really, no.

Adding regex logic for this is tricky at best
and probably not worth the effort because of
the various bits of patch contexts aren't
necessarily visible.

There are also concatenations like
	"foo" DEFINE "bar"
where DEFINE may not be visible in the patch
context and checkpatch is and likely will
remain just a limited regex checker.

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Sergey Senozhatsky]

Hi Joe,

On (04/06/18 16:59), Joe Perches wrote:
> > 
> > Can we tweak checkpatch to catch such things?
> 
> Not really, no.
> 
> Adding regex logic for this is tricky at best
> and probably not worth the effort because of
> the various bits of patch contexts aren't
> necessarily visible.

Agreed. I was more thinking about catching "... %p" and saying
that we'd rather prefer either "... %p," or "... %p " or "... %p\n".
Doesn't sound so complex, can probably catch something fishy one day
(or may be not), and more or less is visible to checkpatch. Well,
more or less...

> There are also concatenations like
> 	"foo" DEFINE "bar"
> where DEFINE may not be visible in the patch
> context and checkpatch is and likely will
> remain just a limited regex checker.

Right. One example might be XFS

	alert("%s: Bad regular inode %Lu, ptr "PTR_FMT,
				__func__, ip->i_ino, ip);

where PTR_FMT is

#ifdef DEBUG
# define PTR_FMT "%px"
#else
# define PTR_FMT "%p"
#endif

	-ss

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Joe Perches]

On Sat, 2018-04-07 at 09:33 +0900, Sergey Senozhatsky wrote:
> Hi Joe,
> 
> On (04/06/18 16:59), Joe Perches wrote:
> > > 
> > > Can we tweak checkpatch to catch such things?
> > 
> > Not really, no.
> > 
> > Adding regex logic for this is tricky at best
> > and probably not worth the effort because of
> > the various bits of patch contexts aren't
> > necessarily visible.
> 
> Agreed. I was more thinking about catching "... %p" and saying
> that we'd rather prefer either "... %p," or "... %p " or "... %p\n".
> Doesn't sound so complex, can probably catch something fishy one day
> (or may be not), and more or less is visible to checkpatch. Well,
> more or less...

This finds the current two bad uses in addition to
the existing similar message for string concatenation
without a space char between concatenated fragments.

For example:

WARNING: break quoted strings at a space character
#3550: FILE: drivers/scsi/megaraid/megaraid_sas_base.c:3550:
+			dev_notice(&instance->pdev->dev, "moving cmd[%d]:%p:%d:%p"
+					"on the defer queue as internal\n",

WARNING: vsprintf %p<extension> string concatenation
#3550: FILE: drivers/scsi/megaraid/megaraid_sas_base.c:3550:
+			dev_notice(&instance->pdev->dev, "moving cmd[%d]:%p:%d:%p"
+					"on the defer queue as internal\n",

I think the new message is not that useful really as the
existing warning is probably enough.

---
 scripts/checkpatch.pl | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index eb534d48140e..a0e43232431e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5313,6 +5313,12 @@ sub process {
 			     "break quoted strings at a space character\n" . $hereprev);
 		}
 
+# check for vsprintf pointer extension concatenation
+		if ($prevrawline =~ /\%p"\s*$/ && $rawline =~ /^\+\s*"\w/) {
+			WARN('POINTER_CONCATENATION',
+			     "vsprintf %p<extension> string concatenation\n" . $hereprev);
+		}
+
 # check for an embedded function name in a string when the function is known
 # This does not work very well for -f --file checking as it depends on patch
 # context providing the function name or a single line form for in-file

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Sergey Senozhatsky]

On (04/06/18 18:00), Joe Perches wrote:
[..]
> This finds the current two bad uses in addition to
> the existing similar message for string concatenation
> without a space char between concatenated fragments.
> 
> For example:
> 
> WARNING: break quoted strings at a space character
> #3550: FILE: drivers/scsi/megaraid/megaraid_sas_base.c:3550:
> +			dev_notice(&instance->pdev->dev, "moving cmd[%d]:%p:%d:%p"
> +					"on the defer queue as internal\n",
> 
> WARNING: vsprintf %p<extension> string concatenation
> #3550: FILE: drivers/scsi/megaraid/megaraid_sas_base.c:3550:
> +			dev_notice(&instance->pdev->dev, "moving cmd[%d]:%p:%d:%p"
> +					"on the defer queue as internal\n",
> 
> I think the new message is not that useful really as the
> existing warning is probably enough.

Oh, so we already have it... Didn't know that. Yes, I think the existing
one is good enough. Thanks for the pointers.

	-ss

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

On Thu 2018-04-05 16:25:41, Rasmus Villemoes wrote:
> On 2018-04-04 10:58, Petr Mladek wrote:
> > There are few printk formats that make sense only with two or more
> > specifiers. Also some specifiers make sense only when a kernel feature
> > is enabled.
> > 
> > The handling of unknown specifiers is strange, inconsistent, and
> > even leaking the address. For example, netdev_bits() prints the
> > non-hashed pointer value or clock() prints "(null)".
> > 
> > The best solution seems to be in flags_string(). It does not print any
> > misleading value. Instead it calls WARN_ONCE() describing the unknown
> > specifier. Therefore it clearly shows the problem and helps to find it.
> >
> I'm not sure it's actually worth WARNing about the unknown variants
> since

I agree that WARN() looks like an overkill for this type of error.
On the other hand, I am not sure how to make it lightweight and useful
at the same time.

We could not be much talkative in the passed buffer because we do not
know how big it is. We could call printk but it might be hard to match
the line with the caller without the backtrace.

The extra printk() message would end up in the per-CPU printk_safe
buffer when the broken vsprintf() comes from printk(). It might be
pushed to the main log buffer "much" later. On the other hand,
the original string would not end in the log buffer at all
when vsprintf() is not called from printk().


> we have static analysis (at least checkpatch and smatch) that
> should catch that.

Yes but not everybody uses it. Also there are things that depends
on CONFIG setting.

Anyway, I think that triggering the WARN() is rather a corner case.
So I would not be much concerned about using WARN().


> Even just git grep -1 -E '%p"$' finds %pt and %po
> which should get fixed before somebody claims those extensions.

I do not understand this much. git grep -1 -E '%p"$' does not find
lines with %pt and %po. Also it seems that %pt and %po are not
part of any format passed to vsprintf.



> But, I don't disagree with trying to fix up the inconsistency, and
> certainly not with fixing netdev_bits(), but it seems you've missed that
> e.g. the "case: 'g'" is completely compiled out for !CONFIG_BLOCK.
> There's also %pOF which is effectively disabled for !CONFIG_OF (which
> obviously makes sense), but with yet a different fallback behaviour.

You are accurate ;-) I was a bit lazy to change them. %pg was "nicely"
compiled out. %pOF was nicely minimalist solution but not usable in
general. I could do it in v5 or in a separate patchset.


> Hm. I think we should somehow distinguish between the cases of "%po" and
> "%pNX", i.e. specifiers/variants that are always bogus, and the cases of
> a %pOF or %pC that somehow happens even though nobody should have a
> struct device_node* or struct clk* to pass.

I think that the existing formulation is fine for the always bogus
specifiers, for example:

  "Unsupported pointer format specifier: %pGa"

What about the following for the other case:

  "Format specifier %%pC supported only with CONFIG_HAVE_CLK\n"

Best Regards,
Petr

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Andy Shevchenko]

On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:
> There are few printk formats that make sense only with two or more
> specifiers. Also some specifiers make sense only when a kernel feature
> is enabled.
> 
> The handling of unknown specifiers is strange, inconsistent, and
> even leaking the address. For example, netdev_bits() prints the
> non-hashed pointer value or clock() prints "(null)".
> 
> The best solution seems to be in flags_string(). It does not print any
> misleading value. Instead it calls WARN_ONCE() describing the unknown
> specifier. Therefore it clearly shows the problem and helps to find
> it.
> 
> Note that WARN_ONCE() used to cause recursive printk(). But it is safe
> now because vscnprintf() is called in printk_safe context from
> vprintk_emit().
> 

> -	if (!IS_ENABLED(CONFIG_HAVE_CLK) || !clk)
> +	if (!IS_ENABLED(CONFIG_HAVE_CLK)) {
> +		WARN_ONCE(1, "Unsupported pointer format specifier:
> %%pC\n");
> +		return buf;
> +	}
> +
> +	if (!clk)
>  		return string(buf, end, NULL, spec);

This change collides with my patch series. Can you elaborate what your
thoughts are about my patches? Are you going incorporate them to your
series? Should I send them independently?

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

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

On Sat 2018-04-07 17:26:40, Andy Shevchenko wrote:
> On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:
> > There are few printk formats that make sense only with two or more
> > specifiers. Also some specifiers make sense only when a kernel feature
> > is enabled.
> > 
> > The handling of unknown specifiers is strange, inconsistent, and
> > even leaking the address. For example, netdev_bits() prints the
> > non-hashed pointer value or clock() prints "(null)".
> > 
> > The best solution seems to be in flags_string(). It does not print any
> > misleading value. Instead it calls WARN_ONCE() describing the unknown
> > specifier. Therefore it clearly shows the problem and helps to find
> > it.
> > 
> > Note that WARN_ONCE() used to cause recursive printk(). But it is safe
> > now because vscnprintf() is called in printk_safe context from
> > vprintk_emit().
> > 
> 
> > -	if (!IS_ENABLED(CONFIG_HAVE_CLK) || !clk)
> > +	if (!IS_ENABLED(CONFIG_HAVE_CLK)) {
> > +		WARN_ONCE(1, "Unsupported pointer format specifier:
> > %%pC\n");
> > +		return buf;
> > +	}
> > +
> > +	if (!clk)
> >  		return string(buf, end, NULL, spec);
> 
> This change collides with my patch series. Can you elaborate what your
> thoughts are about my patches? Are you going incorporate them to your
> series? Should I send them independently?

Good question. I think that the best solution will be that I go
over your patchset and just add all valid ones into printk.git
for-4.18. Then I will base v5 of this patchset on top of it.

I should have done this earlier. But I did not expect that long
way for the access-check stuff. We originally planned to
do the access check first, see
https://lkml.kernel.org/r/1520000254.10722.389.camel@linux.intel.com
But the access check patchset still need some love, so it makes
sense to switch the order.

Best Regards,
Petr

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Andy Shevchenko]

On Mon, 2018-04-09 at 15:50 +0200, Petr Mladek wrote:
> On Sat 2018-04-07 17:26:40, Andy Shevchenko wrote:
> > On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:

> > This change collides with my patch series. Can you elaborate what
> > your
> > thoughts are about my patches? Are you going incorporate them to
> > your
> > series? Should I send them independently?
> 
> Good question. I think that the best solution will be that I go
> over your patchset and just add all valid ones into printk.git
> for-4.18.

I think about 1-7 and 9 that can go as is before your changes.
And patch 8 postpone

>  Then I will base v5 of this patchset on top of it.

I'm going for vacation tomorrow. Can you just take them into your series
or apply to your tree?

> I should have done this earlier. But I did not expect that long
> way for the access-check stuff. We originally planned to
> do the access check first, see
> https://lkml.kernel.org/r/1520000254.10722.389.camel@linux.intel.com

Yeah, I didn't consider that your one patch became a series...

> But the access check patchset still need some love, so it makes
> sense to switch the order.

I agree.

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

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Petr Mladek]

On Tue 2018-04-10 14:41:55, Andy Shevchenko wrote:
> On Mon, 2018-04-09 at 15:50 +0200, Petr Mladek wrote:
> > On Sat 2018-04-07 17:26:40, Andy Shevchenko wrote:
> > > On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:
> 
> > > This change collides with my patch series. Can you elaborate what
> > > your
> > > thoughts are about my patches? Are you going incorporate them to
> > > your
> > > series? Should I send them independently?
> > 
> > Good question. I think that the best solution will be that I go
> > over your patchset and just add all valid ones into printk.git
> > for-4.18.
> 
> I think about 1-7 and 9 that can go as is before your changes.
> And patch 8 postpone

Done. All the 8 patches are in printk.git, branch
for-4.18-vsprintf-cleanup.

I am sorry that we have missed 4.17. In theory, it still might be
possible to push them there. But there does not seem to be a real
reason to rush them.

> >  Then I will base v5 of this patchset on top of it.
> 
> I'm going for vacation tomorrow. Can you just take them into your series
> or apply to your tree?

Good to know.

Enjoy vacation,
Petr

Re: [PATCH v4 4/9] vsprintf: Consolidate handling of unknown pointer specifiers

[Andy Shevchenko]

On Wed, 2018-04-11 at 11:52 +0200, Petr Mladek wrote:
> On Tue 2018-04-10 14:41:55, Andy Shevchenko wrote:
> > On Mon, 2018-04-09 at 15:50 +0200, Petr Mladek wrote:
> > > On Sat 2018-04-07 17:26:40, Andy Shevchenko wrote:

> > I think about 1-7 and 9 that can go as is before your changes.
> > And patch 8 postpone
> 
> Done. All the 8 patches are in printk.git, branch
> for-4.18-vsprintf-cleanup.

Thanks!

> I am sorry that we have missed 4.17. In theory, it still might be
> possible to push them there. But there does not seem to be a real
> reason to rush them.

Agree.

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

[PATCH v4 5/9] vsprintf: Factor out %p[iI] handler as ip_addr_string()

[Petr Mladek]

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

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

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

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 5a0d01846a11..88b80c7059f0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1319,6 +1319,37 @@ char *ip4_addr_string_sa(char *buf, char *end, const struct sockaddr_in *sa,
 }
 
 static noinline_for_stack
+char *ip_addr_string(char *buf, char *end, const void *ptr,
+		     struct printf_spec spec, const char *fmt)
+{
+	switch (fmt[1]) {
+	case '6':
+		return ip6_addr_string(buf, end, ptr, spec, fmt);
+	case '4':
+		return ip4_addr_string(buf, end, ptr, spec, fmt);
+	case 'S': {
+		const union {
+			struct sockaddr		raw;
+			struct sockaddr_in	v4;
+			struct sockaddr_in6	v6;
+		} *sa = ptr;
+
+		switch (sa->raw.sa_family) {
+		case AF_INET:
+			return ip4_addr_string_sa(buf, end, &sa->v4, spec, fmt);
+		case AF_INET6:
+			return ip6_addr_string_sa(buf, end, &sa->v6, spec, fmt);
+		default:
+			return __string(buf, end, "(invalid address)", spec);
+		}}
+	}
+
+	WARN_ONCE(1, "Unsupported pointer format specifier: %%p%c%c\n",
+		  fmt[0], fmt[1]);
+	return buf;
+}
+
+static noinline_for_stack
 char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 		     const char *fmt)
 {
@@ -1909,28 +1940,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 					 * 4:	001.002.003.004
 					 * 6:   000102...0f
 					 */
-		switch (fmt[1]) {
-		case '6':
-			return ip6_addr_string(buf, end, ptr, spec, fmt);
-		case '4':
-			return ip4_addr_string(buf, end, ptr, spec, fmt);
-		case 'S': {
-			const union {
-				struct sockaddr		raw;
-				struct sockaddr_in	v4;
-				struct sockaddr_in6	v6;
-			} *sa = ptr;
-
-			switch (sa->raw.sa_family) {
-			case AF_INET:
-				return ip4_addr_string_sa(buf, end, &sa->v4, spec, fmt);
-			case AF_INET6:
-				return ip6_addr_string_sa(buf, end, &sa->v6, spec, fmt);
-			default:
-				return __string(buf, end, "(invalid address)", spec);
-			}}
-		}
-		break;
+		return ip_addr_string(buf, end, ptr, spec, fmt);
 	case 'E':
 		return escaped_string(buf, end, ptr, spec, fmt);
 	case 'U':
-- 
2.13.6

Re: [PATCH v4 5/9] vsprintf: Factor out %p[iI] handler as ip_addr_string()

[Sergey Senozhatsky]

On (04/04/18 10:58), Petr Mladek wrote:
> 
> Also it is better to warn about unknown specifier instead of falling
> back to the %p behavior. It will help people to understand what is
> going wrong. They expect the IP address and not a pointer anyway
> in this situation.
>

May be. If one sees a hashed value where IP address/device name/etc
was meant to be then it's already a sign that something is wrong.
Those WARN_ONCE that you have added make things simpler, I agree.
A quick question, what happens on !CONFIG_BUG systems (where we have
no_printk() WARN)?

	-ss

Re: [PATCH v4 5/9] vsprintf: Factor out %p[iI] handler as ip_addr_string()

[Petr Mladek]

On Thu 2018-04-05 08:58:16, Sergey Senozhatsky wrote:
> On (04/04/18 10:58), Petr Mladek wrote:
> > 
> > Also it is better to warn about unknown specifier instead of falling
> > back to the %p behavior. It will help people to understand what is
> > going wrong. They expect the IP address and not a pointer anyway
> > in this situation.
> >
> 
> May be. If one sees a hashed value where IP address/device name/etc
> was meant to be then it's already a sign that something is wrong.
> Those WARN_ONCE that you have added make things simpler, I agree.
> A quick question, what happens on !CONFIG_BUG systems (where we have
> no_printk() WARN)?

People with CONFIG_BUG disabled will miss much more important
warnings. IMHO, an unreported typo in printk format will be
one of their smallest problems.

Best Regards,
Petr

Re: [PATCH v4 5/9] vsprintf: Factor out %p[iI] handler as ip_addr_string()

[Andy Shevchenko]

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

> +	WARN_ONCE(1, "Unsupported pointer format specifier:
> %%p%c%c\n",
> +		  fmt[0], fmt[1]);
> 

I think WARN is too much here.

pr_warn_once() ?


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

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

[Petr Mladek]

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

This patch does not change the existing behavior.

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

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

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

[Joe Perches]

On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:
> Move the code from the long pointer() function. We are going to add a check
> for the access to the address that will make it even more complicated.
> 
> This patch does not change the existing behavior.

But it might increase stack consumption.

As the %pV is recursive, this is may not
be a good thing.

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

[Petr Mladek]

On Wed 2018-04-04 07:26:07, Joe Perches wrote:
> On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:
> > Move the code from the long pointer() function. We are going to add a check
> > for the access to the address that will make it even more complicated.
> > 
> > This patch does not change the existing behavior.
> 
> But it might increase stack consumption.
> 
> As the %pV is recursive, this is may not be a good thing.

It seems to be safe to pass just a pointer to struct printf_spec.
In fact, it would make sense to use this also in string() and
__string() calls. Copying 64 bytes many times look useless.

Do you think that other optimizations would be necessary?
For example, marking the function as inline?

Best Regards,
Petr

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

[Joe Perches]

On Fri, 2018-04-06 at 15:12 +0200, Petr Mladek wrote:
> On Wed 2018-04-04 07:26:07, Joe Perches wrote:
> > On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:
> > > Move the code from the long pointer() function. We are going to add a check
> > > for the access to the address that will make it even more complicated.
> > > 
> > > This patch does not change the existing behavior.
> > 
> > But it might increase stack consumption.
> > 
> > As the %pV is recursive, this is may not be a good thing.
> 
> It seems to be safe to pass just a pointer to struct printf_spec.
> In fact, it would make sense to use this also in string() and
> __string() calls. Copying 64 bytes many times look useless.

huh?

struct printf_spec is 64 bits, the same size as a
pointer on 64 bit systems.

I'm dubious about this entire patch series.

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

[Petr Mladek]

On Fri 2018-04-06 07:19:15, Joe Perches wrote:
> On Fri, 2018-04-06 at 15:12 +0200, Petr Mladek wrote:
> > On Wed 2018-04-04 07:26:07, Joe Perches wrote:
> > > On Wed, 2018-04-04 at 10:58 +0200, Petr Mladek wrote:
> > > > Move the code from the long pointer() function. We are going to add a check
> > > > for the access to the address that will make it even more complicated.
> > > > 
> > > > This patch does not change the existing behavior.
> > > 
> > > But it might increase stack consumption.
> > > 
> > > As the %pV is recursive, this is may not be a good thing.
> > 
> > It seems to be safe to pass just a pointer to struct printf_spec.
> > In fact, it would make sense to use this also in string() and
> > __string() calls. Copying 64 bytes many times look useless.
> 
> huh?
> 
> struct printf_spec is 64 bits, the same size as a
> pointer on 64 bit systems.

Yes, it was a nonsense. It was late Friday here when I wrote it.


> I'm dubious about this entire patch series.

I would appreciate some constructive feedback.
What exactly is wrong? What you would suggest instead?

Best Regards,
Petr

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

[Joe Perches]

On Mon, 2018-04-09 at 13:44 +0200, Petr Mladek wrote:
> What exactly is wrong?

I doubt the need.
Adding complexity doesn't seem useful.

> What you would suggest instead?

A static analysis on the source code instead
of runtime checking.

Perhaps a gcc plugin to validate extension to
pointer type or an external tool to do the same.

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

[Petr Mladek]

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

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

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

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

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index f20eaa3f0092..3551b7957d9e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1772,6 +1772,19 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 }
 
 static noinline_for_stack
+char *kobject_string(char *buf, char *end, void *ptr,
+		     struct printf_spec spec, const char *fmt)
+{
+	switch (fmt[1]) {
+	case 'F':
+		return device_node_string(buf, end, ptr, spec, fmt + 1);
+	}
+
+	WARN_ONCE(1, "Unsupported pointer format specifier: %%pO%c\n", fmt[1]);
+	return buf;
+}
+
+static noinline_for_stack
 char *pointer_string(char *buf, char *end, const void *ptr,
 		     struct printf_spec spec)
 {
@@ -1982,10 +1995,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'G':
 		return flags_string(buf, end, ptr, fmt);
 	case 'O':
-		switch (fmt[1]) {
-		case 'F':
-			return device_node_string(buf, end, ptr, spec, fmt + 1);
-		}
+		return kobject_string(buf, end, ptr, spec, fmt);
 	case 'x':
 		return pointer_string(buf, end, ptr, spec);
 	}
-- 
2.13.6

Re: [PATCH v4 7/9] vsprintf: Factor out %pO handler as kobject_string()

[Sergey Senozhatsky]

On (04/04/18 10:58), Petr Mladek wrote:
>  static noinline_for_stack
> +char *kobject_string(char *buf, char *end, void *ptr,
> +		     struct printf_spec spec, const char *fmt)
> +{
> +	switch (fmt[1]) {
> +	case 'F':
> +		return device_node_string(buf, end, ptr, spec, fmt + 1);
> +	}
> +
> +	WARN_ONCE(1, "Unsupported pointer format specifier: %%pO%c\n", fmt[1]);
> +	return buf;
> +}
> +
> +static noinline_for_stack
>  char *pointer_string(char *buf, char *end, const void *ptr,
>  		     struct printf_spec spec)
>  {
> @@ -1982,10 +1995,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  	case 'G':
>  		return flags_string(buf, end, ptr, fmt);
>  	case 'O':
> -		switch (fmt[1]) {
> -		case 'F':
> -			return device_node_string(buf, end, ptr, spec, fmt + 1);
> -		}
> +		return kobject_string(buf, end, ptr, spec, fmt);
>  	case 'x':
>  		return pointer_string(buf, end, ptr, spec);
>  	}

So, previously, unsupported 'O' would end up in ptr_to_id()

	case 'O':
		switch (fmt[1]) {
		case 'F':
			return device_node_string()
		}
	}

	return ptr_to_id();



now we will just return `buf' without doing ptr_to_id()?

	case 'O':
		return kobject_string();  // which does device_node_string()
					  // for fmt 'O' or return buf
					  // for unknown fmt.
	}

	return ptr_to_id();


Was this your intention?

	-ss

Re: [PATCH v4 7/9] vsprintf: Factor out %pO handler as kobject_string()

[Sergey Senozhatsky]

On (04/05/18 08:35), Sergey Senozhatsky wrote:
> 
> Was this your intention?
> 

Ah, it was. You mentioned it in the commit message

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

Is it so? The default is

	/* default is to _not_ leak addresses, hash before printing */
	return ptr_to_id(buf, end, ptr, spec);

	-ss

Re: [PATCH v4 7/9] vsprintf: Factor out %pO handler as kobject_string()

[Petr Mladek]

On Thu 2018-04-05 08:35:11, Sergey Senozhatsky wrote:
> On (04/04/18 10:58), Petr Mladek wrote:
> >  static noinline_for_stack
> > +char *kobject_string(char *buf, char *end, void *ptr,
> > +		     struct printf_spec spec, const char *fmt)
> > +{
> > +	switch (fmt[1]) {
> > +	case 'F':
> > +		return device_node_string(buf, end, ptr, spec, fmt + 1);
> > +	}
> > +
> > +	WARN_ONCE(1, "Unsupported pointer format specifier: %%pO%c\n", fmt[1]);
> > +	return buf;
> > +}
> > +
> > +static noinline_for_stack
> >  char *pointer_string(char *buf, char *end, const void *ptr,
> >  		     struct printf_spec spec)
> >  {
> > @@ -1982,10 +1995,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> >  	case 'G':
> >  		return flags_string(buf, end, ptr, fmt);
> >  	case 'O':
> > -		switch (fmt[1]) {
> > -		case 'F':
> > -			return device_node_string(buf, end, ptr, spec, fmt + 1);
> > -		}
> > +		return kobject_string(buf, end, ptr, spec, fmt);
> >  	case 'x':
> >  		return pointer_string(buf, end, ptr, spec);
> >  	}
> 
> So, previously, unsupported 'O' would end up in ptr_to_id()

No, it fallen through to pointer_string() because there was
missing "break". Therefore it eventually leaked the pointer.

Don't panic. It seems that all %pOF are correct in the mainline.

> 	case 'O':
> 		switch (fmt[1]) {
> 		case 'F':
> 			return device_node_string()
> 		}
> 	}
> 
> 	return ptr_to_id();
> 
> 
> 
> now we will just return `buf' without doing ptr_to_id()?

Yes, but it will also generate the WARNING. It should help people
undestand what is going on. We have no idea what the author
wanted to do. IMHO, it is confusing to show any random (address)
in that case.

Best Regards,
Petr

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

[Petr Mladek]

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

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

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

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

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

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

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

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

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 934559b3c130..dc020087b12a 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -50,6 +50,13 @@ A raw pointer value may be printed with %p which will hash the address
 before printing. The kernel also supports extended specifiers for printing
 pointers of different types.
 
+Some of the extended specifiers print the data on the given address instead
+of printing the address itself. In this case, the following error messages
+might be printed instead of the unreachable information::
+
+	(null)	 data on plain NULL address
+	(efault) data on invalid address
+
 Plain Pointers
 --------------
 
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 71ebfa43ad05..6bc386d7c7c6 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -209,12 +209,12 @@ test_string(void)
 #define ZEROS "00000000"	/* hex 32 zero bits */
 
 static int __init
-plain_format(void)
+plain_format(void *ptr)
 {
 	char buf[PLAIN_BUF_SIZE];
 	int nchars;
 
-	nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR);
+	nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", ptr);
 
 	if (nchars != PTR_WIDTH || strncmp(buf, ZEROS, strlen(ZEROS)) != 0)
 		return -1;
@@ -227,6 +227,7 @@ plain_format(void)
 #define PTR_WIDTH 8
 #define PTR ((void *)0x456789ab)
 #define PTR_STR "456789ab"
+#define ZEROS ""
 
 static int __init
 plain_format(void)
@@ -238,12 +239,12 @@ plain_format(void)
 #endif	/* BITS_PER_LONG == 64 */
 
 static int __init
-plain_hash(void)
+plain_hash(void *ptr)
 {
 	char buf[PLAIN_BUF_SIZE];
 	int nchars;
 
-	nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR);
+	nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", ptr);
 
 	if (nchars != PTR_WIDTH || strncmp(buf, PTR_STR, PTR_WIDTH) == 0)
 		return -1;
@@ -256,18 +257,18 @@ plain_hash(void)
  * after an address is hashed.
  */
 static void __init
-plain(void)
+plain(void *ptr)
 {
 	int err;
 
-	err = plain_hash();
+	err = plain_hash(ptr);
 	if (err) {
 		pr_warn("plain 'p' does not appear to be hashed\n");
 		failed_tests++;
 		return;
 	}
 
-	err = plain_format();
+	err = plain_format(ptr);
 	if (err) {
 		pr_warn("hashing plain 'p' has unexpected format\n");
 		failed_tests++;
@@ -275,6 +276,24 @@ plain(void)
 }
 
 static void __init
+null_pointer(void)
+{
+	plain(NULL);
+	test(ZEROS "00000000", "%px", NULL);
+	test("(null)", "%pE", NULL);
+}
+
+#define PTR_INVALID ((void *)0x000000ab)
+
+static void __init
+invalid_pointer(void)
+{
+	plain(PTR_INVALID);
+	test(ZEROS "000000ab", "%px", PTR_INVALID);
+	test("(efault)", "%pE", PTR_INVALID);
+}
+
+static void __init
 symbol_ptr(void)
 {
 }
@@ -497,7 +516,9 @@ flags(void)
 static void __init
 test_pointer(void)
 {
-	plain();
+	plain(PTR);
+	null_pointer();
+	invalid_pointer();
 	symbol_ptr();
 	kernel_ptr();
 	struct_resource();
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3551b7957d9e..1a080a75a825 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -599,12 +599,46 @@ char *__string(char *buf, char *end, const char *s, struct printf_spec spec)
 	return widen_string(buf, len, end, spec);
 }
 
+ /*
+  * This is not a fool-proof test. 99% of the time that this will fault is
+  * due to a bad pointer, not one that crosses into bad memory. Just test
+  * the address to make sure it doesn't fault due to a poorly added printk
+  * during debugging.
+  */
+static const char *check_pointer_access(const void *ptr)
+{
+	char byte;
+
+	if (!ptr)
+		return "(null)";
+
+	if (probe_kernel_address(ptr, byte))
+		return "(efault)";
+
+	return NULL;
+}
+
+
+static bool valid_pointer_access(char **buf, char *end, const void *ptr,
+				 struct printf_spec spec)
+{
+	const char *err_msg;
+
+	err_msg = check_pointer_access(ptr);
+	if (err_msg) {
+		*buf = __string(*buf, end, err_msg, spec);
+		return false;
+	}
+
+	return true;
+}
+
 static noinline_for_stack
 char *string(char *buf, char *end, const char *s,
 		   struct printf_spec spec)
 {
-	if ((unsigned long)s < PAGE_SIZE)
-		s = "(null)";
+	if (!valid_pointer_access(&buf, end, s, spec))
+		return buf;
 
 	return __string(buf, end, s, spec);
 }
@@ -686,6 +720,9 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
 	int depth;
 	int i, n;
 
+	if (!valid_pointer_access(&buf, end, d, spec))
+		return buf;
+
 	switch (fmt[1]) {
 		case '2': case '3': case '4':
 			depth = fmt[1] - '0';
@@ -726,8 +763,12 @@ static noinline_for_stack
 char *bdev_name(char *buf, char *end, struct block_device *bdev,
 		struct printf_spec spec, const char *fmt)
 {
-	struct gendisk *hd = bdev->bd_disk;
-	
+	struct gendisk *hd;
+
+	if (!valid_pointer_access(&buf, end, bdev, spec))
+		return buf;
+
+	hd = bdev->bd_disk;
 	buf = string(buf, end, hd->disk_name, spec);
 	if (bdev->bd_part->partno) {
 		if (isdigit(hd->disk_name[strlen(hd->disk_name)-1])) {
@@ -826,6 +867,9 @@ char *resource_string(char *buf, char *end, struct resource *res,
 	int decode = (fmt[0] == 'R') ? 1 : 0;
 	const struct printf_spec *specp;
 
+	if (!valid_pointer_access(&buf, end, res, spec))
+		return buf;
+
 	*p++ = '[';
 	if (res->flags & IORESOURCE_IO) {
 		p = __string(p, pend, "io  ", str_spec);
@@ -888,9 +932,8 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 		/* nothing to print */
 		return buf;
 
-	if (ZERO_OR_NULL_PTR(addr))
-		/* NULL pointer */
-		return string(buf, end, NULL, spec);
+	if (!valid_pointer_access(&buf, end, addr, spec))
+		return buf;
 
 	switch (fmt[1]) {
 	case 'C':
@@ -937,6 +980,9 @@ char *bitmap_string(char *buf, char *end, unsigned long *bitmap,
 	int i, chunksz;
 	bool first = true;
 
+	if (!valid_pointer_access(&buf, end, bitmap, spec))
+		return buf;
+
 	/* reused to print numbers */
 	spec = (struct printf_spec){ .flags = SMALL | ZEROPAD, .base = 16 };
 
@@ -978,6 +1024,9 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap,
 	int cur, rbot, rtop;
 	bool first = true;
 
+	if (!valid_pointer_access(&buf, end, bitmap, spec))
+		return buf;
+
 	/* reused to print numbers */
 	spec = (struct printf_spec){ .base = 10 };
 
@@ -1019,6 +1068,9 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
 	char separator;
 	bool reversed = false;
 
+	if (!valid_pointer_access(&buf, end, addr, spec))
+		return buf;
+
 	switch (fmt[1]) {
 	case 'F':
 		separator = '-';
@@ -1319,9 +1371,12 @@ char *ip4_addr_string_sa(char *buf, char *end, const struct sockaddr_in *sa,
 }
 
 static noinline_for_stack
-char *ip_addr_string(char *buf, char *end, const void *ptr,
-		     struct printf_spec spec, const char *fmt)
+char *ip_addr_string(char *buf, char *end, void *ptr, struct printf_spec spec,
+		     const char *fmt)
 {
+	if (!valid_pointer_access(&buf, end, ptr, spec))
+		return buf;
+
 	switch (fmt[1]) {
 	case '6':
 		return ip6_addr_string(buf, end, ptr, spec, fmt);
@@ -1361,9 +1416,8 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 	if (spec.field_width == 0)
 		return buf;				/* nothing to print */
 
-	if (ZERO_OR_NULL_PTR(addr))
-		return string(buf, end, NULL, spec);	/* NULL pointer */
-
+	if (!valid_pointer_access(&buf, end, addr, spec))
+		return buf;
 
 	do {
 		switch (fmt[count++]) {
@@ -1410,10 +1464,14 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 }
 
 static noinline_for_stack
-char *va_format(char *buf, char *end, struct va_format *va_fmt)
+char *va_format(char *buf, char *end, struct va_format *va_fmt,
+		struct printf_spec spec, const char *fmt)
 {
 	va_list va;
 
+	if (!valid_pointer_access(&buf, end, va_fmt, spec))
+		return buf;
+
 	va_copy(va, *va_fmt->va);
 	buf += vsnprintf(buf, end > buf ? end - buf : 0,
 			 va_fmt->fmt, va);
@@ -1432,6 +1490,9 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 	const u8 *index = uuid_index;
 	bool uc = false;
 
+	if (!valid_pointer_access(&buf, end, addr, spec))
+		return buf;
+
 	switch (*(++fmt)) {
 	case 'L':
 		uc = true;		/* fall-through */
@@ -1517,11 +1578,15 @@ char *restricted_pointer(char *buf, char *end, const void *ptr,
 }
 
 static noinline_for_stack
-char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt)
+char *netdev_bits(char *buf, char *end, const void *addr,
+		  struct printf_spec spec, const char *fmt)
 {
 	unsigned long long num;
 	int size;
 
+	if (!valid_pointer_access(&buf, end, addr, spec))
+		return buf;
+
 	switch (fmt[1]) {
 	case 'F':
 		num = *(const netdev_features_t *)addr;
@@ -1537,11 +1602,15 @@ char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt)
 }
 
 static noinline_for_stack
-char *address_val(char *buf, char *end, const void *addr, const char *fmt)
+char *address_val(char *buf, char *end, const void *addr,
+		  struct printf_spec spec, const char *fmt)
 {
 	unsigned long long num;
 	int size;
 
+	if (!valid_pointer_access(&buf, end, addr, spec))
+		return buf;
+
 	switch (fmt[1]) {
 	case 'd':
 		num = *(const dma_addr_t *)addr;
@@ -1566,8 +1635,8 @@ char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
 		return buf;
 	}
 
-	if (!clk)
-		return string(buf, end, NULL, spec);
+	if (!valid_pointer_access(&buf, end, clk, spec))
+		return buf;
 
 	switch (fmt[1]) {
 	case 'r':
@@ -1622,11 +1691,15 @@ char *format_flags(char *buf, char *end, unsigned long flags,
 }
 
 static noinline_for_stack
-char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)
+char *flags_string(char *buf, char *end, void *flags_ptr,
+		   struct printf_spec spec, const char *fmt)
 {
 	unsigned long flags;
 	const struct trace_print_flags *names;
 
+	if (!valid_pointer_access(&buf, end, flags_ptr, spec))
+		return buf;
+
 	switch (fmt[1]) {
 	case 'p':
 		flags = *(unsigned long *)flags_ptr;
@@ -1919,18 +1992,6 @@ static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	      struct printf_spec spec)
 {
-	const int default_width = 2 * sizeof(void *);
-
-	if (!ptr && *fmt != 'K' && *fmt != 'x') {
-		/*
-		 * Print (null) with the same width as a pointer so it makes
-		 * tabular output look nice.
-		 */
-		if (spec.field_width == -1)
-			spec.field_width = default_width;
-		return __string(buf, end, "(null)", spec);
-	}
-
 	switch (*fmt) {
 	case 'F':
 	case 'f':
@@ -1972,13 +2033,13 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'U':
 		return uuid_string(buf, end, ptr, spec, fmt);
 	case 'V':
-		return va_format(buf, end, ptr);
+		return va_format(buf, end, ptr, spec, fmt);
 	case 'K':
 		return restricted_pointer(buf, end, ptr, spec);
 	case 'N':
-		return netdev_bits(buf, end, ptr, fmt);
+		return netdev_bits(buf, end, ptr, spec, fmt);
 	case 'a':
-		return address_val(buf, end, ptr, fmt);
+		return address_val(buf, end, ptr, spec, fmt);
 	case 'd':
 		return dentry_name(buf, end, ptr, spec, fmt);
 	case 'C':
@@ -1993,7 +2054,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 #endif
 
 	case 'G':
-		return flags_string(buf, end, ptr, fmt);
+		return flags_string(buf, end, ptr, spec, fmt);
 	case 'O':
 		return kobject_string(buf, end, ptr, spec, fmt);
 	case 'x':
@@ -2609,11 +2670,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 v4 8/9] vsprintf: Prevent crash when dereferencing invalid pointers

[Rasmus Villemoes]

On 2018-04-04 10:58, Petr Mladek wrote:

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3551b7957d9e..1a080a75a825 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -599,12 +599,46 @@ char *__string(char *buf, char *end, const char *s, struct printf_spec spec)
>  	return widen_string(buf, len, end, spec);
>  }
>  
> + /*
> +  * This is not a fool-proof test. 99% of the time that this will fault is
> +  * due to a bad pointer, not one that crosses into bad memory. Just test
> +  * the address to make sure it doesn't fault due to a poorly added printk
> +  * during debugging.
> +  */
> +static const char *check_pointer_access(const void *ptr)
> +{
> +	char byte;
> +
> +	if (!ptr)
> +		return "(null)";
> +
> +	if (probe_kernel_address(ptr, byte))
> +		return "(efault)";
> +
> +	return NULL;
> +}

So while I think the WARNings are mostly pointless for the bad format
specifiers, I'm wondering why an averted crash is not worth a WARN_ONCE?
This means there's an actual bug somewhere, probably even exploitable,
but we're just silently producing some innocent string...

Also, I'd still prefer to insist on ptr being a kernel pointer. Sure,
for %ph userspace gets to print their own memory, but for a lot of the
others, we're chasing pointers another level, so if an attacker can feed
a user pointer to one of those, there's a trivial arbitrary read gadget.
We have lots of printks in untested error paths, and I find it quite
likely that one of those uses a garbage pointer.

I know you're mostly phrasing this in terms of preventing a crash, but
it seems silly not to close that when it only costs a pointer comparison.

You're also missing the %pD (struct file*) case, which is one of those
double-pointer chasing cases.

Rasmus

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

[Petr Mladek]

On Thu 2018-04-05 16:46:23, Rasmus Villemoes wrote:
> On 2018-04-04 10:58, Petr Mladek wrote:
> 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 3551b7957d9e..1a080a75a825 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -599,12 +599,46 @@ char *__string(char *buf, char *end, const char *s, struct printf_spec spec)
> >  	return widen_string(buf, len, end, spec);
> >  }
> >  
> > + /*
> > +  * This is not a fool-proof test. 99% of the time that this will fault is
> > +  * due to a bad pointer, not one that crosses into bad memory. Just test
> > +  * the address to make sure it doesn't fault due to a poorly added printk
> > +  * during debugging.
> > +  */
> > +static const char *check_pointer_access(const void *ptr)
> > +{
> > +	char byte;
> > +
> > +	if (!ptr)
> > +		return "(null)";
> > +
> > +	if (probe_kernel_address(ptr, byte))
> > +		return "(efault)";
> > +
> > +	return NULL;
> > +}
> 
> So while I think the WARNings are mostly pointless for the bad format
> specifiers, I'm wondering why an averted crash is not worth a
> WARN_ONCE?

It is used to match the error with the code. It was more explained in
the other mail.


> This means there's an actual bug somewhere, probably even exploitable,
> but we're just silently producing some innocent string...

Good point! It would make sense in many situations, especially when
we "silently" crashed so far.

I just wonder if some code already relies on the fact that passing
NULL is rather innocent, for example, in some timer- or networking-
related debug output. This change might make it hard to read.

Anyway, I still thing that it makes sense. But I would do it as
a separate patch so that it can be reverted easily. In each case,
it should spend some time in linux-next.


> Also, I'd still prefer to insist on ptr being a kernel pointer. Sure,
> for %ph userspace gets to print their own memory, but for a lot of the
> others, we're chasing pointers another level, so if an attacker can feed
> a user pointer to one of those, there's a trivial arbitrary read gadget.
> We have lots of printks in untested error paths, and I find it quite
> likely that one of those uses a garbage pointer.
> 
> I know you're mostly phrasing this in terms of preventing a crash, but
> it seems silly not to close that when it only costs a pointer comparison.

I thought that it was good idea but Steven was against, see
https://lkml.kernel.org/r/20180315130612.4b4cd091@vmware.local.home


> You're also missing the %pD (struct file*) case, which is one of those
> double-pointer chasing cases.

I wanted to keep the initial code simple. Well, %pD is pretty
straightforward, I could move the check to the cycle in v5.

I just want to avoid a monster patchset that would add the check
for every read byte. I am not persuaded that it is worth it.

Best Regards,
Petr

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

[Rasmus Villemoes]

On 2018-04-06 14:26, Petr Mladek wrote:
> On Thu 2018-04-05 16:46:23, Rasmus Villemoes wrote:
>> On 2018-04-04 10:58, Petr Mladek wrote:
>>
>>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>>> index 3551b7957d9e..1a080a75a825 100644
>>> --- a/lib/vsprintf.c
>>> +++ b/lib/vsprintf.c
>>> @@ -599,12 +599,46 @@ char *__string(char *buf, char *end, const char *s, struct printf_spec spec)
>>>  	return widen_string(buf, len, end, spec);
>>>  }
>>>  
>>> + /*
>>> +  * This is not a fool-proof test. 99% of the time that this will fault is
>>> +  * due to a bad pointer, not one that crosses into bad memory. Just test
>>> +  * the address to make sure it doesn't fault due to a poorly added printk
>>> +  * during debugging.
>>> +  */
>>> +static const char *check_pointer_access(const void *ptr)
>>> +{
>>> +	char byte;
>>> +
>>> +	if (!ptr)
>>> +		return "(null)";
>>> +
>>> +	if (probe_kernel_address(ptr, byte))
>>> +		return "(efault)";
>>> +
>>> +	return NULL;
>>> +}
>>
>> So while I think the WARNings are mostly pointless for the bad format
>> specifiers, I'm wondering why an averted crash is not worth a
>> WARN_ONCE?
> 
> It is used to match the error with the code. It was more explained in
> the other mail.

Yes, I agree that _if_ we want to trigger some action rather than just
gracefully handling a bogus %pXYZ by printing nothing, it must include a
backtrace to be useful, and not just some pr_warn. I'm just doubting
whether it is worth the code size increase - especially if we'd want to
be _consistent_ and then also warn about %phq and %pISaa and %pbm and
all the other cases where we do not check the trailing letters for
making sense. So maybe a WARN for a 'toplevel' %p* makes sense, but I'd
rather rely on static analysis for the rest and remove WARN_ONCE in
flags_string than introducing a myriad new WARN_ONCEs.

> 
>> This means there's an actual bug somewhere, probably even exploitable,
>> but we're just silently producing some innocent string...
> 
> Good point! It would make sense in many situations, especially when
> we "silently" crashed so far.
> 
> I just wonder if some code already relies on the fact that passing
> NULL is rather innocent, for example, in some timer- or networking-
> related debug output. This change might make it hard to read.

Yeah, I would also just WARN in the case where we get to
probe_kernel_address and fails that. That's why it should be in
check_pointer_access where we can distinguish, and not in its caller.

But I think string() still needs to allow not just NULL but the full
first page (mapping that to "(null)" as it used to), so that doesn't
change the need for string to special-case that before calling
valid_pointer_access.

>> Also, I'd still prefer to insist on ptr being a kernel pointer. Sure,
>> for %ph userspace gets to print their own memory, but for a lot of the
>> others, we're chasing pointers another level, so if an attacker can feed
>> a user pointer to one of those, there's a trivial arbitrary read gadget.
>> We have lots of printks in untested error paths, and I find it quite
>> likely that one of those uses a garbage pointer.
>>
>> I know you're mostly phrasing this in terms of preventing a crash, but
>> it seems silly not to close that when it only costs a pointer comparison.
> 
> I thought that it was good idea but Steven was against, see
> https://lkml.kernel.org/r/20180315130612.4b4cd091@vmware.local.home

I know. If he has veto power, so be it.

>> You're also missing the %pD (struct file*) case, which is one of those
>> double-pointer chasing cases.
> 
> I wanted to keep the initial code simple. Well, %pD is pretty
> straightforward, I could move the check to the cycle in v5.
> 
> I just want to avoid a monster patchset that would add the check
> for every read byte. I am not persuaded that it is worth it.

No, we most definitely do not want to read every byte with
probe_kernel_read, vsnprintf() is used far too widely for that. I think
checking that the initial pointer points to valid memory is a fine
heuristic. I'm just saying that that is then precisely what we should
do, and %pD does a dereference before we get to dentry_name. (And ok, if
we then end up doing another check due to reuse of the %pd code, fine).

Rasmus

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

[Petr Mladek]

On Fri 2018-04-06 15:12:03, Rasmus Villemoes wrote:
> On 2018-04-06 14:26, Petr Mladek wrote:
> > On Thu 2018-04-05 16:46:23, Rasmus Villemoes wrote:
> >> On 2018-04-04 10:58, Petr Mladek wrote:
> >>
> >>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> >>> index 3551b7957d9e..1a080a75a825 100644
> >>> --- a/lib/vsprintf.c
> >>> +++ b/lib/vsprintf.c
> >>> @@ -599,12 +599,46 @@ char *__string(char *buf, char *end, const char *s, struct printf_spec spec)
> >>>  	return widen_string(buf, len, end, spec);
> >>>  }
> >>>  
> >>> + /*
> >>> +  * This is not a fool-proof test. 99% of the time that this will fault is
> >>> +  * due to a bad pointer, not one that crosses into bad memory. Just test
> >>> +  * the address to make sure it doesn't fault due to a poorly added printk
> >>> +  * during debugging.
> >>> +  */
> >>> +static const char *check_pointer_access(const void *ptr)
> >>> +{
> >>> +	char byte;
> >>> +
> >>> +	if (!ptr)
> >>> +		return "(null)";
> >>> +
> >>> +	if (probe_kernel_address(ptr, byte))
> >>> +		return "(efault)";
> >>> +
> >>> +	return NULL;
> >>> +}
> >>
> >> So while I think the WARNings are mostly pointless for the bad format
> >> specifiers, I'm wondering why an averted crash is not worth a
> >> WARN_ONCE?
> > 
> > It is used to match the error with the code. It was more explained in
> > the other mail.
> 
> Yes, I agree that _if_ we want to trigger some action rather than just
> gracefully handling a bogus %pXYZ by printing nothing, it must include a
> backtrace to be useful, and not just some pr_warn. I'm just doubting
> whether it is worth the code size increase - especially if we'd want to
> be _consistent_ and then also warn about %phq and %pISaa and %pbm and
> all the other cases where we do not check the trailing letters for
> making sense. So maybe a WARN for a 'toplevel' %p* makes sense, but I'd
> rather rely on static analysis for the rest and remove WARN_ONCE in
> flags_string than introducing a myriad new WARN_ONCEs.

Static analyze is fine if you are cleaning existing code. But it
does not work well when the behavior depends on CONFIG setting.
Also it is not much helpful when you write new code and it does
not work as expected.

The increase of the code is minimal. It either replaced existing
handling or it was added into newly created functions.

I needed to create the functions because I wanted to add
the access check. I would need to handle the error anyway.
In fact, the refactoring helped to fix a possible information leak, see
https://lkml.kernel.org/r/20180404085843.16050-8-pmladek@suse.com

IMHO, we do not need to solve this on all levels. There is mostly
reasonable fallback.

We could do nothing and just eat the invalid format specifier.
But whom will it help? Are these WARN_ONCE()s really so useless and
even harmful? I would understand this fear if they started complicating
the code or if we started to print many of them at runtime regularly.
But this is not the case.



> >> This means there's an actual bug somewhere, probably even exploitable,
> >> but we're just silently producing some innocent string...
> > 
> > Good point! It would make sense in many situations, especially when
> > we "silently" crashed so far.
> > 
> > I just wonder if some code already relies on the fact that passing
> > NULL is rather innocent, for example, in some timer- or networking-
> > related debug output. This change might make it hard to read.
> 
> Yeah, I would also just WARN in the case where we get to
> probe_kernel_address and fails that. That's why it should be in
> check_pointer_access where we can distinguish, and not in its caller.

Yes.


> But I think string() still needs to allow not just NULL but the full
> first page (mapping that to "(null)" as it used to), so that doesn't
> change the need for string to special-case that before calling
> valid_pointer_access.

Well, the special handling for %s is weird. In fact, writing (null) for
the entire first page is strange. On the other hand, we have written
(null) and did not warn for all the affected %p formats so far.

Therefore I suggest to WARN() only when probe_kernel_address()
fails and not for NULL. Also I suggest to do it consistently.
It will get some testing in linux-next and RCs. We could change
this if people scream. But passing invalid pointer to %s looks
like a bug. I believe that there are not many of them.


> >> Also, I'd still prefer to insist on ptr being a kernel pointer. Sure,
> >> for %ph userspace gets to print their own memory, but for a lot of the
> >> others, we're chasing pointers another level, so if an attacker can feed
> >> a user pointer to one of those, there's a trivial arbitrary read gadget.
> >> We have lots of printks in untested error paths, and I find it quite
> >> likely that one of those uses a garbage pointer.
> >>
> >> I know you're mostly phrasing this in terms of preventing a crash, but
> >> it seems silly not to close that when it only costs a pointer comparison.
> > 
> > I thought that it was good idea but Steven was against, see
> > https://lkml.kernel.org/r/20180315130612.4b4cd091@vmware.local.home
> 
> I know. If he has veto power, so be it.

Steven is very experienced and wrote amazing amount of kernel code.
I just take his opinion seriously. Of course, it does not mean that
he is right.

Anyway, adding the check would change the existing behavior. Steven
suggest that it might be an obstacle. People want from printk()
to give information and do not make unnecessary obstructions.
The question is when the check might help and when it might
be annoying. I do not have time to go more deeply into this
and argue about it at the moment. But feel to do so...


> >> You're also missing the %pD (struct file*) case, which is one of those
> >> double-pointer chasing cases.
> > 
> > I wanted to keep the initial code simple. Well, %pD is pretty
> > straightforward, I could move the check to the cycle in v5.
> > 
> > I just want to avoid a monster patchset that would add the check
> > for every read byte. I am not persuaded that it is worth it.
> 
> No, we most definitely do not want to read every byte with
> probe_kernel_read, vsnprintf() is used far too widely for that. I think
> checking that the initial pointer points to valid memory is a fine
> heuristic. I'm just saying that that is then precisely what we should
> do, and %pD does a dereference before we get to dentry_name. (And ok, if
> we then end up doing another check due to reuse of the %pd code, fine).

OK, I'll extend the check in %pD.

Best Regards,
Petr

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

[Rasmus Villemoes]

On 2018-04-04 10:58, Petr Mladek wrote:

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3551b7957d9e..1a080a75a825 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -599,12 +599,46 @@ char *__string(char *buf, char *end, const char *s, struct printf_spec spec)
>  	return widen_string(buf, len, end, spec);
>  }
>  
> + /*
> +  * This is not a fool-proof test. 99% of the time that this will fault is
> +  * due to a bad pointer, not one that crosses into bad memory. Just test
> +  * the address to make sure it doesn't fault due to a poorly added printk
> +  * during debugging.
> +  */
> +static const char *check_pointer_access(const void *ptr)
> +{
> +	char byte;
> +
> +	if (!ptr)
> +		return "(null)";
> +
> +	if (probe_kernel_address(ptr, byte))
> +		return "(efault)";
> +
> +	return NULL;
> +}
> +
> +
> +static bool valid_pointer_access(char **buf, char *end, const void *ptr,
> +				 struct printf_spec spec)
> +{
> +	const char *err_msg;
> +
> +	err_msg = check_pointer_access(ptr);
> +	if (err_msg) {
> +		*buf = __string(*buf, end, err_msg, spec);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  static noinline_for_stack
>  char *string(char *buf, char *end, const char *s,
>  		   struct printf_spec spec)
>  {
> -	if ((unsigned long)s < PAGE_SIZE)
> -		s = "(null)";
> +	if (!valid_pointer_access(&buf, end, s, spec))
> +		return buf;
>  
>  	return __string(buf, end, s, spec);
>  }

Obviously, if you do add a WARN to the check_pointer_access (and please
do), that somehow needs to be suppressed for the "%s", NULL and "%s",
ZEROPTR cases, which are grandfathered in and I think is relied upon in
some places. It should be as simple as keeping  the < PAGE_SIZE check
and do "else if (!valid...())".

Rasmus

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

[Petr Mladek]

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

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

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

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index dc020087b12a..974af53e9b43 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -56,6 +56,7 @@ might be printed instead of the unreachable information::
 
 	(null)	 data on plain NULL address
 	(efault) data on invalid address
+	(einval) invalid data on a valid address
 
 Plain Pointers
 --------------
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 1a080a75a825..4fcd4ce91d32 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1395,7 +1395,7 @@ char *ip_addr_string(char *buf, char *end, void *ptr, struct printf_spec spec,
 		case AF_INET6:
 			return ip6_addr_string_sa(buf, end, &sa->v6, spec, fmt);
 		default:
-			return __string(buf, end, "(invalid address)", spec);
+			return __string(buf, end, "(einval)", spec);
 		}}
 	}
 
-- 
2.13.6