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

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

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

> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 71ebfa43ad05..61c05a352d79 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -207,14 +207,15 @@ test_string(void)
> @@ -256,18 +259,18 @@ plain_hash(void)
>   * after an address is hashed.
>   */
>  static void __init
> -plain(void)
> +plain(void *ptr)
>  {
>  	int err;
>  
> -	err = plain_hash();
> +	err = plain_hash(ptr);
>  	if (err) {
>  		pr_warn("plain 'p' does not appear to be hashed\n");
>  		failed_tests++;
>  		return;
>  	}
>  
> -	err = plain_format();
> +	err = plain_format(ptr);
>  	if (err) {
>  		pr_warn("hashing plain 'p' has unexpected format\n");
>  		failed_tests++;
> @@ -275,6 +278,24 @@ plain(void)
>  }

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

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

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

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

return NULL;

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

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

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

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

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

Rasmus