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

Rasmus Villemoes
2018-03-14T22:12:42Z

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