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

[Petr Mladek <pmladek@suse.com>]

On Wed 2018-03-14 23:12:36, Rasmus Villemoes wrote:
> On 2018-03-14 15:09, Petr Mladek wrote:
> 
> > diff --git a/lib/test_printf.c b/lib/test_printf.c
> > index 71ebfa43ad05..61c05a352d79 100644
> > --- a/lib/test_printf.c
> > +++ b/lib/test_printf.c
> > @@ -207,14 +207,15 @@ test_string(void)
> > @@ -256,18 +259,18 @@ plain_hash(void)
> >   * after an address is hashed.
> >   */
> >  static void __init
> > -plain(void)
> > +plain(void *ptr)
> >  {
> >  	int err;
> >  
> > -	err = plain_hash();
> > +	err = plain_hash(ptr);
> >  	if (err) {
> >  		pr_warn("plain 'p' does not appear to be hashed\n");
> >  		failed_tests++;
> >  		return;
> >  	}
> >  
> > -	err = plain_format();
> > +	err = plain_format(ptr);
> >  	if (err) {
> >  		pr_warn("hashing plain 'p' has unexpected format\n");
> >  		failed_tests++;
> > @@ -275,6 +278,24 @@ plain(void)
> >  }
> 
> Thanks for adding tests. Please increment total_tests for each test case.

Good point! Will do in v4.

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

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

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

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

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

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

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

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


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

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

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

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

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


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

Good point. The following should do the job:

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

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

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

	return 0;
}

Best Regards,
Petr