diff mbox series

printk: hash addresses printed with %p

Message ID 20210224200101.9835-2-tim.gardner@canonical.com
State New
Headers show
Series CVE-2018-7273 | expand

Commit Message

Tim Gardner Feb. 24, 2021, 8:01 p.m. UTC
From: "Tobin C. Harding" <me@tobin.cc>

Currently there exist approximately 14 000 places in the kernel where
addresses are being printed using an unadorned %p. This potentially
leaks sensitive information regarding the Kernel layout in memory. Many
of these calls are stale, instead of fixing every call lets hash the
address by default before printing. This will of course break some
users, forcing code printing needed addresses to be updated.

Code that _really_ needs the address will soon be able to use the new
printk specifier %px to print the address.

For what it's worth, usage of unadorned %p can be broken down as
follows (thanks to Joe Perches).

$ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
   1084 arch
     20 block
     10 crypto
     32 Documentation
   8121 drivers
   1221 fs
    143 include
    101 kernel
     69 lib
    100 mm
   1510 net
     40 samples
      7 scripts
     11 security
    166 sound
    152 tools
      2 virt

Add function ptr_to_id() to map an address to a 32 bit unique
identifier. Hash any unadorned usage of specifier %p and any malformed
specifiers.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
(backported from commit ad67b74d2469d9b82aaa572d76474c95bc484d57)
CVE-2018-7273
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>

Conflicts:
	Documentation/printk-formats.txt (Added a paragraph under Kernel Pointers)
	lib/vsprintf.c (required additional include files, no code changes)
---
 Documentation/printk-formats.txt |   6 ++
 lib/test_printf.c                | 108 ++++++++++++++++++++-----------
 lib/vsprintf.c                   |  86 ++++++++++++++++++++++--
 3 files changed, 155 insertions(+), 45 deletions(-)

Comments

Thadeu Lima de Souza Cascardo Feb. 24, 2021, 8:26 p.m. UTC | #1
On Wed, Feb 24, 2021 at 01:01:01PM -0700, Tim Gardner wrote:
> From: "Tobin C. Harding" <me@tobin.cc>
> 
> Currently there exist approximately 14 000 places in the kernel where
> addresses are being printed using an unadorned %p. This potentially
> leaks sensitive information regarding the Kernel layout in memory. Many
> of these calls are stale, instead of fixing every call lets hash the
> address by default before printing. This will of course break some
> users, forcing code printing needed addresses to be updated.
> 
> Code that _really_ needs the address will soon be able to use the new
> printk specifier %px to print the address.
> 
> For what it's worth, usage of unadorned %p can be broken down as
> follows (thanks to Joe Perches).
> 
> $ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
>    1084 arch
>      20 block
>      10 crypto
>      32 Documentation
>    8121 drivers
>    1221 fs
>     143 include
>     101 kernel
>      69 lib
>     100 mm
>    1510 net
>      40 samples
>       7 scripts
>      11 security
>     166 sound
>     152 tools
>       2 virt
> 
> Add function ptr_to_id() to map an address to a 32 bit unique
> identifier. Hash any unadorned usage of specifier %p and any malformed
> specifiers.
> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> (backported from commit ad67b74d2469d9b82aaa572d76474c95bc484d57)
> CVE-2018-7273
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> 
> Conflicts:
> 	Documentation/printk-formats.txt (Added a paragraph under Kernel Pointers)
> 	lib/vsprintf.c (required additional include files, no code changes)
> ---
>  Documentation/printk-formats.txt |   6 ++
>  lib/test_printf.c                | 108 ++++++++++++++++++++-----------
>  lib/vsprintf.c                   |  86 ++++++++++++++++++++++--
>  3 files changed, 155 insertions(+), 45 deletions(-)
> 
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index ed6f6abaad57..2329b1eac0fa 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -64,6 +64,12 @@ Kernel Pointers:
>  	users. The behaviour of %pK depends on the kptr_restrict sysctl - see
>  	Documentation/sysctl/kernel.txt for more details.
>  
> +	Pointers printed without a specifier extension (i.e unadorned %p) are
> +	hashed to give a unique identifier without leaking kernel addresses to user
> +	space. On 64 bit machines the first 32 bits are zeroed.
> +
> +		%p	abcdef12 or 00000000abcdef12
> +
>  Struct Resources:
>  
>  	%pr	[mem 0x60000000-0x6fffffff flags 0x2200] or
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index c5a666af9ba5..e2200f06f168 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -18,24 +18,6 @@
>  #define BUF_SIZE 256
>  #define FILL_CHAR '$'
>  
> -#define PTR1 ((void*)0x01234567)
> -#define PTR2 ((void*)(long)(int)0xfedcba98)
> -
> -#if BITS_PER_LONG == 64
> -#define PTR1_ZEROES "000000000"
> -#define PTR1_SPACES "         "
> -#define PTR1_STR "1234567"
> -#define PTR2_STR "fffffffffedcba98"
> -#define PTR_WIDTH 16
> -#else
> -#define PTR1_ZEROES "0"
> -#define PTR1_SPACES " "
> -#define PTR1_STR "1234567"
> -#define PTR2_STR "fedcba98"
> -#define PTR_WIDTH 8
> -#endif
> -#define PTR_WIDTH_STR stringify(PTR_WIDTH)
> -
>  static unsigned total_tests __initdata;
>  static unsigned failed_tests __initdata;
>  static char *test_buffer __initdata;
> @@ -160,30 +142,79 @@ test_string(void)
>  	test("a  |   |   ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, "c");
>  }
>  
> +#define PLAIN_BUF_SIZE 64	/* leave some space so we don't oops */
> +
> +#if BITS_PER_LONG == 64
> +
> +#define PTR_WIDTH 16
> +#define PTR ((void *)0xffff0123456789ab)
> +#define PTR_STR "ffff0123456789ab"
> +#define ZEROS "00000000"	/* hex 32 zero bits */
> +
> +static int __init
> +plain_format(void)
> +{
> +	char buf[PLAIN_BUF_SIZE];
> +	int nchars;
> +
> +	nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR);
> +
> +	if (nchars != PTR_WIDTH || strncmp(buf, ZEROS, strlen(ZEROS)) != 0)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +#else
> +
> +#define PTR_WIDTH 8
> +#define PTR ((void *)0x456789ab)
> +#define PTR_STR "456789ab"
> +
> +static int __init
> +plain_format(void)
> +{
> +	/* Format is implicitly tested for 32 bit machines by plain_hash() */
> +	return 0;
> +}
> +
> +#endif	/* BITS_PER_LONG == 64 */
> +
> +static int __init
> +plain_hash(void)
> +{
> +	char buf[PLAIN_BUF_SIZE];
> +	int nchars;
> +
> +	nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR);
> +
> +	if (nchars != PTR_WIDTH || strncmp(buf, PTR_STR, PTR_WIDTH) == 0)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +/*
> + * We can't use test() to test %p because we don't know what output to expect
> + * after an address is hashed.
> + */
>  static void __init
>  plain(void)
>  {
> -	test(PTR1_ZEROES PTR1_STR " " PTR2_STR, "%p %p", PTR1, PTR2);
> -	/*
> -	 * The field width is overloaded for some %p extensions to
> -	 * pass another piece of information. For plain pointers, the
> -	 * behaviour is slightly odd: One cannot pass either the 0
> -	 * flag nor a precision to %p without gcc complaining, and if
> -	 * one explicitly gives a field width, the number is no longer
> -	 * zero-padded.
> -	 */
> -	test("|" PTR1_STR PTR1_SPACES "  |  " PTR1_SPACES PTR1_STR "|",
> -	     "|%-*p|%*p|", PTR_WIDTH+2, PTR1, PTR_WIDTH+2, PTR1);
> -	test("|" PTR2_STR "  |  " PTR2_STR "|",
> -	     "|%-*p|%*p|", PTR_WIDTH+2, PTR2, PTR_WIDTH+2, PTR2);
> +	int err;
>  
> -	/*
> -	 * Unrecognized %p extensions are treated as plain %p, but the
> -	 * alphanumeric suffix is ignored (that is, does not occur in
> -	 * the output.)
> -	 */
> -	test("|"PTR1_ZEROES PTR1_STR"|", "|%p0y|", PTR1);
> -	test("|"PTR2_STR"|", "|%p0y|", PTR2);
> +	err = plain_hash();
> +	if (err) {
> +		pr_warn("plain 'p' does not appear to be hashed\n");
> +		failed_tests++;
> +		return;
> +	}
> +
> +	err = plain_format();
> +	if (err) {
> +		pr_warn("hashing plain 'p' has unexpected format\n");
> +		failed_tests++;
> +	}
>  }
>  
>  static void __init
> @@ -194,6 +225,7 @@ symbol_ptr(void)
>  static void __init
>  kernel_ptr(void)
>  {
> +	/* We can't test this without access to kptr_restrict. */
>  }
>  
>  static void __init
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 646009db4198..3cfeeaf0518d 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -31,6 +31,13 @@
>  #include <linux/dcache.h>
>  #include <linux/cred.h>
>  #include <net/addrconf.h>
> +#include <linux/siphash.h>
> +#include <linux/compiler.h>


> +#ifdef CONFIG_BLOCK
> +#include <linux/blkdev.h>
> +#endif
> +
> +#include "../mm/internal.h"	/* For the trace_print_flags arrays */
>  

This hunk is not needed from what I could see from history. It's worth build
testing anyway, but they should be left out.

Can you also test that it works as expected in respect to dmesg and
kptr_restrict values, specially for the earliest messages, as they depend on
random to be initialized?

Cascardo.

>  #include <asm/page.h>		/* for PAGE_SIZE */
>  #include <asm/sections.h>	/* for dereference_function_descriptor() */
> @@ -1360,6 +1367,73 @@ char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
>  
>  int kptr_restrict __read_mostly;
>  
> +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
> @@ -1451,6 +1525,9 @@ int kptr_restrict __read_mostly;
>   * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
>   * function pointers are really function descriptors, which contain a
>   * pointer to the real address.
> + *
> + * Note: The default behaviour (unadorned %p) is to hash the address,
> + * rendering it useful as a unique identifier.
>   */
>  static noinline_for_stack
>  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> @@ -1598,14 +1675,9 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  				   ((const struct file *)ptr)->f_path.dentry,
>  				   spec, fmt);
>  	}
> -	spec.flags |= SMALL;
> -	if (spec.field_width == -1) {
> -		spec.field_width = default_width;
> -		spec.flags |= ZEROPAD;
> -	}
> -	spec.base = 16;
>  
> -	return number(buf, end, (unsigned long) ptr, spec);
> +	/* default is to _not_ leak addresses, hash before printing */
> +	return ptr_to_id(buf, end, ptr, spec);
>  }
>  
>  /*
> -- 
> 2.17.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Tim Gardner March 1, 2021, 7:57 p.m. UTC | #2
On 2/24/21 1:26 PM, Thadeu Lima de Souza Cascardo wrote:
> On Wed, Feb 24, 2021 at 01:01:01PM -0700, Tim Gardner wrote:
>> From: "Tobin C. Harding" <me@tobin.cc>
>>
>> Currently there exist approximately 14 000 places in the kernel where
>> addresses are being printed using an unadorned %p. This potentially
>> leaks sensitive information regarding the Kernel layout in memory. Many
>> of these calls are stale, instead of fixing every call lets hash the
>> address by default before printing. This will of course break some
>> users, forcing code printing needed addresses to be updated.
>>
>> Code that _really_ needs the address will soon be able to use the new
>> printk specifier %px to print the address.
>>
>> For what it's worth, usage of unadorned %p can be broken down as
>> follows (thanks to Joe Perches).
>>
>> $ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
>>     1084 arch
>>       20 block
>>       10 crypto
>>       32 Documentation
>>     8121 drivers
>>     1221 fs
>>      143 include
>>      101 kernel
>>       69 lib
>>      100 mm
>>     1510 net
>>       40 samples
>>        7 scripts
>>       11 security
>>      166 sound
>>      152 tools
>>        2 virt
>>
>> Add function ptr_to_id() to map an address to a 32 bit unique
>> identifier. Hash any unadorned usage of specifier %p and any malformed
>> specifiers.
>>
>> Signed-off-by: Tobin C. Harding <me@tobin.cc>
>> (backported from commit ad67b74d2469d9b82aaa572d76474c95bc484d57)
>> CVE-2018-7273
>> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
>>
>> Conflicts:
>> 	Documentation/printk-formats.txt (Added a paragraph under Kernel Pointers)
>> 	lib/vsprintf.c (required additional include files, no code changes)
>> ---
>>   Documentation/printk-formats.txt |   6 ++
>>   lib/test_printf.c                | 108 ++++++++++++++++++++-----------
>>   lib/vsprintf.c                   |  86 ++++++++++++++++++++++--
>>   3 files changed, 155 insertions(+), 45 deletions(-)
>>
>> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
>> index ed6f6abaad57..2329b1eac0fa 100644
>> --- a/Documentation/printk-formats.txt
>> +++ b/Documentation/printk-formats.txt
>> @@ -64,6 +64,12 @@ Kernel Pointers:
>>   	users. The behaviour of %pK depends on the kptr_restrict sysctl - see
>>   	Documentation/sysctl/kernel.txt for more details.
>>   
>> +	Pointers printed without a specifier extension (i.e unadorned %p) are
>> +	hashed to give a unique identifier without leaking kernel addresses to user
>> +	space. On 64 bit machines the first 32 bits are zeroed.
>> +
>> +		%p	abcdef12 or 00000000abcdef12
>> +
>>   Struct Resources:
>>   
>>   	%pr	[mem 0x60000000-0x6fffffff flags 0x2200] or
>> diff --git a/lib/test_printf.c b/lib/test_printf.c
>> index c5a666af9ba5..e2200f06f168 100644
>> --- a/lib/test_printf.c
>> +++ b/lib/test_printf.c
>> @@ -18,24 +18,6 @@
>>   #define BUF_SIZE 256
>>   #define FILL_CHAR '$'
>>   
>> -#define PTR1 ((void*)0x01234567)
>> -#define PTR2 ((void*)(long)(int)0xfedcba98)
>> -
>> -#if BITS_PER_LONG == 64
>> -#define PTR1_ZEROES "000000000"
>> -#define PTR1_SPACES "         "
>> -#define PTR1_STR "1234567"
>> -#define PTR2_STR "fffffffffedcba98"
>> -#define PTR_WIDTH 16
>> -#else
>> -#define PTR1_ZEROES "0"
>> -#define PTR1_SPACES " "
>> -#define PTR1_STR "1234567"
>> -#define PTR2_STR "fedcba98"
>> -#define PTR_WIDTH 8
>> -#endif
>> -#define PTR_WIDTH_STR stringify(PTR_WIDTH)
>> -
>>   static unsigned total_tests __initdata;
>>   static unsigned failed_tests __initdata;
>>   static char *test_buffer __initdata;
>> @@ -160,30 +142,79 @@ test_string(void)
>>   	test("a  |   |   ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, "c");
>>   }
>>   
>> +#define PLAIN_BUF_SIZE 64	/* leave some space so we don't oops */
>> +
>> +#if BITS_PER_LONG == 64
>> +
>> +#define PTR_WIDTH 16
>> +#define PTR ((void *)0xffff0123456789ab)
>> +#define PTR_STR "ffff0123456789ab"
>> +#define ZEROS "00000000"	/* hex 32 zero bits */
>> +
>> +static int __init
>> +plain_format(void)
>> +{
>> +	char buf[PLAIN_BUF_SIZE];
>> +	int nchars;
>> +
>> +	nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR);
>> +
>> +	if (nchars != PTR_WIDTH || strncmp(buf, ZEROS, strlen(ZEROS)) != 0)
>> +		return -1;
>> +
>> +	return 0;
>> +}
>> +
>> +#else
>> +
>> +#define PTR_WIDTH 8
>> +#define PTR ((void *)0x456789ab)
>> +#define PTR_STR "456789ab"
>> +
>> +static int __init
>> +plain_format(void)
>> +{
>> +	/* Format is implicitly tested for 32 bit machines by plain_hash() */
>> +	return 0;
>> +}
>> +
>> +#endif	/* BITS_PER_LONG == 64 */
>> +
>> +static int __init
>> +plain_hash(void)
>> +{
>> +	char buf[PLAIN_BUF_SIZE];
>> +	int nchars;
>> +
>> +	nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR);
>> +
>> +	if (nchars != PTR_WIDTH || strncmp(buf, PTR_STR, PTR_WIDTH) == 0)
>> +		return -1;
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * We can't use test() to test %p because we don't know what output to expect
>> + * after an address is hashed.
>> + */
>>   static void __init
>>   plain(void)
>>   {
>> -	test(PTR1_ZEROES PTR1_STR " " PTR2_STR, "%p %p", PTR1, PTR2);
>> -	/*
>> -	 * The field width is overloaded for some %p extensions to
>> -	 * pass another piece of information. For plain pointers, the
>> -	 * behaviour is slightly odd: One cannot pass either the 0
>> -	 * flag nor a precision to %p without gcc complaining, and if
>> -	 * one explicitly gives a field width, the number is no longer
>> -	 * zero-padded.
>> -	 */
>> -	test("|" PTR1_STR PTR1_SPACES "  |  " PTR1_SPACES PTR1_STR "|",
>> -	     "|%-*p|%*p|", PTR_WIDTH+2, PTR1, PTR_WIDTH+2, PTR1);
>> -	test("|" PTR2_STR "  |  " PTR2_STR "|",
>> -	     "|%-*p|%*p|", PTR_WIDTH+2, PTR2, PTR_WIDTH+2, PTR2);
>> +	int err;
>>   
>> -	/*
>> -	 * Unrecognized %p extensions are treated as plain %p, but the
>> -	 * alphanumeric suffix is ignored (that is, does not occur in
>> -	 * the output.)
>> -	 */
>> -	test("|"PTR1_ZEROES PTR1_STR"|", "|%p0y|", PTR1);
>> -	test("|"PTR2_STR"|", "|%p0y|", PTR2);
>> +	err = plain_hash();
>> +	if (err) {
>> +		pr_warn("plain 'p' does not appear to be hashed\n");
>> +		failed_tests++;
>> +		return;
>> +	}
>> +
>> +	err = plain_format();
>> +	if (err) {
>> +		pr_warn("hashing plain 'p' has unexpected format\n");
>> +		failed_tests++;
>> +	}
>>   }
>>   
>>   static void __init
>> @@ -194,6 +225,7 @@ symbol_ptr(void)
>>   static void __init
>>   kernel_ptr(void)
>>   {
>> +	/* We can't test this without access to kptr_restrict. */
>>   }
>>   
>>   static void __init
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index 646009db4198..3cfeeaf0518d 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -31,6 +31,13 @@
>>   #include <linux/dcache.h>
>>   #include <linux/cred.h>
>>   #include <net/addrconf.h>
>> +#include <linux/siphash.h>
>> +#include <linux/compiler.h>
> 
> 
>> +#ifdef CONFIG_BLOCK
>> +#include <linux/blkdev.h>
>> +#endif
>> +
>> +#include "../mm/internal.h"	/* For the trace_print_flags arrays */
>>   
> 
> This hunk is not needed from what I could see from history. It's worth build
> testing anyway, but they should be left out.
> 

Indeed, those include files are not required.

> Can you also test that it works as expected in respect to dmesg and
> kptr_restrict values, specially for the earliest messages, as they depend on
> random to be initialized?
> 

The %pK format (kptr_restrict) is natively supported in Xenial. One has 
to assume upstream thought about when kptr_restrict!=0 could be used 
with respect to when /dev/random is initialized. The use of an unadorned 
%p format early in boot is a little tougher to predict wrt randomness. 
However, even if /dev/random isn't initialized, we're no worse off then 
we were before. Right ?

rtg

> Cascardo.
> 
>>   #include <asm/page.h>		/* for PAGE_SIZE */
>>   #include <asm/sections.h>	/* for dereference_function_descriptor() */
>> @@ -1360,6 +1367,73 @@ char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
>>   
>>   int kptr_restrict __read_mostly;
>>   
>> +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
>> @@ -1451,6 +1525,9 @@ int kptr_restrict __read_mostly;
>>    * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
>>    * function pointers are really function descriptors, which contain a
>>    * pointer to the real address.
>> + *
>> + * Note: The default behaviour (unadorned %p) is to hash the address,
>> + * rendering it useful as a unique identifier.
>>    */
>>   static noinline_for_stack
>>   char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>> @@ -1598,14 +1675,9 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>>   				   ((const struct file *)ptr)->f_path.dentry,
>>   				   spec, fmt);
>>   	}
>> -	spec.flags |= SMALL;
>> -	if (spec.field_width == -1) {
>> -		spec.field_width = default_width;
>> -		spec.flags |= ZEROPAD;
>> -	}
>> -	spec.base = 16;
>>   
>> -	return number(buf, end, (unsigned long) ptr, spec);
>> +	/* default is to _not_ leak addresses, hash before printing */
>> +	return ptr_to_id(buf, end, ptr, spec);
>>   }
>>   
>>   /*
>> -- 
>> 2.17.1
>>
>>
>> -- 
>> kernel-team mailing list
>> kernel-team@lists.ubuntu.com
>> https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff mbox series

Patch

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index ed6f6abaad57..2329b1eac0fa 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -64,6 +64,12 @@  Kernel Pointers:
 	users. The behaviour of %pK depends on the kptr_restrict sysctl - see
 	Documentation/sysctl/kernel.txt for more details.
 
+	Pointers printed without a specifier extension (i.e unadorned %p) are
+	hashed to give a unique identifier without leaking kernel addresses to user
+	space. On 64 bit machines the first 32 bits are zeroed.
+
+		%p	abcdef12 or 00000000abcdef12
+
 Struct Resources:
 
 	%pr	[mem 0x60000000-0x6fffffff flags 0x2200] or
diff --git a/lib/test_printf.c b/lib/test_printf.c
index c5a666af9ba5..e2200f06f168 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -18,24 +18,6 @@ 
 #define BUF_SIZE 256
 #define FILL_CHAR '$'
 
-#define PTR1 ((void*)0x01234567)
-#define PTR2 ((void*)(long)(int)0xfedcba98)
-
-#if BITS_PER_LONG == 64
-#define PTR1_ZEROES "000000000"
-#define PTR1_SPACES "         "
-#define PTR1_STR "1234567"
-#define PTR2_STR "fffffffffedcba98"
-#define PTR_WIDTH 16
-#else
-#define PTR1_ZEROES "0"
-#define PTR1_SPACES " "
-#define PTR1_STR "1234567"
-#define PTR2_STR "fedcba98"
-#define PTR_WIDTH 8
-#endif
-#define PTR_WIDTH_STR stringify(PTR_WIDTH)
-
 static unsigned total_tests __initdata;
 static unsigned failed_tests __initdata;
 static char *test_buffer __initdata;
@@ -160,30 +142,79 @@  test_string(void)
 	test("a  |   |   ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, "c");
 }
 
+#define PLAIN_BUF_SIZE 64	/* leave some space so we don't oops */
+
+#if BITS_PER_LONG == 64
+
+#define PTR_WIDTH 16
+#define PTR ((void *)0xffff0123456789ab)
+#define PTR_STR "ffff0123456789ab"
+#define ZEROS "00000000"	/* hex 32 zero bits */
+
+static int __init
+plain_format(void)
+{
+	char buf[PLAIN_BUF_SIZE];
+	int nchars;
+
+	nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR);
+
+	if (nchars != PTR_WIDTH || strncmp(buf, ZEROS, strlen(ZEROS)) != 0)
+		return -1;
+
+	return 0;
+}
+
+#else
+
+#define PTR_WIDTH 8
+#define PTR ((void *)0x456789ab)
+#define PTR_STR "456789ab"
+
+static int __init
+plain_format(void)
+{
+	/* Format is implicitly tested for 32 bit machines by plain_hash() */
+	return 0;
+}
+
+#endif	/* BITS_PER_LONG == 64 */
+
+static int __init
+plain_hash(void)
+{
+	char buf[PLAIN_BUF_SIZE];
+	int nchars;
+
+	nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR);
+
+	if (nchars != PTR_WIDTH || strncmp(buf, PTR_STR, PTR_WIDTH) == 0)
+		return -1;
+
+	return 0;
+}
+
+/*
+ * We can't use test() to test %p because we don't know what output to expect
+ * after an address is hashed.
+ */
 static void __init
 plain(void)
 {
-	test(PTR1_ZEROES PTR1_STR " " PTR2_STR, "%p %p", PTR1, PTR2);
-	/*
-	 * The field width is overloaded for some %p extensions to
-	 * pass another piece of information. For plain pointers, the
-	 * behaviour is slightly odd: One cannot pass either the 0
-	 * flag nor a precision to %p without gcc complaining, and if
-	 * one explicitly gives a field width, the number is no longer
-	 * zero-padded.
-	 */
-	test("|" PTR1_STR PTR1_SPACES "  |  " PTR1_SPACES PTR1_STR "|",
-	     "|%-*p|%*p|", PTR_WIDTH+2, PTR1, PTR_WIDTH+2, PTR1);
-	test("|" PTR2_STR "  |  " PTR2_STR "|",
-	     "|%-*p|%*p|", PTR_WIDTH+2, PTR2, PTR_WIDTH+2, PTR2);
+	int err;
 
-	/*
-	 * Unrecognized %p extensions are treated as plain %p, but the
-	 * alphanumeric suffix is ignored (that is, does not occur in
-	 * the output.)
-	 */
-	test("|"PTR1_ZEROES PTR1_STR"|", "|%p0y|", PTR1);
-	test("|"PTR2_STR"|", "|%p0y|", PTR2);
+	err = plain_hash();
+	if (err) {
+		pr_warn("plain 'p' does not appear to be hashed\n");
+		failed_tests++;
+		return;
+	}
+
+	err = plain_format();
+	if (err) {
+		pr_warn("hashing plain 'p' has unexpected format\n");
+		failed_tests++;
+	}
 }
 
 static void __init
@@ -194,6 +225,7 @@  symbol_ptr(void)
 static void __init
 kernel_ptr(void)
 {
+	/* We can't test this without access to kptr_restrict. */
 }
 
 static void __init
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 646009db4198..3cfeeaf0518d 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -31,6 +31,13 @@ 
 #include <linux/dcache.h>
 #include <linux/cred.h>
 #include <net/addrconf.h>
+#include <linux/siphash.h>
+#include <linux/compiler.h>
+#ifdef CONFIG_BLOCK
+#include <linux/blkdev.h>
+#endif
+
+#include "../mm/internal.h"	/* For the trace_print_flags arrays */
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/sections.h>	/* for dereference_function_descriptor() */
@@ -1360,6 +1367,73 @@  char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
 
 int kptr_restrict __read_mostly;
 
+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
@@ -1451,6 +1525,9 @@  int kptr_restrict __read_mostly;
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a
  * pointer to the real address.
+ *
+ * Note: The default behaviour (unadorned %p) is to hash the address,
+ * rendering it useful as a unique identifier.
  */
 static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
@@ -1598,14 +1675,9 @@  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 				   ((const struct file *)ptr)->f_path.dentry,
 				   spec, fmt);
 	}
-	spec.flags |= SMALL;
-	if (spec.field_width == -1) {
-		spec.field_width = default_width;
-		spec.flags |= ZEROPAD;
-	}
-	spec.base = 16;
 
-	return number(buf, end, (unsigned long) ptr, spec);
+	/* default is to _not_ leak addresses, hash before printing */
+	return ptr_to_id(buf, end, ptr, spec);
 }
 
 /*