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

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

[Alexey Dobriyan]

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

This is getting ridiculous.

Instead of simply printing a pointer as %08lx or %016llx, not only glibc
(null) stupidity is propagated but expanded and "improved".

I assure you reading 0000000000000000 is just as obvious as (null) and
reading fffffffffffffffa is just as good as -ENOMEM.

In fact printing with hex is more information. Maybe it is important
that buggy pointer is small value but it's value is lost.

Sure don't dereference a pointer for very small or very erry values
but print it without all the bell and whistles.

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

[Adam Borowski]

On Tue, Mar 06, 2018 at 09:22:17PM +0300, Alexey Dobriyan wrote:
> > +#define BAD_PTR_STRING(x) (!(x) ? "(null)" : IS_ERR(x) ? "(err)" : "(invalid)")
> 
> This is getting ridiculous.
> 
> Instead of simply printing a pointer as %08lx or %016llx, not only glibc
> (null) stupidity is propagated but expanded and "improved".

This is not about printing a pointer, this is about attempting to print an
object referenced by such a bad pointer.  Which leads to a crash: in
userspace, you get a segfault; in the kernel, at least in the case I tested,
the system is dead without even a squeal on either console or serial.

> I assure you reading 0000000000000000 is just as obvious as (null) and
> reading fffffffffffffffa is just as good as -ENOMEM.
> 
> In fact printing with hex is more information. Maybe it is important
> that buggy pointer is small value but it's value is lost.
>
> Sure don't dereference a pointer for very small or very erry values
> but print it without all the bell and whistles.

That's a reasonable suggestion, but it still needs to be special cased.
Note the difference between printk("%px", 42) and printk("%s", 42).

See this part:
-       if (!ptr && *fmt != 'K' && *fmt != 'x') {
+       if (IS_BAD_PTR(ptr) && *fmt != 'K' && *fmt != 'x') {
Printing the pointer is already excluded; what I'm fixing is:
1. lying that the bad pointer was (null) when it was -ENOMEM (commit 1)
2. crash when some bad code tries to printk("%s", -ENOMEM) (commit 2)

So, if what you propose is applying commit 2, and changing 1 to print the
raw value instead of (null)/(err)/(invalid), that sounds good.


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

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

[Petr Mladek]

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

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

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

Best Regards,
Petr

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

[Adam Borowski]

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

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

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

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

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

[Andy Shevchenko]

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

Thanks for the patch, my comments below.

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

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

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

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

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

It looks ugly.

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

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

It simple doesn't make sense.

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

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

Doesn't make sense before your patch 2.

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

This is perhaps one valid change in such situation.

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

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

[Adam Borowski]

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

(Review snipped.)

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


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

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

[Andy Shevchenko]

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

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

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