[PATCH v2] printk: hash addresses printed with %p

[PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

Currently there are many places in the kernel where addresses are being
printed using an unadorned %p. Kernel pointers should be printed using
%pK allowing some control via the kptr_restrict sysctl. Exposing addresses
gives attackers sensitive information about the kernel layout in memory.

We can reduce the attack surface by hashing all addresses printed with
%p. This will of course break some users, forcing code printing needed
addresses to be updated.

For what it's worth, usage of unadorned %p can be broken down as follows

    git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l

arch: 2512
block: 20
crypto: 12
fs: 1221
include: 147
kernel: 109
lib: 77
mm: 120
net: 1516
security: 11
sound: 168
virt: 2
drivers: 8420

Add helper function siphash_1ulong(). Add function ptr_to_id() to map an
address to a 32 bit unique identifier.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---

V2:
 - Use SipHash to do the hashing

The discussion related to this patch has been fragmented. There are
three other threads associated with this patch. Email threads by
subject:

[PATCH] printk: hash addresses printed with %p
[PATCH 0/3] add %pX specifier
[kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options

 include/linux/siphash.h |  2 ++
 lib/siphash.c           | 13 +++++++++++++
 lib/vsprintf.c          | 32 +++++++++++++++++++++++++++++---
 3 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/include/linux/siphash.h b/include/linux/siphash.h
index fa7a6b9cedbf..a9392568c8b8 100644
--- a/include/linux/siphash.h
+++ b/include/linux/siphash.h
@@ -26,6 +26,8 @@ u64 __siphash_aligned(const void *data, size_t len, const siphash_key_t *key);
 u64 __siphash_unaligned(const void *data, size_t len, const siphash_key_t *key);
 #endif
 
+unsigned long siphash_1ulong(const unsigned long a, const siphash_key_t *key);
+
 u64 siphash_1u64(const u64 a, const siphash_key_t *key);
 u64 siphash_2u64(const u64 a, const u64 b, const siphash_key_t *key);
 u64 siphash_3u64(const u64 a, const u64 b, const u64 c,
diff --git a/lib/siphash.c b/lib/siphash.c
index 3ae58b4edad6..63f4ff57c9ce 100644
--- a/lib/siphash.c
+++ b/lib/siphash.c
@@ -116,6 +116,19 @@ EXPORT_SYMBOL(__siphash_unaligned);
 #endif
 
 /**
+ * siphash_1ulong - computes siphash PRF value
+ * @first: value to hash
+ * @key: the siphash key
+ */
+unsigned long siphash_1ulong(const unsigned long first, const siphash_key_t *key)
+{
+#ifdef CONFIG_64BIT
+	return (unsigned long)siphash_1u64((u64)first, key);
+#endif
+	return (unsigned long)siphash_1u32((u32)first, key);
+}
+
+/**
  * siphash_1u64 - compute 64-bit siphash PRF value of a u64
  * @first: first u64
  * @key: the siphash key
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86c3385b9eb3..afd1c835b0f6 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -33,6 +33,7 @@
 #include <linux/uuid.h>
 #include <linux/of.h>
 #include <net/addrconf.h>
+#include <linux/siphash.h>
 #ifdef CONFIG_BLOCK
 #include <linux/blkdev.h>
 #endif
@@ -503,6 +504,7 @@ char *number(char *buf, char *end, unsigned long long num,
 			*buf = '0';
 		++buf;
 	}
+
 	/* actual digits of result */
 	while (--i >= 0) {
 		if (buf < end)
@@ -1591,6 +1593,28 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
+/* Maps a pointer to a 32 bit unique identifier. */
+static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
+{
+	static siphash_key_t ptr_secret __read_mostly;
+	static bool have_key = false;
+	unsigned long hashval;
+
+	/* Kernel doesn't boot if we use get_random_once() */
+	if (!have_key) {
+		get_random_bytes(&ptr_secret, sizeof(ptr_secret));
+		have_key = true;
+	}
+
+	hashval = siphash_1ulong((unsigned long)ptr, &ptr_secret);
+
+	spec.field_width = 2 + 2 * sizeof(unsigned int); /* 0x + hex */
+	spec.flags = SPECIAL | SMALL | ZEROPAD;
+	spec.base = 16;
+
+	return number(buf, end, (u32)hashval, spec);
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1703,6 +1727,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.
+ *
+ * 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,
@@ -1858,14 +1885,13 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			return device_node_string(buf, end, ptr, spec, fmt + 1);
 		}
 	}
-	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);
+	return ptr_to_id(buf, end, ptr, spec);
 }
 
 /*
-- 
2.7.4

[kernel-hardening] [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

Currently there are many places in the kernel where addresses are being
printed using an unadorned %p. Kernel pointers should be printed using
%pK allowing some control via the kptr_restrict sysctl. Exposing addresses
gives attackers sensitive information about the kernel layout in memory.

We can reduce the attack surface by hashing all addresses printed with
%p. This will of course break some users, forcing code printing needed
addresses to be updated.

For what it's worth, usage of unadorned %p can be broken down as follows

    git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l

arch: 2512
block: 20
crypto: 12
fs: 1221
include: 147
kernel: 109
lib: 77
mm: 120
net: 1516
security: 11
sound: 168
virt: 2
drivers: 8420

Add helper function siphash_1ulong(). Add function ptr_to_id() to map an
address to a 32 bit unique identifier.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---

V2:
 - Use SipHash to do the hashing

The discussion related to this patch has been fragmented. There are
three other threads associated with this patch. Email threads by
subject:

[PATCH] printk: hash addresses printed with %p
[PATCH 0/3] add %pX specifier
[kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options

 include/linux/siphash.h |  2 ++
 lib/siphash.c           | 13 +++++++++++++
 lib/vsprintf.c          | 32 +++++++++++++++++++++++++++++---
 3 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/include/linux/siphash.h b/include/linux/siphash.h
index fa7a6b9cedbf..a9392568c8b8 100644
--- a/include/linux/siphash.h
+++ b/include/linux/siphash.h
@@ -26,6 +26,8 @@ u64 __siphash_aligned(const void *data, size_t len, const siphash_key_t *key);
 u64 __siphash_unaligned(const void *data, size_t len, const siphash_key_t *key);
 #endif
 
+unsigned long siphash_1ulong(const unsigned long a, const siphash_key_t *key);
+
 u64 siphash_1u64(const u64 a, const siphash_key_t *key);
 u64 siphash_2u64(const u64 a, const u64 b, const siphash_key_t *key);
 u64 siphash_3u64(const u64 a, const u64 b, const u64 c,
diff --git a/lib/siphash.c b/lib/siphash.c
index 3ae58b4edad6..63f4ff57c9ce 100644
--- a/lib/siphash.c
+++ b/lib/siphash.c
@@ -116,6 +116,19 @@ EXPORT_SYMBOL(__siphash_unaligned);
 #endif
 
 /**
+ * siphash_1ulong - computes siphash PRF value
+ * @first: value to hash
+ * @key: the siphash key
+ */
+unsigned long siphash_1ulong(const unsigned long first, const siphash_key_t *key)
+{
+#ifdef CONFIG_64BIT
+	return (unsigned long)siphash_1u64((u64)first, key);
+#endif
+	return (unsigned long)siphash_1u32((u32)first, key);
+}
+
+/**
  * siphash_1u64 - compute 64-bit siphash PRF value of a u64
  * @first: first u64
  * @key: the siphash key
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86c3385b9eb3..afd1c835b0f6 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -33,6 +33,7 @@
 #include <linux/uuid.h>
 #include <linux/of.h>
 #include <net/addrconf.h>
+#include <linux/siphash.h>
 #ifdef CONFIG_BLOCK
 #include <linux/blkdev.h>
 #endif
@@ -503,6 +504,7 @@ char *number(char *buf, char *end, unsigned long long num,
 			*buf = '0';
 		++buf;
 	}
+
 	/* actual digits of result */
 	while (--i >= 0) {
 		if (buf < end)
@@ -1591,6 +1593,28 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
+/* Maps a pointer to a 32 bit unique identifier. */
+static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
+{
+	static siphash_key_t ptr_secret __read_mostly;
+	static bool have_key = false;
+	unsigned long hashval;
+
+	/* Kernel doesn't boot if we use get_random_once() */
+	if (!have_key) {
+		get_random_bytes(&ptr_secret, sizeof(ptr_secret));
+		have_key = true;
+	}
+
+	hashval = siphash_1ulong((unsigned long)ptr, &ptr_secret);
+
+	spec.field_width = 2 + 2 * sizeof(unsigned int); /* 0x + hex */
+	spec.flags = SPECIAL | SMALL | ZEROPAD;
+	spec.base = 16;
+
+	return number(buf, end, (u32)hashval, spec);
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1703,6 +1727,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.
+ *
+ * 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,
@@ -1858,14 +1885,13 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			return device_node_string(buf, end, ptr, spec, fmt + 1);
 		}
 	}
-	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);
+	return ptr_to_id(buf, end, ptr, spec);
 }
 
 /*
-- 
2.7.4

Re: [PATCH v2] printk: hash addresses printed with %p

[Joe Perches]

On Tue, 2017-10-17 at 15:52 +1100, Tobin C. Harding wrote:
> Currently there are many places in the kernel where addresses are being
> printed using an unadorned %p. Kernel pointers should be printed using
> %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> gives attackers sensitive information about the kernel layout in memory.
> 
> We can reduce the attack surface by hashing all addresses printed with
> %p. This will of course break some users, forcing code printing needed
> addresses to be updated.
> 
> For what it's worth, usage of unadorned %p can be broken down as follows
> 
>     git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l

Not really.
There are many asm uses included there

I think a better grep is:

$ 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

> arch: 2512

arch is especially overestimated.

[kernel-hardening] Re: [PATCH v2] printk: hash addresses printed with %p

[Joe Perches]

On Tue, 2017-10-17 at 15:52 +1100, Tobin C. Harding wrote:
> Currently there are many places in the kernel where addresses are being
> printed using an unadorned %p. Kernel pointers should be printed using
> %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> gives attackers sensitive information about the kernel layout in memory.
> 
> We can reduce the attack surface by hashing all addresses printed with
> %p. This will of course break some users, forcing code printing needed
> addresses to be updated.
> 
> For what it's worth, usage of unadorned %p can be broken down as follows
> 
>     git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l

Not really.
There are many asm uses included there

I think a better grep is:

$ 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

> arch: 2512

arch is especially overestimated.

Re: [PATCH v2] printk: hash addresses printed with %p

[Steven Rostedt]

On Tue, 17 Oct 2017 15:52:51 +1100
"Tobin C. Harding" <me@tobin.cc> wrote:

> Currently there are many places in the kernel where addresses are being
> printed using an unadorned %p. Kernel pointers should be printed using
> %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> gives attackers sensitive information about the kernel layout in memory.
> 
> We can reduce the attack surface by hashing all addresses printed with
> %p. This will of course break some users, forcing code printing needed
> addresses to be updated.
> 
> For what it's worth, usage of unadorned %p can be broken down as follows
> 
>     git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l

Does %p[FfSs] leak addresses? Well, I guess it does if they are not
found in kallsyms, but otherwise you have:

  function+0x<offset>

-- Steve


> 
> arch: 2512
> block: 20
> crypto: 12
> fs: 1221
> include: 147
> kernel: 109
> lib: 77
> mm: 120
> net: 1516
> security: 11
> sound: 168
> virt: 2
> drivers: 8420
> 
> Add helper function siphash_1ulong(). Add function ptr_to_id() to map an
> address to a 32 bit unique identifier.
> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> ---
>

[kernel-hardening] Re: [PATCH v2] printk: hash addresses printed with %p

[Steven Rostedt]

On Tue, 17 Oct 2017 15:52:51 +1100
"Tobin C. Harding" <me@tobin.cc> wrote:

> Currently there are many places in the kernel where addresses are being
> printed using an unadorned %p. Kernel pointers should be printed using
> %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> gives attackers sensitive information about the kernel layout in memory.
> 
> We can reduce the attack surface by hashing all addresses printed with
> %p. This will of course break some users, forcing code printing needed
> addresses to be updated.
> 
> For what it's worth, usage of unadorned %p can be broken down as follows
> 
>     git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l

Does %p[FfSs] leak addresses? Well, I guess it does if they are not
found in kallsyms, but otherwise you have:

  function+0x<offset>

-- Steve


> 
> arch: 2512
> block: 20
> crypto: 12
> fs: 1221
> include: 147
> kernel: 109
> lib: 77
> mm: 120
> net: 1516
> security: 11
> sound: 168
> virt: 2
> drivers: 8420
> 
> Add helper function siphash_1ulong(). Add function ptr_to_id() to map an
> address to a 32 bit unique identifier.
> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> ---
>

RE: [PATCH v2] printk: hash addresses printed with %p

[Roberts, William C]

> -----Original Message-----
> From: Tobin C. Harding [mailto:me@tobin.cc]
> Sent: Monday, October 16, 2017 9:53 PM
> To: kernel-hardening@lists.openwall.com
> Cc: Tobin C. Harding <me@tobin.cc>; Linus Torvalds <torvalds@linux-
> foundation.org>; Kees Cook <keescook@chromium.org>; Paolo Bonzini
> <pbonzini@redhat.com>; Tycho Andersen <tycho@docker.com>; Roberts,
> William C <william.c.roberts@intel.com>; Tejun Heo <tj@kernel.org>; Jordan
> Glover <Golden_Miller83@protonmail.ch>; Greg KH
> <gregkh@linuxfoundation.org>; Petr Mladek <pmladek@suse.com>; Joe
> Perches <joe@perches.com>; Ian Campbell <ijc@hellion.org.uk>; Sergey
> Senozhatsky <sergey.senozhatsky@gmail.com>; Catalin Marinas
> <catalin.marinas@arm.com>; Will Deacon <will.deacon@arm.com>; Steven
> Rostedt <rostedt@goodmis.org>; Chris Fries <cfries@google.com>; Dave
> Weinstein <olorin@google.com>; Daniel Micay <danielmicay@gmail.com>; Djalal
> Harouni <tixxdz@gmail.com>; linux-kernel@vger.kernel.org
> Subject: [PATCH v2] printk: hash addresses printed with %p
> 
> Currently there are many places in the kernel where addresses are being printed
> using an unadorned %p. Kernel pointers should be printed using %pK allowing
> some control via the kptr_restrict sysctl. Exposing addresses gives attackers
> sensitive information about the kernel layout in memory.
> 
> We can reduce the attack surface by hashing all addresses printed with %p. This
> will of course break some users, forcing code printing needed addresses to be
> updated.
> 
> For what it's worth, usage of unadorned %p can be broken down as follows
> 
>     git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> 
> arch: 2512
> block: 20
> crypto: 12
> fs: 1221
> include: 147
> kernel: 109
> lib: 77
> mm: 120
> net: 1516
> security: 11
> sound: 168
> virt: 2
> drivers: 8420
> 
> Add helper function siphash_1ulong(). Add function ptr_to_id() to map an
> address to a 32 bit unique identifier.
> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> ---
> 
> V2:
>  - Use SipHash to do the hashing
> 
> The discussion related to this patch has been fragmented. There are three other
> threads associated with this patch. Email threads by
> subject:
> 
> [PATCH] printk: hash addresses printed with %p [PATCH 0/3] add %pX specifier
> [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options
> 
>  include/linux/siphash.h |  2 ++
>  lib/siphash.c           | 13 +++++++++++++
>  lib/vsprintf.c          | 32 +++++++++++++++++++++++++++++---
>  3 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/siphash.h b/include/linux/siphash.h index
> fa7a6b9cedbf..a9392568c8b8 100644
> --- a/include/linux/siphash.h
> +++ b/include/linux/siphash.h
> @@ -26,6 +26,8 @@ u64 __siphash_aligned(const void *data, size_t len, const
> siphash_key_t *key);
>  u64 __siphash_unaligned(const void *data, size_t len, const siphash_key_t
> *key);  #endif
> 
> +unsigned long siphash_1ulong(const unsigned long a, const siphash_key_t
> +*key);
> +
>  u64 siphash_1u64(const u64 a, const siphash_key_t *key);
>  u64 siphash_2u64(const u64 a, const u64 b, const siphash_key_t *key);
>  u64 siphash_3u64(const u64 a, const u64 b, const u64 c, diff --git a/lib/siphash.c
> b/lib/siphash.c index 3ae58b4edad6..63f4ff57c9ce 100644
> --- a/lib/siphash.c
> +++ b/lib/siphash.c
> @@ -116,6 +116,19 @@ EXPORT_SYMBOL(__siphash_unaligned);
>  #endif
> 
>  /**
> + * siphash_1ulong - computes siphash PRF value
> + * @first: value to hash
> + * @key: the siphash key
> + */
> +unsigned long siphash_1ulong(const unsigned long first, const
> +siphash_key_t *key) { #ifdef CONFIG_64BIT
> +	return (unsigned long)siphash_1u64((u64)first, key); #endif
> +	return (unsigned long)siphash_1u32((u32)first, key); }
> +
> +/**
>   * siphash_1u64 - compute 64-bit siphash PRF value of a u64
>   * @first: first u64
>   * @key: the siphash key
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 86c3385b9eb3..afd1c835b0f6 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -33,6 +33,7 @@
>  #include <linux/uuid.h>
>  #include <linux/of.h>
>  #include <net/addrconf.h>
> +#include <linux/siphash.h>
>  #ifdef CONFIG_BLOCK
>  #include <linux/blkdev.h>
>  #endif
> @@ -503,6 +504,7 @@ char *number(char *buf, char *end, unsigned long long
> num,
>  			*buf = '0';
>  		++buf;
>  	}
> +

Unneeded whitespace change?

>  	/* actual digits of result */
>  	while (--i >= 0) {
>  		if (buf < end)
> @@ -1591,6 +1593,28 @@ char *device_node_string(char *buf, char *end, struct
> device_node *dn,
>  	return widen_string(buf, buf - buf_start, end, spec);  }
> 
> +/* Maps a pointer to a 32 bit unique identifier. */ static char
> +*ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec) {
> +	static siphash_key_t ptr_secret __read_mostly;
> +	static bool have_key = false;
> +	unsigned long hashval;
> +
> +	/* Kernel doesn't boot if we use get_random_once() */
> +	if (!have_key) {
> +		get_random_bytes(&ptr_secret, sizeof(ptr_secret));
> +		have_key = true;

Wouldn't one want to use an atomic test and swap for this
block?

> +	}
> +
> +	hashval = siphash_1ulong((unsigned long)ptr, &ptr_secret);
> +
> +	spec.field_width = 2 + 2 * sizeof(unsigned int); /* 0x + hex */
> +	spec.flags = SPECIAL | SMALL | ZEROPAD;
> +	spec.base = 16;
> +
> +	return number(buf, end, (u32)hashval, spec); }
> +
>  int kptr_restrict __read_mostly;
> 
>  /*
> @@ -1703,6 +1727,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.
> + *
> + * 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, @@ -1858,14
> +1885,13 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  			return device_node_string(buf, end, ptr, spec, fmt + 1);
>  		}
>  	}
> -	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);
> +	return ptr_to_id(buf, end, ptr, spec);
>  }
> 
>  /*
> --
> 2.7.4

[kernel-hardening] RE: [PATCH v2] printk: hash addresses printed with %p

[Roberts, William C]

> -----Original Message-----
> From: Tobin C. Harding [mailto:me@tobin.cc]
> Sent: Monday, October 16, 2017 9:53 PM
> To: kernel-hardening@lists.openwall.com
> Cc: Tobin C. Harding <me@tobin.cc>; Linus Torvalds <torvalds@linux-
> foundation.org>; Kees Cook <keescook@chromium.org>; Paolo Bonzini
> <pbonzini@redhat.com>; Tycho Andersen <tycho@docker.com>; Roberts,
> William C <william.c.roberts@intel.com>; Tejun Heo <tj@kernel.org>; Jordan
> Glover <Golden_Miller83@protonmail.ch>; Greg KH
> <gregkh@linuxfoundation.org>; Petr Mladek <pmladek@suse.com>; Joe
> Perches <joe@perches.com>; Ian Campbell <ijc@hellion.org.uk>; Sergey
> Senozhatsky <sergey.senozhatsky@gmail.com>; Catalin Marinas
> <catalin.marinas@arm.com>; Will Deacon <will.deacon@arm.com>; Steven
> Rostedt <rostedt@goodmis.org>; Chris Fries <cfries@google.com>; Dave
> Weinstein <olorin@google.com>; Daniel Micay <danielmicay@gmail.com>; Djalal
> Harouni <tixxdz@gmail.com>; linux-kernel@vger.kernel.org
> Subject: [PATCH v2] printk: hash addresses printed with %p
> 
> Currently there are many places in the kernel where addresses are being printed
> using an unadorned %p. Kernel pointers should be printed using %pK allowing
> some control via the kptr_restrict sysctl. Exposing addresses gives attackers
> sensitive information about the kernel layout in memory.
> 
> We can reduce the attack surface by hashing all addresses printed with %p. This
> will of course break some users, forcing code printing needed addresses to be
> updated.
> 
> For what it's worth, usage of unadorned %p can be broken down as follows
> 
>     git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> 
> arch: 2512
> block: 20
> crypto: 12
> fs: 1221
> include: 147
> kernel: 109
> lib: 77
> mm: 120
> net: 1516
> security: 11
> sound: 168
> virt: 2
> drivers: 8420
> 
> Add helper function siphash_1ulong(). Add function ptr_to_id() to map an
> address to a 32 bit unique identifier.
> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> ---
> 
> V2:
>  - Use SipHash to do the hashing
> 
> The discussion related to this patch has been fragmented. There are three other
> threads associated with this patch. Email threads by
> subject:
> 
> [PATCH] printk: hash addresses printed with %p [PATCH 0/3] add %pX specifier
> [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options
> 
>  include/linux/siphash.h |  2 ++
>  lib/siphash.c           | 13 +++++++++++++
>  lib/vsprintf.c          | 32 +++++++++++++++++++++++++++++---
>  3 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/siphash.h b/include/linux/siphash.h index
> fa7a6b9cedbf..a9392568c8b8 100644
> --- a/include/linux/siphash.h
> +++ b/include/linux/siphash.h
> @@ -26,6 +26,8 @@ u64 __siphash_aligned(const void *data, size_t len, const
> siphash_key_t *key);
>  u64 __siphash_unaligned(const void *data, size_t len, const siphash_key_t
> *key);  #endif
> 
> +unsigned long siphash_1ulong(const unsigned long a, const siphash_key_t
> +*key);
> +
>  u64 siphash_1u64(const u64 a, const siphash_key_t *key);
>  u64 siphash_2u64(const u64 a, const u64 b, const siphash_key_t *key);
>  u64 siphash_3u64(const u64 a, const u64 b, const u64 c, diff --git a/lib/siphash.c
> b/lib/siphash.c index 3ae58b4edad6..63f4ff57c9ce 100644
> --- a/lib/siphash.c
> +++ b/lib/siphash.c
> @@ -116,6 +116,19 @@ EXPORT_SYMBOL(__siphash_unaligned);
>  #endif
> 
>  /**
> + * siphash_1ulong - computes siphash PRF value
> + * @first: value to hash
> + * @key: the siphash key
> + */
> +unsigned long siphash_1ulong(const unsigned long first, const
> +siphash_key_t *key) { #ifdef CONFIG_64BIT
> +	return (unsigned long)siphash_1u64((u64)first, key); #endif
> +	return (unsigned long)siphash_1u32((u32)first, key); }
> +
> +/**
>   * siphash_1u64 - compute 64-bit siphash PRF value of a u64
>   * @first: first u64
>   * @key: the siphash key
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 86c3385b9eb3..afd1c835b0f6 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -33,6 +33,7 @@
>  #include <linux/uuid.h>
>  #include <linux/of.h>
>  #include <net/addrconf.h>
> +#include <linux/siphash.h>
>  #ifdef CONFIG_BLOCK
>  #include <linux/blkdev.h>
>  #endif
> @@ -503,6 +504,7 @@ char *number(char *buf, char *end, unsigned long long
> num,
>  			*buf = '0';
>  		++buf;
>  	}
> +

Unneeded whitespace change?

>  	/* actual digits of result */
>  	while (--i >= 0) {
>  		if (buf < end)
> @@ -1591,6 +1593,28 @@ char *device_node_string(char *buf, char *end, struct
> device_node *dn,
>  	return widen_string(buf, buf - buf_start, end, spec);  }
> 
> +/* Maps a pointer to a 32 bit unique identifier. */ static char
> +*ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec) {
> +	static siphash_key_t ptr_secret __read_mostly;
> +	static bool have_key = false;
> +	unsigned long hashval;
> +
> +	/* Kernel doesn't boot if we use get_random_once() */
> +	if (!have_key) {
> +		get_random_bytes(&ptr_secret, sizeof(ptr_secret));
> +		have_key = true;

Wouldn't one want to use an atomic test and swap for this
block?

> +	}
> +
> +	hashval = siphash_1ulong((unsigned long)ptr, &ptr_secret);
> +
> +	spec.field_width = 2 + 2 * sizeof(unsigned int); /* 0x + hex */
> +	spec.flags = SPECIAL | SMALL | ZEROPAD;
> +	spec.base = 16;
> +
> +	return number(buf, end, (u32)hashval, spec); }
> +
>  int kptr_restrict __read_mostly;
> 
>  /*
> @@ -1703,6 +1727,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.
> + *
> + * 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, @@ -1858,14
> +1885,13 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  			return device_node_string(buf, end, ptr, spec, fmt + 1);
>  		}
>  	}
> -	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);
> +	return ptr_to_id(buf, end, ptr, spec);
>  }
> 
>  /*
> --
> 2.7.4

Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 17, 2017 at 05:27:15PM +0000, Roberts, William C wrote:
> 
> 
> > -----Original Message-----
> > From: Tobin C. Harding [mailto:me@tobin.cc]
> > Sent: Monday, October 16, 2017 9:53 PM
> > To: kernel-hardening@lists.openwall.com
> > Cc: Tobin C. Harding <me@tobin.cc>; Linus Torvalds <torvalds@linux-
> > foundation.org>; Kees Cook <keescook@chromium.org>; Paolo Bonzini
> > <pbonzini@redhat.com>; Tycho Andersen <tycho@docker.com>; Roberts,
> > William C <william.c.roberts@intel.com>; Tejun Heo <tj@kernel.org>; Jordan
> > Glover <Golden_Miller83@protonmail.ch>; Greg KH
> > <gregkh@linuxfoundation.org>; Petr Mladek <pmladek@suse.com>; Joe
> > Perches <joe@perches.com>; Ian Campbell <ijc@hellion.org.uk>; Sergey
> > Senozhatsky <sergey.senozhatsky@gmail.com>; Catalin Marinas
> > <catalin.marinas@arm.com>; Will Deacon <will.deacon@arm.com>; Steven
> > Rostedt <rostedt@goodmis.org>; Chris Fries <cfries@google.com>; Dave
> > Weinstein <olorin@google.com>; Daniel Micay <danielmicay@gmail.com>; Djalal
> > Harouni <tixxdz@gmail.com>; linux-kernel@vger.kernel.org
> > Subject: [PATCH v2] printk: hash addresses printed with %p
> > 
> > Currently there are many places in the kernel where addresses are being printed
> > using an unadorned %p. Kernel pointers should be printed using %pK allowing
> > some control via the kptr_restrict sysctl. Exposing addresses gives attackers
> > sensitive information about the kernel layout in memory.
> > 
> > We can reduce the attack surface by hashing all addresses printed with %p. This
> > will of course break some users, forcing code printing needed addresses to be
> > updated.
> > 
> > For what it's worth, usage of unadorned %p can be broken down as follows
> > 
> >     git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> > 
> > arch: 2512
> > block: 20
> > crypto: 12
> > fs: 1221
> > include: 147
> > kernel: 109
> > lib: 77
> > mm: 120
> > net: 1516
> > security: 11
> > sound: 168
> > virt: 2
> > drivers: 8420
> > 
> > Add helper function siphash_1ulong(). Add function ptr_to_id() to map an
> > address to a 32 bit unique identifier.
> > 
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > ---
> > 
> > V2:
> >  - Use SipHash to do the hashing
> > 
> > The discussion related to this patch has been fragmented. There are three other
> > threads associated with this patch. Email threads by
> > subject:
> > 
> > [PATCH] printk: hash addresses printed with %p [PATCH 0/3] add %pX specifier
> > [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options
> > 
> >  include/linux/siphash.h |  2 ++
> >  lib/siphash.c           | 13 +++++++++++++
> >  lib/vsprintf.c          | 32 +++++++++++++++++++++++++++++---
> >  3 files changed, 44 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/siphash.h b/include/linux/siphash.h index
> > fa7a6b9cedbf..a9392568c8b8 100644
> > --- a/include/linux/siphash.h
> > +++ b/include/linux/siphash.h
> > @@ -26,6 +26,8 @@ u64 __siphash_aligned(const void *data, size_t len, const
> > siphash_key_t *key);
> >  u64 __siphash_unaligned(const void *data, size_t len, const siphash_key_t
> > *key);  #endif
> > 
> > +unsigned long siphash_1ulong(const unsigned long a, const siphash_key_t
> > +*key);
> > +
> >  u64 siphash_1u64(const u64 a, const siphash_key_t *key);
> >  u64 siphash_2u64(const u64 a, const u64 b, const siphash_key_t *key);
> >  u64 siphash_3u64(const u64 a, const u64 b, const u64 c, diff --git a/lib/siphash.c
> > b/lib/siphash.c index 3ae58b4edad6..63f4ff57c9ce 100644
> > --- a/lib/siphash.c
> > +++ b/lib/siphash.c
> > @@ -116,6 +116,19 @@ EXPORT_SYMBOL(__siphash_unaligned);
> >  #endif
> > 
> >  /**
> > + * siphash_1ulong - computes siphash PRF value
> > + * @first: value to hash
> > + * @key: the siphash key
> > + */
> > +unsigned long siphash_1ulong(const unsigned long first, const
> > +siphash_key_t *key) { #ifdef CONFIG_64BIT
> > +	return (unsigned long)siphash_1u64((u64)first, key); #endif
> > +	return (unsigned long)siphash_1u32((u32)first, key); }
> > +
> > +/**
> >   * siphash_1u64 - compute 64-bit siphash PRF value of a u64
> >   * @first: first u64
> >   * @key: the siphash key
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 86c3385b9eb3..afd1c835b0f6 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -33,6 +33,7 @@
> >  #include <linux/uuid.h>
> >  #include <linux/of.h>
> >  #include <net/addrconf.h>
> > +#include <linux/siphash.h>
> >  #ifdef CONFIG_BLOCK
> >  #include <linux/blkdev.h>
> >  #endif
> > @@ -503,6 +504,7 @@ char *number(char *buf, char *end, unsigned long long
> > num,
> >  			*buf = '0';
> >  		++buf;
> >  	}
> > +
> 
> Unneeded whitespace change?

:) thanks

> 
> >  	/* actual digits of result */
> >  	while (--i >= 0) {
> >  		if (buf < end)
> > @@ -1591,6 +1593,28 @@ char *device_node_string(char *buf, char *end, struct
> > device_node *dn,
> >  	return widen_string(buf, buf - buf_start, end, spec);  }
> > 
> > +/* Maps a pointer to a 32 bit unique identifier. */ static char
> > +*ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec) {
> > +	static siphash_key_t ptr_secret __read_mostly;
> > +	static bool have_key = false;
> > +	unsigned long hashval;
> > +
> > +	/* Kernel doesn't boot if we use get_random_once() */
> > +	if (!have_key) {
> > +		get_random_bytes(&ptr_secret, sizeof(ptr_secret));
> > +		have_key = true;
> 
> Wouldn't one want to use an atomic test and swap for this
> block?

Great, thanks for the pointer.

Thanks for the review William.

Tobin.

[kernel-hardening] Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 17, 2017 at 05:27:15PM +0000, Roberts, William C wrote:
> 
> 
> > -----Original Message-----
> > From: Tobin C. Harding [mailto:me@tobin.cc]
> > Sent: Monday, October 16, 2017 9:53 PM
> > To: kernel-hardening@lists.openwall.com
> > Cc: Tobin C. Harding <me@tobin.cc>; Linus Torvalds <torvalds@linux-
> > foundation.org>; Kees Cook <keescook@chromium.org>; Paolo Bonzini
> > <pbonzini@redhat.com>; Tycho Andersen <tycho@docker.com>; Roberts,
> > William C <william.c.roberts@intel.com>; Tejun Heo <tj@kernel.org>; Jordan
> > Glover <Golden_Miller83@protonmail.ch>; Greg KH
> > <gregkh@linuxfoundation.org>; Petr Mladek <pmladek@suse.com>; Joe
> > Perches <joe@perches.com>; Ian Campbell <ijc@hellion.org.uk>; Sergey
> > Senozhatsky <sergey.senozhatsky@gmail.com>; Catalin Marinas
> > <catalin.marinas@arm.com>; Will Deacon <will.deacon@arm.com>; Steven
> > Rostedt <rostedt@goodmis.org>; Chris Fries <cfries@google.com>; Dave
> > Weinstein <olorin@google.com>; Daniel Micay <danielmicay@gmail.com>; Djalal
> > Harouni <tixxdz@gmail.com>; linux-kernel@vger.kernel.org
> > Subject: [PATCH v2] printk: hash addresses printed with %p
> > 
> > Currently there are many places in the kernel where addresses are being printed
> > using an unadorned %p. Kernel pointers should be printed using %pK allowing
> > some control via the kptr_restrict sysctl. Exposing addresses gives attackers
> > sensitive information about the kernel layout in memory.
> > 
> > We can reduce the attack surface by hashing all addresses printed with %p. This
> > will of course break some users, forcing code printing needed addresses to be
> > updated.
> > 
> > For what it's worth, usage of unadorned %p can be broken down as follows
> > 
> >     git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> > 
> > arch: 2512
> > block: 20
> > crypto: 12
> > fs: 1221
> > include: 147
> > kernel: 109
> > lib: 77
> > mm: 120
> > net: 1516
> > security: 11
> > sound: 168
> > virt: 2
> > drivers: 8420
> > 
> > Add helper function siphash_1ulong(). Add function ptr_to_id() to map an
> > address to a 32 bit unique identifier.
> > 
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > ---
> > 
> > V2:
> >  - Use SipHash to do the hashing
> > 
> > The discussion related to this patch has been fragmented. There are three other
> > threads associated with this patch. Email threads by
> > subject:
> > 
> > [PATCH] printk: hash addresses printed with %p [PATCH 0/3] add %pX specifier
> > [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options
> > 
> >  include/linux/siphash.h |  2 ++
> >  lib/siphash.c           | 13 +++++++++++++
> >  lib/vsprintf.c          | 32 +++++++++++++++++++++++++++++---
> >  3 files changed, 44 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/siphash.h b/include/linux/siphash.h index
> > fa7a6b9cedbf..a9392568c8b8 100644
> > --- a/include/linux/siphash.h
> > +++ b/include/linux/siphash.h
> > @@ -26,6 +26,8 @@ u64 __siphash_aligned(const void *data, size_t len, const
> > siphash_key_t *key);
> >  u64 __siphash_unaligned(const void *data, size_t len, const siphash_key_t
> > *key);  #endif
> > 
> > +unsigned long siphash_1ulong(const unsigned long a, const siphash_key_t
> > +*key);
> > +
> >  u64 siphash_1u64(const u64 a, const siphash_key_t *key);
> >  u64 siphash_2u64(const u64 a, const u64 b, const siphash_key_t *key);
> >  u64 siphash_3u64(const u64 a, const u64 b, const u64 c, diff --git a/lib/siphash.c
> > b/lib/siphash.c index 3ae58b4edad6..63f4ff57c9ce 100644
> > --- a/lib/siphash.c
> > +++ b/lib/siphash.c
> > @@ -116,6 +116,19 @@ EXPORT_SYMBOL(__siphash_unaligned);
> >  #endif
> > 
> >  /**
> > + * siphash_1ulong - computes siphash PRF value
> > + * @first: value to hash
> > + * @key: the siphash key
> > + */
> > +unsigned long siphash_1ulong(const unsigned long first, const
> > +siphash_key_t *key) { #ifdef CONFIG_64BIT
> > +	return (unsigned long)siphash_1u64((u64)first, key); #endif
> > +	return (unsigned long)siphash_1u32((u32)first, key); }
> > +
> > +/**
> >   * siphash_1u64 - compute 64-bit siphash PRF value of a u64
> >   * @first: first u64
> >   * @key: the siphash key
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 86c3385b9eb3..afd1c835b0f6 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -33,6 +33,7 @@
> >  #include <linux/uuid.h>
> >  #include <linux/of.h>
> >  #include <net/addrconf.h>
> > +#include <linux/siphash.h>
> >  #ifdef CONFIG_BLOCK
> >  #include <linux/blkdev.h>
> >  #endif
> > @@ -503,6 +504,7 @@ char *number(char *buf, char *end, unsigned long long
> > num,
> >  			*buf = '0';
> >  		++buf;
> >  	}
> > +
> 
> Unneeded whitespace change?

:) thanks

> 
> >  	/* actual digits of result */
> >  	while (--i >= 0) {
> >  		if (buf < end)
> > @@ -1591,6 +1593,28 @@ char *device_node_string(char *buf, char *end, struct
> > device_node *dn,
> >  	return widen_string(buf, buf - buf_start, end, spec);  }
> > 
> > +/* Maps a pointer to a 32 bit unique identifier. */ static char
> > +*ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec) {
> > +	static siphash_key_t ptr_secret __read_mostly;
> > +	static bool have_key = false;
> > +	unsigned long hashval;
> > +
> > +	/* Kernel doesn't boot if we use get_random_once() */
> > +	if (!have_key) {
> > +		get_random_bytes(&ptr_secret, sizeof(ptr_secret));
> > +		have_key = true;
> 
> Wouldn't one want to use an atomic test and swap for this
> block?

Great, thanks for the pointer.

Thanks for the review William.

Tobin.

Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 17, 2017 at 09:31:19AM -0400, Steven Rostedt wrote:
> On Tue, 17 Oct 2017 15:52:51 +1100
> "Tobin C. Harding" <me@tobin.cc> wrote:
> 
> > Currently there are many places in the kernel where addresses are being
> > printed using an unadorned %p. Kernel pointers should be printed using
> > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> > gives attackers sensitive information about the kernel layout in memory.
> > 
> > We can reduce the attack surface by hashing all addresses printed with
> > %p. This will of course break some users, forcing code printing needed
> > addresses to be updated.
> > 
> > For what it's worth, usage of unadorned %p can be broken down as follows
> > 
> >     git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> 
> Does %p[FfSs] leak addresses? Well, I guess it does if they are not
> found in kallsyms, but otherwise you have:
> 
>   function+0x<offset>

You are correct %pF and %pS print an offset. Does this provide an attack vector,
I didn't think so but I'm no security expert. If they do then we need to amend
those calls also.

We still have %pa[pd] to see to as well obviously.

thanks for the review,
Tobin.

[kernel-hardening] Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 17, 2017 at 09:31:19AM -0400, Steven Rostedt wrote:
> On Tue, 17 Oct 2017 15:52:51 +1100
> "Tobin C. Harding" <me@tobin.cc> wrote:
> 
> > Currently there are many places in the kernel where addresses are being
> > printed using an unadorned %p. Kernel pointers should be printed using
> > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> > gives attackers sensitive information about the kernel layout in memory.
> > 
> > We can reduce the attack surface by hashing all addresses printed with
> > %p. This will of course break some users, forcing code printing needed
> > addresses to be updated.
> > 
> > For what it's worth, usage of unadorned %p can be broken down as follows
> > 
> >     git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> 
> Does %p[FfSs] leak addresses? Well, I guess it does if they are not
> found in kallsyms, but otherwise you have:
> 
>   function+0x<offset>

You are correct %pF and %pS print an offset. Does this provide an attack vector,
I didn't think so but I'm no security expert. If they do then we need to amend
those calls also.

We still have %pa[pd] to see to as well obviously.

thanks for the review,
Tobin.

Re: [PATCH v2] printk: hash addresses printed with %p

[Kees Cook]

On Tue, Oct 17, 2017 at 4:15 PM, Tobin C. Harding <me@tobin.cc> wrote:
> On Tue, Oct 17, 2017 at 09:31:19AM -0400, Steven Rostedt wrote:
>> On Tue, 17 Oct 2017 15:52:51 +1100
>> "Tobin C. Harding" <me@tobin.cc> wrote:
>>
>> > Currently there are many places in the kernel where addresses are being
>> > printed using an unadorned %p. Kernel pointers should be printed using
>> > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
>> > gives attackers sensitive information about the kernel layout in memory.
>> >
>> > We can reduce the attack surface by hashing all addresses printed with
>> > %p. This will of course break some users, forcing code printing needed
>> > addresses to be updated.
>> >
>> > For what it's worth, usage of unadorned %p can be broken down as follows
>> >
>> >     git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
>>
>> Does %p[FfSs] leak addresses? Well, I guess it does if they are not
>> found in kallsyms, but otherwise you have:
>>
>>   function+0x<offset>
>
> You are correct %pF and %pS print an offset. Does this provide an attack vector,
> I didn't think so but I'm no security expert. If they do then we need to amend
> those calls also.

They haven't traditionally been a big deal. If they turn out to be
problematic, we can deal with it then, IMO.

-Kees

-- 
Kees Cook
Pixel Security

[kernel-hardening] Re: [PATCH v2] printk: hash addresses printed with %p

[Kees Cook]

On Tue, Oct 17, 2017 at 4:15 PM, Tobin C. Harding <me@tobin.cc> wrote:
> On Tue, Oct 17, 2017 at 09:31:19AM -0400, Steven Rostedt wrote:
>> On Tue, 17 Oct 2017 15:52:51 +1100
>> "Tobin C. Harding" <me@tobin.cc> wrote:
>>
>> > Currently there are many places in the kernel where addresses are being
>> > printed using an unadorned %p. Kernel pointers should be printed using
>> > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
>> > gives attackers sensitive information about the kernel layout in memory.
>> >
>> > We can reduce the attack surface by hashing all addresses printed with
>> > %p. This will of course break some users, forcing code printing needed
>> > addresses to be updated.
>> >
>> > For what it's worth, usage of unadorned %p can be broken down as follows
>> >
>> >     git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
>>
>> Does %p[FfSs] leak addresses? Well, I guess it does if they are not
>> found in kallsyms, but otherwise you have:
>>
>>   function+0x<offset>
>
> You are correct %pF and %pS print an offset. Does this provide an attack vector,
> I didn't think so but I'm no security expert. If they do then we need to amend
> those calls also.

They haven't traditionally been a big deal. If they turn out to be
problematic, we can deal with it then, IMO.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2] printk: hash addresses printed with %p

[Jason A. Donenfeld]

Hi Tobin,

Many comments in line below.

On Tue, Oct 17, 2017 at 6:52 AM, Tobin C. Harding <me@tobin.cc> wrote:
>
> diff --git a/include/linux/siphash.h b/include/linux/siphash.h
> index fa7a6b9cedbf..a9392568c8b8 100644
> --- a/include/linux/siphash.h
> +++ b/include/linux/siphash.h
> @@ -26,6 +26,8 @@ u64 __siphash_aligned(const void *data, size_t len, const siphash_key_t *key);
>  u64 __siphash_unaligned(const void *data, size_t len, const siphash_key_t *key);
>  #endif
>
> +unsigned long siphash_1ulong(const unsigned long a, const siphash_key_t *key);

This signature is incorrect, as siphash always returns a u64. The
caller should do the casting, not the actual function itself.
[However, see below. I don't think you should be touching this file.]

>  u64 siphash_1u64(const u64 a, const siphash_key_t *key);
>  u64 siphash_2u64(const u64 a, const u64 b, const siphash_key_t *key);
>  u64 siphash_3u64(const u64 a, const u64 b, const u64 c,
> diff --git a/lib/siphash.c b/lib/siphash.c
> index 3ae58b4edad6..63f4ff57c9ce 100644
> --- a/lib/siphash.c
> +++ b/lib/siphash.c
> @@ -116,6 +116,19 @@ EXPORT_SYMBOL(__siphash_unaligned);
>  #endif
>
>  /**
> + * siphash_1ulong - computes siphash PRF value
> + * @first: value to hash
> + * @key: the siphash key
> + */

Please match the template usage text of every single other function, like so:

* siphash_1ulong - compute 64-bit siphash PRF value of 1 unsigned long
* @first: first unsigned long
* @key: the siphash key

[However, see below. I don't think you should be touching this file.]


> +unsigned long siphash_1ulong(const unsigned long first, const siphash_key_t *key)

Return u64. [However, see below. I don't think you should be touching
this file.]

> +{
> +#ifdef CONFIG_64BIT
> +       return (unsigned long)siphash_1u64((u64)first, key);

Don't cast it here. [However, see below. I don't think you should be
touching this file.]

> +#endif

There's no point in making gcc's life harder. Use an #else for the
32-bit section. [However, see below. I don't think you should be
touching this file.]


> +       return (unsigned long)siphash_1u32((u32)first, key);

Also don't cast. [However, see below. I don't think you should be
touching this file.]

> +/* Maps a pointer to a 32 bit unique identifier. */
> +static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
> +{
> +       static siphash_key_t ptr_secret __read_mostly;
> +       static bool have_key = false;
> +       unsigned long hashval;
> +
> +       /* Kernel doesn't boot if we use get_random_once() */
> +       if (!have_key) {
> +               get_random_bytes(&ptr_secret, sizeof(ptr_secret));
> +               have_key = true;
> +       }

This is wrong. You need to either use get_random_bytes_wait, which you
can't actually do safely here. So, better, use
add_random_ready_callback to get a notification of when this is safe
to use. Before it's safe to use, simply return "(ptr value)" or some
similar stub.

> +
> +       hashval = siphash_1ulong((unsigned long)ptr, &ptr_secret);

As mentioned above with the [brackets], don't pollute
siphash.h/siphash.c with the helper, and just put the #ifdef stuff
here. That should make it much more clear what's going on and also
make it easier in the future to swap out the 32-bit function when
we're ready.

So, this looks like instead:

#ifdef CONFIG_64BIT
hashval = (unsigned long)siphash_1u64((u64)ptr, key);
#else
hashval = (unsigned long)siphash_1u32((u32)ptr, key);
#endif

However, in another thread, Linus mentioned that he'd prefer all the
obfuscated values actually be 32-bit. So, this then looks like:

unsigned int hashval;
...
#ifdef CONFIG_64BIT
hashval = (unsigned int)siphash_1u64((u64)ptr, key);
#else
hashval = (unsigned int)siphash_1u32((u32)ptr, key);
#endif


Looking forward to v3!

Thanks,
Jason

[kernel-hardening] Re: [PATCH v2] printk: hash addresses printed with %p

[Jason A. Donenfeld]

Hi Tobin,

Many comments in line below.

On Tue, Oct 17, 2017 at 6:52 AM, Tobin C. Harding <me@tobin.cc> wrote:
>
> diff --git a/include/linux/siphash.h b/include/linux/siphash.h
> index fa7a6b9cedbf..a9392568c8b8 100644
> --- a/include/linux/siphash.h
> +++ b/include/linux/siphash.h
> @@ -26,6 +26,8 @@ u64 __siphash_aligned(const void *data, size_t len, const siphash_key_t *key);
>  u64 __siphash_unaligned(const void *data, size_t len, const siphash_key_t *key);
>  #endif
>
> +unsigned long siphash_1ulong(const unsigned long a, const siphash_key_t *key);

This signature is incorrect, as siphash always returns a u64. The
caller should do the casting, not the actual function itself.
[However, see below. I don't think you should be touching this file.]

>  u64 siphash_1u64(const u64 a, const siphash_key_t *key);
>  u64 siphash_2u64(const u64 a, const u64 b, const siphash_key_t *key);
>  u64 siphash_3u64(const u64 a, const u64 b, const u64 c,
> diff --git a/lib/siphash.c b/lib/siphash.c
> index 3ae58b4edad6..63f4ff57c9ce 100644
> --- a/lib/siphash.c
> +++ b/lib/siphash.c
> @@ -116,6 +116,19 @@ EXPORT_SYMBOL(__siphash_unaligned);
>  #endif
>
>  /**
> + * siphash_1ulong - computes siphash PRF value
> + * @first: value to hash
> + * @key: the siphash key
> + */

Please match the template usage text of every single other function, like so:

* siphash_1ulong - compute 64-bit siphash PRF value of 1 unsigned long
* @first: first unsigned long
* @key: the siphash key

[However, see below. I don't think you should be touching this file.]


> +unsigned long siphash_1ulong(const unsigned long first, const siphash_key_t *key)

Return u64. [However, see below. I don't think you should be touching
this file.]

> +{
> +#ifdef CONFIG_64BIT
> +       return (unsigned long)siphash_1u64((u64)first, key);

Don't cast it here. [However, see below. I don't think you should be
touching this file.]

> +#endif

There's no point in making gcc's life harder. Use an #else for the
32-bit section. [However, see below. I don't think you should be
touching this file.]


> +       return (unsigned long)siphash_1u32((u32)first, key);

Also don't cast. [However, see below. I don't think you should be
touching this file.]

> +/* Maps a pointer to a 32 bit unique identifier. */
> +static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
> +{
> +       static siphash_key_t ptr_secret __read_mostly;
> +       static bool have_key = false;
> +       unsigned long hashval;
> +
> +       /* Kernel doesn't boot if we use get_random_once() */
> +       if (!have_key) {
> +               get_random_bytes(&ptr_secret, sizeof(ptr_secret));
> +               have_key = true;
> +       }

This is wrong. You need to either use get_random_bytes_wait, which you
can't actually do safely here. So, better, use
add_random_ready_callback to get a notification of when this is safe
to use. Before it's safe to use, simply return "(ptr value)" or some
similar stub.

> +
> +       hashval = siphash_1ulong((unsigned long)ptr, &ptr_secret);

As mentioned above with the [brackets], don't pollute
siphash.h/siphash.c with the helper, and just put the #ifdef stuff
here. That should make it much more clear what's going on and also
make it easier in the future to swap out the 32-bit function when
we're ready.

So, this looks like instead:

#ifdef CONFIG_64BIT
hashval = (unsigned long)siphash_1u64((u64)ptr, key);
#else
hashval = (unsigned long)siphash_1u32((u32)ptr, key);
#endif

However, in another thread, Linus mentioned that he'd prefer all the
obfuscated values actually be 32-bit. So, this then looks like:

unsigned int hashval;
...
#ifdef CONFIG_64BIT
hashval = (unsigned int)siphash_1u64((u64)ptr, key);
#else
hashval = (unsigned int)siphash_1u32((u32)ptr, key);
#endif


Looking forward to v3!

Thanks,
Jason

Re: [PATCH v2] printk: hash addresses printed with %p

[Steven Rostedt]

On Wed, 18 Oct 2017 10:15:59 +1100
"Tobin C. Harding" <me@tobin.cc> wrote:

> > Does %p[FfSs] leak addresses? Well, I guess it does if they are not
> > found in kallsyms, but otherwise you have:
> > 
> >   function+0x<offset>  
> 
> You are correct %pF and %pS print an offset. Does this provide an attack vector,
> I didn't think so but I'm no security expert. If they do then we need to amend
> those calls also.

Hopefully not. We changed stack dumps to use them only instead of
showing addresses because of the location leak.

-- Steve

[kernel-hardening] Re: [PATCH v2] printk: hash addresses printed with %p

[Steven Rostedt]

On Wed, 18 Oct 2017 10:15:59 +1100
"Tobin C. Harding" <me@tobin.cc> wrote:

> > Does %p[FfSs] leak addresses? Well, I guess it does if they are not
> > found in kallsyms, but otherwise you have:
> > 
> >   function+0x<offset>  
> 
> You are correct %pF and %pS print an offset. Does this provide an attack vector,
> I didn't think so but I'm no security expert. If they do then we need to amend
> those calls also.

Hopefully not. We changed stack dumps to use them only instead of
showing addresses because of the location leak.

-- Steve

Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Wed, Oct 18, 2017 at 02:27:43AM +0200, Jason A. Donenfeld wrote:
[snip]

Thank you for your extensive comments Jason. I had v3 in flight before I received your email, please
don't think I ignored your suggestions.

v4 to come!

thanks,
Tobin.

[kernel-hardening] Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Wed, Oct 18, 2017 at 02:27:43AM +0200, Jason A. Donenfeld wrote:
[snip]

Thank you for your extensive comments Jason. I had v3 in flight before I received your email, please
don't think I ignored your suggestions.

v4 to come!

thanks,
Tobin.

Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 17, 2017 at 05:13:10PM -0700, Kees Cook wrote:
> On Tue, Oct 17, 2017 at 4:15 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > On Tue, Oct 17, 2017 at 09:31:19AM -0400, Steven Rostedt wrote:
> >> On Tue, 17 Oct 2017 15:52:51 +1100
> >> "Tobin C. Harding" <me@tobin.cc> wrote:
> >>
> >> > Currently there are many places in the kernel where addresses are being
> >> > printed using an unadorned %p. Kernel pointers should be printed using
> >> > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> >> > gives attackers sensitive information about the kernel layout in memory.
> >> >
> >> > We can reduce the attack surface by hashing all addresses printed with
> >> > %p. This will of course break some users, forcing code printing needed
> >> > addresses to be updated.
> >> >
> >> > For what it's worth, usage of unadorned %p can be broken down as follows
> >> >
> >> >     git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> >>
> >> Does %p[FfSs] leak addresses? Well, I guess it does if they are not
> >> found in kallsyms, but otherwise you have:
> >>
> >>   function+0x<offset>
> >
> > You are correct %pF and %pS print an offset. Does this provide an attack vector,
> > I didn't think so but I'm no security expert. If they do then we need to amend
> > those calls also.
> 
> They haven't traditionally been a big deal. If they turn out to be
> problematic, we can deal with it then, IMO.

Thanks Kees,
Tobin.

[kernel-hardening] Re: [PATCH v2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Tue, Oct 17, 2017 at 05:13:10PM -0700, Kees Cook wrote:
> On Tue, Oct 17, 2017 at 4:15 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > On Tue, Oct 17, 2017 at 09:31:19AM -0400, Steven Rostedt wrote:
> >> On Tue, 17 Oct 2017 15:52:51 +1100
> >> "Tobin C. Harding" <me@tobin.cc> wrote:
> >>
> >> > Currently there are many places in the kernel where addresses are being
> >> > printed using an unadorned %p. Kernel pointers should be printed using
> >> > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> >> > gives attackers sensitive information about the kernel layout in memory.
> >> >
> >> > We can reduce the attack surface by hashing all addresses printed with
> >> > %p. This will of course break some users, forcing code printing needed
> >> > addresses to be updated.
> >> >
> >> > For what it's worth, usage of unadorned %p can be broken down as follows
> >> >
> >> >     git grep '%p[^KFfSsBRrbMmIiEUVKNhdDgCGO]' | wc -l
> >>
> >> Does %p[FfSs] leak addresses? Well, I guess it does if they are not
> >> found in kallsyms, but otherwise you have:
> >>
> >>   function+0x<offset>
> >
> > You are correct %pF and %pS print an offset. Does this provide an attack vector,
> > I didn't think so but I'm no security expert. If they do then we need to amend
> > those calls also.
> 
> They haven't traditionally been a big deal. If they turn out to be
> problematic, we can deal with it then, IMO.

Thanks Kees,
Tobin.

Re: [kernel-hardening] Re: [PATCH v2] printk: hash addresses printed with %p

[Theodore Ts'o]

On Wed, Oct 18, 2017 at 01:28:05PM +1100, Tobin C. Harding wrote:
> > >> Does %p[FfSs] leak addresses? Well, I guess it does if they are not
> > >> found in kallsyms, but otherwise you have:
> > >>
> > >>   function+0x<offset>
> > >
> > 
> > They haven't traditionally been a big deal. If they turn out to be
> > problematic, we can deal with it then, IMO.

If it's not in kallsyms, the raw address is probably not going to be
terribly useful --- so even if it's not traditionally been a big deal,
why not just hash them if it's not going to be printed as "function+0x<offset>"?

If nothing else, it will help correlate the random address with other
places where it was printed via %p.

					- Ted

Re: [kernel-hardening] Re: [PATCH v2] printk: hash addresses printed with %p

[Theodore Ts'o]

On Wed, Oct 18, 2017 at 01:28:05PM +1100, Tobin C. Harding wrote:
> > >> Does %p[FfSs] leak addresses? Well, I guess it does if they are not
> > >> found in kallsyms, but otherwise you have:
> > >>
> > >>   function+0x<offset>
> > >
> > 
> > They haven't traditionally been a big deal. If they turn out to be
> > problematic, we can deal with it then, IMO.

If it's not in kallsyms, the raw address is probably not going to be
terribly useful --- so even if it's not traditionally been a big deal,
why not just hash them if it's not going to be printed as "function+0x<offset>"?

If nothing else, it will help correlate the random address with other
places where it was printed via %p.

					- Ted