[PATCH V8 0/2] printk: hash addresses printed with %p

[PATCH V8 0/2] 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.

With this version we include hashing of malformed specifiers also.
Malformed specifiers include incomplete (e.g %pi) and also non-existent
specifiers. checkpatch should warn for non-existent specifiers but
AFAICT won't warn for incomplete specifiers.

Here is the behaviour that this set implements.

For kpt_restrict==0

Randomness not ready:
  printed with %p: 		(pointer)          # NOTE: with padding
Valid pointer:
  printed with %pK: 		deadbeefdeadbeef
  printed with %p: 		0xdeadbeef
  malformed specifier (eg %i):  0xdeadbeef
NULL pointer:
  printed with %pK: 		0000000000000000
  printed with %p: 		(null)               # NOTE: no padding
  malformed specifier (eg %i):  (null)

For kpt_restrict==2

Valid pointer:
  printed with %pK: 		0000000000000000

All other output as for kptr_restrict==0

V8:
 - Add second patch cleaning up null pointer printing in pointer()
 - Move %pK handling to separate function, further cleaning up pointer()
 - Move ptr_to_id() call outside of switch statement making hashing
   the default behaviour (including malformed specifiers).
 - Remove use of static_key, replace with simple boolean.

V7:
 - Use tabs instead of spaces (ouch!).

V6:
 - Use __early_initcall() to fill the SipHash key.
 - Use static keys to guard hashing before the key is available.

V5:
 - Remove spin lock.
 - Add Jason A. Donenfeld to CC list by request.
 - Add Theodore Ts'o to CC list due to comment on previous version.

V4:
 - Remove changes to siphash.{ch}
 - Do word size check, and return value cast, directly in ptr_to_id().
 - Use add_ready_random_callback() to guard call to get_random_bytes()

V3:
 - Use atomic_xchg() to guard setting [random] key.
 - Remove erroneous white space change.

V2:
 - Use SipHash to do the hashing.

The discussion related to this patch has been fragmented. There are
three 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

Tobin C. Harding (2):
  printk: remove tabular output for NULL pointer
  printk: hash addresses printed with %p

 lib/vsprintf.c | 166 +++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 108 insertions(+), 58 deletions(-)

-- 
2.7.4

[PATCH V8 1/2] printk: remove tabular output for NULL pointer

[Tobin C. Harding]

Currently pointer() checks for a NULL pointer argument and then if so
attempts to print "(null)" with _some_ standard width. This width cannot
correctly be ascertained here because many of the printk specifiers
print pointers of varying widths.

Remove the attempt to print NULL pointers with a correct width.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 lib/vsprintf.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86c3385b9eb3..16a587aed40e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1710,15 +1710,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 {
 	const int default_width = 2 * sizeof(void *);
 
-	if (!ptr && *fmt != 'K') {
-		/*
-		 * Print (null) with the same width as a pointer so it makes
-		 * tabular output look nice.
-		 */
-		if (spec.field_width == -1)
-			spec.field_width = default_width;
+	if (!ptr && *fmt != 'K')
 		return string(buf, end, "(null)", spec);
-	}
 
 	switch (*fmt) {
 	case 'F':
-- 
2.7.4

[PATCH V8 2/2] 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 (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.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 lib/vsprintf.c | 157 +++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 107 insertions(+), 50 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 16a587aed40e..8f4aebd10c7e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -33,6 +33,8 @@
 #include <linux/uuid.h>
 #include <linux/of.h>
 #include <net/addrconf.h>
+#include <linux/siphash.h>
+#include <linux/compiler.h>
 #ifdef CONFIG_BLOCK
 #include <linux/blkdev.h>
 #endif
@@ -1344,6 +1346,57 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 }
 
 static noinline_for_stack
+char *kernel_pointer(char *buf, char *end, const void *ptr,
+		     struct printf_spec spec)
+{
+	spec.base = 16;
+	spec.flags |= SMALL;
+	if (spec.field_width == -1) {
+		spec.field_width = 2 * sizeof(void *);
+		spec.flags |= ZEROPAD;
+	}
+
+	switch (kptr_restrict) {
+	case 0:
+		/* Always print %pK values */
+		break;
+	case 1: {
+		const struct cred *cred;
+
+		/*
+		 * kptr_restrict==1 cannot be used in IRQ context
+		 * because its test for CAP_SYSLOG would be meaningless.
+		 */
+		if (in_irq() || in_serving_softirq() || in_nmi())
+			return string(buf, end, "pK-error", spec);
+
+		/*
+		 * Only print the real pointer value if the current
+		 * process has CAP_SYSLOG and is running with the
+		 * same credentials it started with. This is because
+		 * access to files is checked at open() time, but %pK
+		 * checks permission at read() time. We don't want to
+		 * leak pointer values if a binary opens a file using
+		 * %pK and then elevates privileges before reading it.
+		 */
+		cred = current_cred();
+		if (!has_capability_noaudit(current, CAP_SYSLOG) ||
+		    !uid_eq(cred->euid, cred->uid) ||
+		    !gid_eq(cred->egid, cred->gid))
+			ptr = NULL;
+		break;
+	}
+	case 2:
+	default:
+		/* Always print 0's for %pK */
+		ptr = NULL;
+		break;
+	}
+
+	return number(buf, end, (unsigned long)ptr, spec);
+}
+
+static noinline_for_stack
 char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt)
 {
 	unsigned long long num;
@@ -1591,6 +1644,54 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
+static bool have_filled_random_ptr_key;
+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));
+	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 int hashval;
+	int size = sizeof(hashval);
+
+	if (unlikely(!have_filled_random_ptr_key)) {
+		spec.field_width = 2 + 2 * size; /* 0x + hex */
+		return string(buf, end, "(pointer)", spec);
+	}
+
+#ifdef CONFIG_64BIT
+	hashval = (unsigned int)siphash_1u64((u64)ptr, &ptr_key);
+#else
+	hashval = (unsigned int)siphash_1u32((u32)ptr, &ptr_key);
+#endif
+
+	return special_hex_number(buf, end, hashval, size);
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1703,13 +1804,14 @@ 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,
 	      struct printf_spec spec)
 {
-	const int default_width = 2 * sizeof(void *);
-
 	if (!ptr && *fmt != 'K')
 		return string(buf, end, "(null)", spec);
 
@@ -1785,47 +1887,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			return buf;
 		}
 	case 'K':
-		switch (kptr_restrict) {
-		case 0:
-			/* Always print %pK values */
-			break;
-		case 1: {
-			const struct cred *cred;
-
-			/*
-			 * kptr_restrict==1 cannot be used in IRQ context
-			 * because its test for CAP_SYSLOG would be meaningless.
-			 */
-			if (in_irq() || in_serving_softirq() || in_nmi()) {
-				if (spec.field_width == -1)
-					spec.field_width = default_width;
-				return string(buf, end, "pK-error", spec);
-			}
-
-			/*
-			 * Only print the real pointer value if the current
-			 * process has CAP_SYSLOG and is running with the
-			 * same credentials it started with. This is because
-			 * access to files is checked at open() time, but %pK
-			 * checks permission at read() time. We don't want to
-			 * leak pointer values if a binary opens a file using
-			 * %pK and then elevates privileges before reading it.
-			 */
-			cred = current_cred();
-			if (!has_capability_noaudit(current, CAP_SYSLOG) ||
-			    !uid_eq(cred->euid, cred->uid) ||
-			    !gid_eq(cred->egid, cred->gid))
-				ptr = NULL;
-			break;
-		}
-		case 2:
-		default:
-			/* Always print 0's for %pK */
-			ptr = NULL;
-			break;
-		}
-		break;
-
+		return kernel_pointer(buf, end, ptr, spec);
 	case 'N':
 		return netdev_bits(buf, end, ptr, fmt);
 	case 'a':
@@ -1851,14 +1913,9 @@ 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);
+	/* default is to _not_ leak addresses, hash before printing */
+	return ptr_to_id(buf, end, ptr, spec);
 }
 
 /*
-- 
2.7.4

Re: [PATCH V8 2/2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Thu, Oct 26, 2017 at 01:53:56PM +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 (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.
> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> ---
>  lib/vsprintf.c | 157 +++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 107 insertions(+), 50 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 16a587aed40e..8f4aebd10c7e 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -33,6 +33,8 @@
>  #include <linux/uuid.h>
>  #include <linux/of.h>
>  #include <net/addrconf.h>
> +#include <linux/siphash.h>
> +#include <linux/compiler.h>
>  #ifdef CONFIG_BLOCK
>  #include <linux/blkdev.h>
>  #endif
> @@ -1344,6 +1346,57 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
>  }
>  
>  static noinline_for_stack
> +char *kernel_pointer(char *buf, char *end, const void *ptr,
> +		     struct printf_spec spec)
> +{
> +	spec.base = 16;
> +	spec.flags |= SMALL;
> +	if (spec.field_width == -1) {
> +		spec.field_width = 2 * sizeof(void *);
> +		spec.flags |= ZEROPAD;
> +	}
> +
> +	switch (kptr_restrict) {
> +	case 0:
> +		/* Always print %pK values */
> +		break;
> +	case 1: {
> +		const struct cred *cred;
> +
> +		/*
> +		 * kptr_restrict==1 cannot be used in IRQ context
> +		 * because its test for CAP_SYSLOG would be meaningless.
> +		 */
> +		if (in_irq() || in_serving_softirq() || in_nmi())
> +			return string(buf, end, "pK-error", spec);
> +
> +		/*
> +		 * Only print the real pointer value if the current
> +		 * process has CAP_SYSLOG and is running with the
> +		 * same credentials it started with. This is because
> +		 * access to files is checked at open() time, but %pK
> +		 * checks permission at read() time. We don't want to
> +		 * leak pointer values if a binary opens a file using
> +		 * %pK and then elevates privileges before reading it.
> +		 */
> +		cred = current_cred();
> +		if (!has_capability_noaudit(current, CAP_SYSLOG) ||
> +		    !uid_eq(cred->euid, cred->uid) ||
> +		    !gid_eq(cred->egid, cred->gid))
> +			ptr = NULL;
> +		break;
> +	}
> +	case 2:
> +	default:
> +		/* Always print 0's for %pK */
> +		ptr = NULL;
> +		break;
> +	}
> +
> +	return number(buf, end, (unsigned long)ptr, spec);
> +}
> +
> +static noinline_for_stack
>  char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt)
>  {
>  	unsigned long long num;
> @@ -1591,6 +1644,54 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
>  	return widen_string(buf, buf - buf_start, end, spec);
>  }
>  
> +static bool have_filled_random_ptr_key;
> +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));
> +	WRITE_ONCE(have_filled_random_ptr_key, true);

This usage of WRITE_ONCE was suggested by Jason A. Donenfeld. I read
include/linux/compiler.h but was not able to grok it. Is this enough to
stop the compiler re-ordering these two statements? 

Or do I need to read Documentation/memory-barriers.txt [again]?

thanks,
Tobin.

Re: [PATCH V8 2/2] printk: hash addresses printed with %p

[Jason A. Donenfeld]

On Thu, Oct 26, 2017 at 4:53 AM, Tobin C. Harding <me@tobin.cc> wrote:
> +static bool have_filled_random_ptr_key;

__read_mostly

Re: [PATCH V8 1/2] printk: remove tabular output for NULL pointer

[Joe Perches]

On Thu, 2017-10-26 at 13:53 +1100, Tobin C. Harding wrote:
> Currently pointer() checks for a NULL pointer argument and then if so
> attempts to print "(null)" with _some_ standard width. This width cannot
> correctly be ascertained here because many of the printk specifiers
> print pointers of varying widths.

I believe this is not a good change.
Only pointers without a <foo> extension call pointer()

> Remove the attempt to print NULL pointers with a correct width.

the correct width for a %p is the default width.
The correct width for %p<foo> is unknown.

> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> ---
>  lib/vsprintf.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 86c3385b9eb3..16a587aed40e 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1710,15 +1710,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  {
>  	const int default_width = 2 * sizeof(void *);
>  
> -	if (!ptr && *fmt != 'K') {
> -		/*
> -		 * Print (null) with the same width as a pointer so it makes
> -		 * tabular output look nice.
> -		 */
> -		if (spec.field_width == -1)
> -			spec.field_width = default_width;
> +	if (!ptr && *fmt != 'K')
>  		return string(buf, end, "(null)", spec);
> -	}
>  
>  	switch (*fmt) {
>  	case 'F':

Re: [PATCH V8 1/2] printk: remove tabular output for NULL pointer

[Tobin C. Harding]

Hi Joe,

thanks for your review.

On Wed, Oct 25, 2017 at 09:57:23PM -0700, Joe Perches wrote:
> On Thu, 2017-10-26 at 13:53 +1100, Tobin C. Harding wrote:
> > Currently pointer() checks for a NULL pointer argument and then if so
> > attempts to print "(null)" with _some_ standard width. This width cannot
> > correctly be ascertained here because many of the printk specifiers
> > print pointers of varying widths.
> 
> I believe this is not a good change.
> Only pointers without a <foo> extension call pointer()

Sorry, I don't understand what you mean here. All the %p<foo> specifier code is
handled by pointer()?

> > Remove the attempt to print NULL pointers with a correct width.
> 
> the correct width for a %p is the default width.

It is the default width if we are printing addresses. Once we hash 64
bit address to a 32 bit identifier then we don't have a default width.

> The correct width for %p<foo> is unknown.

I agree.

If I have misunderstood you, please forgive me. I am very appreciative
of the reviews this patch is getting and the patience the list is having
with the many iterations.

thanks,
Tobin.

Re: [PATCH V8 1/2] printk: remove tabular output for NULL pointer

[Joe Perches]

On Thu, 2017-10-26 at 17:27 +1100, Tobin C. Harding wrote:
> Hi Joe,
> 
> thanks for your review.
> 
> On Wed, Oct 25, 2017 at 09:57:23PM -0700, Joe Perches wrote:
> > On Thu, 2017-10-26 at 13:53 +1100, Tobin C. Harding wrote:
> > > Currently pointer() checks for a NULL pointer argument and then if so
> > > attempts to print "(null)" with _some_ standard width. This width cannot
> > > correctly be ascertained here because many of the printk specifiers
> > > print pointers of varying widths.
> > 
> > I believe this is not a good change.
> > Only pointers without a <foo> extension call pointer()
>
> Sorry, I don't understand what you mean here. All the %p<foo> specifier code is
> handled by pointer()?

Sorry, I was imprecise/wrong.

None of the %p<foo> extensions except %pK and %p<invalid_foo>
actually use this bit of the pointer() call.

All of the other valid %p<foo> extension uses do not end up
at this block being executed so it's effectively only regular
pointers being output by number()

> > > Remove the attempt to print NULL pointers with a correct width.
> > 
> > the correct width for a %p is the default width.
> 
> It is the default width if we are printing addresses. Once we hash 64
> bit address to a 32 bit identifier then we don't have a default width.

Perhaps that 32 bit identifier should use leading 0's for
the default width.

aside:

Why hash 64 bits to 32?
Why shouldn't the hash width be 64 bits on 64 bit systems?

Re: [PATCH V8 1/2] printk: remove tabular output for NULL pointer

[Tobin C. Harding]

On Thu, Oct 26, 2017 at 01:05:39AM -0700, Joe Perches wrote:
> On Thu, 2017-10-26 at 17:27 +1100, Tobin C. Harding wrote:
> > Hi Joe,
> > 
> > thanks for your review.
> > 
> > On Wed, Oct 25, 2017 at 09:57:23PM -0700, Joe Perches wrote:
> > > On Thu, 2017-10-26 at 13:53 +1100, Tobin C. Harding wrote:
> > > > Currently pointer() checks for a NULL pointer argument and then if so
> > > > attempts to print "(null)" with _some_ standard width. This width cannot
> > > > correctly be ascertained here because many of the printk specifiers
> > > > print pointers of varying widths.
> > > 
> > > I believe this is not a good change.
> > > Only pointers without a <foo> extension call pointer()
> >
> > Sorry, I don't understand what you mean here. All the %p<foo> specifier code is
> > handled by pointer()?
> 
> Sorry, I was imprecise/wrong.
> 
> None of the %p<foo> extensions except %pK and %p<invalid_foo>
> actually use this bit of the pointer() call.

	if (!ptr && *fmt != 'K') {
		/*
		 * Print (null) with the same width as a pointer so it makes
		 * tabular output look nice.
		 */
		if (spec.field_width == -1)
			spec.field_width = default_width;
		return string(buf, end, "(null)", spec);
	}

Is there something I'm missing here? This code reads like its all %p<foo>
(including %p and %p<invalid_foo>) except %pK that hit this block when
a NULL pointer is passed in.

> All of the other valid %p<foo> extension uses do not end up
> at this block being executed so it's effectively only regular
> pointers being output by number()
> 
> > > > Remove the attempt to print NULL pointers with a correct width.
> > > 
> > > the correct width for a %p is the default width.
> > 
> > It is the default width if we are printing addresses. Once we hash 64
> > bit address to a 32 bit identifier then we don't have a default width.
> 
> Perhaps that 32 bit identifier should use leading 0's for
> the default width.

That's a fair comment.

> aside:
> 
> Why hash 64 bits to 32?
> Why shouldn't the hash width be 64 bits on 64 bit systems?

Quoted from Linus in an earlier thread discussing this change

	Date: Thu, 12 Oct 2017 11:37:22 -0700 Linus Torvalds wrote:

	In fact, I'd prefer mapping the pointer to a 32-bit value, even on
	64-bit architectures. When people use these things for debugging and
	for identifying which device node or socket or whatever they are
	tracking, we're generally talking a (small) handful of different
	devices or whatever.


Hope this helps,
Tobin.

Re: [PATCH V8 1/2] printk: remove tabular output for NULL pointer

[Joe Perches]

On Thu, 2017-10-26 at 20:37 +1100, Tobin C. Harding wrote:
> On Thu, Oct 26, 2017 at 01:05:39AM -0700, Joe Perches wrote:
> > On Thu, 2017-10-26 at 17:27 +1100, Tobin C. Harding wrote:
> > > Hi Joe,
> > > 
> > > thanks for your review.
> > > 
> > > On Wed, Oct 25, 2017 at 09:57:23PM -0700, Joe Perches wrote:
> > > > On Thu, 2017-10-26 at 13:53 +1100, Tobin C. Harding wrote:
> > > > > Currently pointer() checks for a NULL pointer argument and then if so
> > > > > attempts to print "(null)" with _some_ standard width. This width cannot
> > > > > correctly be ascertained here because many of the printk specifiers
> > > > > print pointers of varying widths.
> > > > 
> > > > I believe this is not a good change.
> > > > Only pointers without a <foo> extension call pointer()
> > > 
> > > Sorry, I don't understand what you mean here. All the %p<foo> specifier code is
> > > handled by pointer()?
> > 
> > Sorry, I was imprecise/wrong.
> > 
> > None of the %p<foo> extensions except %pK and %p<invalid_foo>
> > actually use this bit of the pointer() call.
> 
> 	if (!ptr && *fmt != 'K') {
> 		/*
> 		 * Print (null) with the same width as a pointer so it makes
> 		 * tabular output look nice.
> 		 */
> 		if (spec.field_width == -1)
> 			spec.field_width = default_width;
> 		return string(buf, end, "(null)", spec);
> 	}
> 
> Is there something I'm missing here? This code reads like its all %p<foo>
> (including %p and %p<invalid_foo>) except %pK that hit this block when
> a NULL pointer is passed in.

The idea for aligning is described in commit 5e0579812834a

$ git log --stat -p -1 --format=email 5e0579812834a
>From 5e0579812834ab7fa072db4a15ebdff68d62e2e7 Mon Sep 17 00:00:00 2001
From: Joe Perches <joe@perches.com>
Date: Tue, 26 Oct 2010 14:22:50 -0700
Subject: [PATCH] vsprintf.c: use default pointer field size for "(null)"
 strings

It might be nicer to align the output.

For instance, ACPI messages sometimes have "(null)" pointers.

$ dmesg | grep "(null)"  -A 1 -B 1
[    0.198733] ACPI: Dynamic OEM Table Load:
[    0.198745] ACPI: SSDT (null) 00239 (v02  PmRef  Cpu0Ist 00003000 INTL 20051117)
[    0.199294] ACPI: SSDT 7f596e10 001C7 (v02  PmRef  Cpu0Cst 00003001 INTL 20051117)
[    0.200708] ACPI: Dynamic OEM Table Load:
[    0.200721] ACPI: SSDT (null) 001C7 (v02  PmRef  Cpu0Cst 00003001 INTL 20051117)
[    0.201950] ACPI: SSDT 7f597f10 000D0 (v02  PmRef  Cpu1Ist 00003000 INTL 20051117)
[    0.203386] ACPI: Dynamic OEM Table Load:
[    0.203398] ACPI: SSDT (null) 000D0 (v02  PmRef  Cpu1Ist 00003000 INTL 20051117)
[    0.203871] ACPI: SSDT 7f595f10 00083 (v02  PmRef  Cpu1Cst 00003000 INTL 20051117)
[    0.205301] ACPI: Dynamic OEM Table Load:
[    0.205315] ACPI: SSDT (null) 00083 (v02  PmRef  Cpu1Cst 00003000 INTL 20051117)

> > All of the other valid %p<foo> extension uses do not end up
> > at this block being executed so it's effectively only regular
> > pointers being output by number()

Because passing NULL to any of the %p<foo> extensions
excluding %pK is probably a defect.

> > > > > Remove the attempt to print NULL pointers with a correct width.
> > > > 
> > > > the correct width for a %p is the default width.
> > > 
> > > It is the default width if we are printing addresses. Once we hash 64
> > > bit address to a 32 bit identifier then we don't have a default width.
> > 
> > Perhaps that 32 bit identifier should use leading 0's for
> > the default width.
> 
> That's a fair comment.
> 
> > aside:
> > 
> > Why hash 64 bits to 32?
> > Why shouldn't the hash width be 64 bits on 64 bit systems?
> 
> Quoted from Linus in an earlier thread discussing this change
> 
> 	Date: Thu, 12 Oct 2017 11:37:22 -0700 Linus Torvalds wrote:
> 
> 	In fact, I'd prefer mapping the pointer to a 32-bit value, even on
> 	64-bit architectures. When people use these things for debugging and
> 	for identifying which device node or socket or whatever they are
> 	tracking, we're generally talking a (small) handful of different
> 	devices or whatever.

I wonder about this and userland programs and API breakage.

I'd expect there could be cases of userland parsers that
expect a certain width for pointer fields.

$ git grep -E "\bseq_.*%p\W" | wc -l
112

Re: [PATCH V8 1/2] printk: remove tabular output for NULL pointer

[Tobin C. Harding]

On Thu, Oct 26, 2017 at 07:47:19AM -0700, Joe Perches wrote:
> On Thu, 2017-10-26 at 20:37 +1100, Tobin C. Harding wrote:
> > On Thu, Oct 26, 2017 at 01:05:39AM -0700, Joe Perches wrote:
> > > On Thu, 2017-10-26 at 17:27 +1100, Tobin C. Harding wrote:
> > > > Hi Joe,
> > > > 
> > > > thanks for your review.
> > > > 
> > > > On Wed, Oct 25, 2017 at 09:57:23PM -0700, Joe Perches wrote:
> > > > > On Thu, 2017-10-26 at 13:53 +1100, Tobin C. Harding wrote:
> > > > > > Currently pointer() checks for a NULL pointer argument and then if so
> > > > > > attempts to print "(null)" with _some_ standard width. This width cannot
> > > > > > correctly be ascertained here because many of the printk specifiers
> > > > > > print pointers of varying widths.
> > > > > 
> > > > > I believe this is not a good change.
> > > > > Only pointers without a <foo> extension call pointer()
> > > > 
> > > > Sorry, I don't understand what you mean here. All the %p<foo> specifier code is
> > > > handled by pointer()?
> > > 
> > > Sorry, I was imprecise/wrong.
> > > 
> > > None of the %p<foo> extensions except %pK and %p<invalid_foo>
> > > actually use this bit of the pointer() call.
> > 
> > 	if (!ptr && *fmt != 'K') {
> > 		/*
> > 		 * Print (null) with the same width as a pointer so it makes
> > 		 * tabular output look nice.
> > 		 */
> > 		if (spec.field_width == -1)
> > 			spec.field_width = default_width;
> > 		return string(buf, end, "(null)", spec);
> > 	}
> > 
> > Is there something I'm missing here? This code reads like its all %p<foo>
> > (including %p and %p<invalid_foo>) except %pK that hit this block when
> > a NULL pointer is passed in.
> 
> The idea for aligning is described in commit 5e0579812834a
> 
> $ git log --stat -p -1 --format=email 5e0579812834a
> From 5e0579812834ab7fa072db4a15ebdff68d62e2e7 Mon Sep 17 00:00:00 2001
> From: Joe Perches <joe@perches.com>
> Date: Tue, 26 Oct 2010 14:22:50 -0700
> Subject: [PATCH] vsprintf.c: use default pointer field size for "(null)"
>  strings
> 
> It might be nicer to align the output.
> 
> For instance, ACPI messages sometimes have "(null)" pointers.
> 
> $ dmesg | grep "(null)"  -A 1 -B 1
> [    0.198733] ACPI: Dynamic OEM Table Load:
> [    0.198745] ACPI: SSDT (null) 00239 (v02  PmRef  Cpu0Ist 00003000 INTL 20051117)
> [    0.199294] ACPI: SSDT 7f596e10 001C7 (v02  PmRef  Cpu0Cst 00003001 INTL 20051117)
> [    0.200708] ACPI: Dynamic OEM Table Load:
> [    0.200721] ACPI: SSDT (null) 001C7 (v02  PmRef  Cpu0Cst 00003001 INTL 20051117)
> [    0.201950] ACPI: SSDT 7f597f10 000D0 (v02  PmRef  Cpu1Ist 00003000 INTL 20051117)
> [    0.203386] ACPI: Dynamic OEM Table Load:
> [    0.203398] ACPI: SSDT (null) 000D0 (v02  PmRef  Cpu1Ist 00003000 INTL 20051117)
> [    0.203871] ACPI: SSDT 7f595f10 00083 (v02  PmRef  Cpu1Cst 00003000 INTL 20051117)
> [    0.205301] ACPI: Dynamic OEM Table Load:
> [    0.205315] ACPI: SSDT (null) 00083 (v02  PmRef  Cpu1Cst 00003000 INTL 20051117)

Does this give the correct level of control? Would it not be better to
control the output of NULL pointers in the code that prints them. The
other side of the coin is that with padding a random single debug
message ends up with unwanted white space, e.g

 [    0.205315] foo: This pointer (null)         some useful error message 

Just thoughts.

> > > All of the other valid %p<foo> extension uses do not end up
> > > at this block being executed so it's effectively only regular
> > > pointers being output by number()
> 
> Because passing NULL to any of the %p<foo> extensions
> excluding %pK is probably a defect.

This implies that passing NULL to %p is a defect also, does it not. Like
already stated, if it is not a defect, the calling code is in a better
position to decide on the formatting, unless we printed
	<-    (null)    ->

with the correct spacing for all architectures, this seems a bit
extravagant though.

> > > > > > Remove the attempt to print NULL pointers with a correct width.
> > > > > 
> > > > > the correct width for a %p is the default width.
> > > > 
> > > > It is the default width if we are printing addresses. Once we hash 64
> > > > bit address to a 32 bit identifier then we don't have a default width.
> > > 
> > > Perhaps that 32 bit identifier should use leading 0's for
> > > the default width.
> > 
> > That's a fair comment.
> > 
> > > aside:
> > > 
> > > Why hash 64 bits to 32?
> > > Why shouldn't the hash width be 64 bits on 64 bit systems?
> > 
> > Quoted from Linus in an earlier thread discussing this change
> > 
> > 	Date: Thu, 12 Oct 2017 11:37:22 -0700 Linus Torvalds wrote:
> > 
> > 	In fact, I'd prefer mapping the pointer to a 32-bit value, even on
> > 	64-bit architectures. When people use these things for debugging and
> > 	for identifying which device node or socket or whatever they are
> > 	tracking, we're generally talking a (small) handful of different
> > 	devices or whatever.
> 
> I wonder about this and userland programs and API breakage.
> 
> I'd expect there could be cases of userland parsers that
> expect a certain width for pointer fields.
> 
> $ git grep -E "\bseq_.*%p\W" | wc -l
> 112

This is a good point. Making %p now prefix with 0x could also
potentially break things for the same reason. Perhaps your suggestion of
having leading 0's in front of the 32 bit identifier on 64 bit machines
solves a number of these problems (without the 0x prefix).

1. It leaves the output layout unchanged, no userland breakages.
2. It still has the advantages of a 32 bit hash mentioned by Linus.
3. It makes explicit that something funny is going on with the address,
   multiple addresses with 32 leading 0's will stand out.


thanks,
Tobin.

Re: [PATCH V8 1/2] printk: remove tabular output for NULL pointer

[Joe Perches]

On Fri, 2017-10-27 at 10:57 +1100, Tobin C. Harding wrote:
> On Thu, Oct 26, 2017 at 07:47:19AM -0700, Joe Perches wrote:
> > On Thu, 2017-10-26 at 20:37 +1100, Tobin C. Harding wrote:
> > > On Thu, Oct 26, 2017 at 01:05:39AM -0700, Joe Perches wrote:
> > > > On Thu, 2017-10-26 at 17:27 +1100, Tobin C. Harding wrote:
> > > > > Hi Joe,
> > > > > 
> > > > > thanks for your review.
> > > > > 
> > > > > On Wed, Oct 25, 2017 at 09:57:23PM -0700, Joe Perches wrote:
> > > > > > On Thu, 2017-10-26 at 13:53 +1100, Tobin C. Harding wrote:
> > > > > > > Currently pointer() checks for a NULL pointer argument and then if so
> > > > > > > attempts to print "(null)" with _some_ standard width. This width cannot
> > > > > > > correctly be ascertained here because many of the printk specifiers
> > > > > > > print pointers of varying widths.
> > > > > > 
> > > > > > I believe this is not a good change.
> > > > > > Only pointers without a <foo> extension call pointer()
> > > > > 
> > > > > Sorry, I don't understand what you mean here. All the %p<foo> specifier code is
> > > > > handled by pointer()?
> > > > 
> > > > Sorry, I was imprecise/wrong.
> > > > 
> > > > None of the %p<foo> extensions except %pK and %p<invalid_foo>
> > > > actually use this bit of the pointer() call.
> > > 
> > > 	if (!ptr && *fmt != 'K') {
> > > 		/*
> > > 		 * Print (null) with the same width as a pointer so it makes
> > > 		 * tabular output look nice.
> > > 		 */
> > > 		if (spec.field_width == -1)
> > > 			spec.field_width = default_width;
> > > 		return string(buf, end, "(null)", spec);
> > > 	}
> > > 
> > > Is there something I'm missing here? This code reads like its all %p<foo>
> > > (including %p and %p<invalid_foo>) except %pK that hit this block when
> > > a NULL pointer is passed in.
> > 
> > The idea for aligning is described in commit 5e0579812834a
> > 
> > $ git log --stat -p -1 --format=email 5e0579812834a
> > From 5e0579812834ab7fa072db4a15ebdff68d62e2e7 Mon Sep 17 00:00:00 2001
> > From: Joe Perches <joe@perches.com>
> > Date: Tue, 26 Oct 2010 14:22:50 -0700
> > Subject: [PATCH] vsprintf.c: use default pointer field size for "(null)"
> >  strings
> > 
> > It might be nicer to align the output.
> > 
> > For instance, ACPI messages sometimes have "(null)" pointers.
> > 
> > $ dmesg | grep "(null)"  -A 1 -B 1
> > [    0.198733] ACPI: Dynamic OEM Table Load:
> > [    0.198745] ACPI: SSDT (null) 00239 (v02  PmRef  Cpu0Ist 00003000 INTL 20051117)
> > [    0.199294] ACPI: SSDT 7f596e10 001C7 (v02  PmRef  Cpu0Cst 00003001 INTL 20051117)
> > [    0.200708] ACPI: Dynamic OEM Table Load:
> > [    0.200721] ACPI: SSDT (null) 001C7 (v02  PmRef  Cpu0Cst 00003001 INTL 20051117)
> > [    0.201950] ACPI: SSDT 7f597f10 000D0 (v02  PmRef  Cpu1Ist 00003000 INTL 20051117)
> > [    0.203386] ACPI: Dynamic OEM Table Load:
> > [    0.203398] ACPI: SSDT (null) 000D0 (v02  PmRef  Cpu1Ist 00003000 INTL 20051117)
> > [    0.203871] ACPI: SSDT 7f595f10 00083 (v02  PmRef  Cpu1Cst 00003000 INTL 20051117)
> > [    0.205301] ACPI: Dynamic OEM Table Load:
> > [    0.205315] ACPI: SSDT (null) 00083 (v02  PmRef  Cpu1Cst 00003000 INTL 20051117)
> 
> Does this give the correct level of control? Would it not be better to
> control the output of NULL pointers in the code that prints them. The
> other side of the coin is that with padding a random single debug
> message ends up with unwanted white space, e.g
> 
>  [    0.205315] foo: This pointer (null)         some useful error message 
> 
> Just thoughts.

I'm not sure there are any of those uses.
Perhaps you could show actual examples.

> > > > All of the other valid %p<foo> extension uses do not end up
> > > > at this block being executed so it's effectively only regular
> > > > pointers being output by number()
> > 
> > Because passing NULL to any of the %p<foo> extensions
> > excluding %pK is probably a defect.
> 
> This implies that passing NULL to %p is a defect also, does it not.

No, it does not imply that.

%p and %pK just print the value of the pointer arg.
%p<foo> extensions other than %pK dereference the pointer arg.
NULL
dereferences cause an oops.

> > I'd expect there could be cases of userland parsers that
> > expect a certain width for pointer fields.
> > 
> > $ git grep -E "\bseq_.*%p\W" | wc -l
> > 112
> 
> This is a good point. Making %p now prefix with 0x could also
> potentially break things for the same reason.

I thought it was agreed not to do that.

> Perhaps your suggestion of
> having leading 0's in front of the 32 bit identifier on 64 bit machines
> solves a number of these problems (without the 0x prefix).
> 
> 1. It leaves the output layout unchanged, no userland breakages.
> 2. It still has the advantages of a 32 bit hash mentioned by Linus.
> 3. It makes explicit that something funny is going on with the address,
>    multiple addresses with 32 leading 0's will stand out.

cheers, Joe

Re: [PATCH V8 0/2] printk: hash addresses printed with %p

[Sergey Senozhatsky]

On (10/26/17 13:53), 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.
> 
> With this version we include hashing of malformed specifiers also.
> Malformed specifiers include incomplete (e.g %pi) and also non-existent
> specifiers. checkpatch should warn for non-existent specifiers but
> AFAICT won't warn for incomplete specifiers.
> 
> Here is the behaviour that this set implements.
> 
> For kpt_restrict==0
> 
> Randomness not ready:
>   printed with %p: 		(pointer)          # NOTE: with padding
> Valid pointer:
>   printed with %pK: 		deadbeefdeadbeef
>   printed with %p: 		0xdeadbeef
>   malformed specifier (eg %i):  0xdeadbeef
> NULL pointer:
>   printed with %pK: 		0000000000000000
>   printed with %p: 		(null)               # NOTE: no padding
>   malformed specifier (eg %i):  (null)

a quick question:
 do we care about cases when kernel pointers are printed with %x/%X and
 not with %p?

	-ss

Re: [PATCH V8 2/2] printk: hash addresses printed with %p

[Steven Rostedt]

On Thu, 26 Oct 2017 13:58:38 +1100
"Tobin C. Harding" <me@tobin.cc> wrote:

> > +static bool have_filled_random_ptr_key;
> > +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));
> > +	WRITE_ONCE(have_filled_random_ptr_key, true);  
> 
> This usage of WRITE_ONCE was suggested by Jason A. Donenfeld. I read
> include/linux/compiler.h but was not able to grok it. Is this enough to
> stop the compiler re-ordering these two statements? 
> 
> Or do I need to read Documentation/memory-barriers.txt [again]?

No, the WRITE_ONCE does not stop the compiler from reordering those
statements. If you need that, then you need to do:

	get_random_bytes(&ptr_key, sizeof(ptr_key));
	barrier();
	WRITE_ONCE(have_filled_random_ptr_key, true);

and that only works against interrupts. If you need synchronization
across CPUs, then you need smp_mb().

-- Steve

Re: [PATCH V8 0/2] printk: hash addresses printed with %p

[Kees Cook]

On Wed, Oct 25, 2017 at 7:53 PM, Tobin C. Harding <me@tobin.cc> wrote:
> Here is the behaviour that this set implements.
>
> For kpt_restrict==0
>
> Randomness not ready:
>   printed with %p:              (pointer)          # NOTE: with padding
> Valid pointer:
>   printed with %pK:             deadbeefdeadbeef
>   printed with %p:              0xdeadbeef
>   malformed specifier (eg %i):  0xdeadbeef

I really think we can't include SPECIAL unless _every_ callsite of %p
is actually doing "0x%p", and then we're replacing all of those. We're
not doing that, though...

$ git grep '%p\b' | wc -l
12766
$ git grep '0x%p\b' | wc -l
1837

If we need some kind of special marking that this is a hashed
variable, that should be something other than "0x". If we're using the
existing "(null)" and new "(pointer)" text, maybe "(hash:xxxxxx)"
should be used instead? Then the (rare) callers with 0x become
"0x(hash:xxxx)" and naked callers produce "(hash:xxxx)".

I think the first step for this is to just leave SPECIAL out.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH V8 0/2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Mon, Oct 30, 2017 at 03:03:21PM -0700, Kees Cook wrote:
> On Wed, Oct 25, 2017 at 7:53 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > Here is the behaviour that this set implements.
> >
> > For kpt_restrict==0
> >
> > Randomness not ready:
> >   printed with %p:              (pointer)          # NOTE: with padding
> > Valid pointer:
> >   printed with %pK:             deadbeefdeadbeef
> >   printed with %p:              0xdeadbeef
> >   malformed specifier (eg %i):  0xdeadbeef
> 
> I really think we can't include SPECIAL unless _every_ callsite of %p
> is actually doing "0x%p", and then we're replacing all of those. We're
> not doing that, though...
> 
> $ git grep '%p\b' | wc -l
> 12766
> $ git grep '0x%p\b' | wc -l
> 1837
> 
> If we need some kind of special marking that this is a hashed
> variable, that should be something other than "0x". If we're using the
> existing "(null)" and new "(pointer)" text, maybe "(hash:xxxxxx)"
> should be used instead? Then the (rare) callers with 0x become
> "0x(hash:xxxx)" and naked callers produce "(hash:xxxx)".
> 
> I think the first step for this is to just leave SPECIAL out.

Thanks Kees. V9 leaves SPECIAL out. Also V9 prints the whole 64 bit
address with the first 32 bits masked to zero. The intent being to _not_
change the output format from what it currently is. So it will look like
this; 

	00000000c09e81d0

What do you think?

Amusingly I think this whole conversation is going to come up again
when we do %pa, in inverse, since %pa currently does us SPECIAL.

thanks,
Tobin.

Re: [PATCH V8 2/2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Mon, Oct 30, 2017 at 05:33:22PM -0400, Steven Rostedt wrote:
> On Thu, 26 Oct 2017 13:58:38 +1100
> "Tobin C. Harding" <me@tobin.cc> wrote:
> 
> > > +static bool have_filled_random_ptr_key;
> > > +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));
> > > +	WRITE_ONCE(have_filled_random_ptr_key, true);  
> > 
> > This usage of WRITE_ONCE was suggested by Jason A. Donenfeld. I read
> > include/linux/compiler.h but was not able to grok it. Is this enough to
> > stop the compiler re-ordering these two statements? 
> > 
> > Or do I need to read Documentation/memory-barriers.txt [again]?
> 
> No, the WRITE_ONCE does not stop the compiler from reordering those
> statements. If you need that, then you need to do:
> 
> 	get_random_bytes(&ptr_key, sizeof(ptr_key));
> 	barrier();
> 	WRITE_ONCE(have_filled_random_ptr_key, true);
> 
> and that only works against interrupts. If you need synchronization
> across CPUs, then you need smp_mb().

Cool. So I think we need

 	get_random_bytes(&ptr_key, sizeof(ptr_key));
	smp_mb();
 	WRITE_ONCE(have_filled_random_ptr_key, true);

V10 to include this unless I have it wrong.

thanks,
Tobin.

Re: [PATCH V8 2/2] printk: hash addresses printed with %p

[Steven Rostedt]

On Tue, 31 Oct 2017 09:41:02 +1100
"Tobin C. Harding" <me@tobin.cc> wrote:


> Cool. So I think we need
> 
>  	get_random_bytes(&ptr_key, sizeof(ptr_key));

You'll need to add a comment here to describe what ordering the memory
barrier is used against. That is, somewhere else there's something that
needs to see the "true" after the "get_random_bytes" update. Describe
it here.

-- Steve


> 	smp_mb();
>  	WRITE_ONCE(have_filled_random_ptr_key, true);
> 
> V10 to include this unless I have it wrong.
> 
> thanks,
> Tobin.

Re: [PATCH V8 2/2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Mon, Oct 30, 2017 at 08:00:46PM -0400, Steven Rostedt wrote:
> On Tue, 31 Oct 2017 09:41:02 +1100
> "Tobin C. Harding" <me@tobin.cc> wrote:
> 
> 
> > Cool. So I think we need
> > 
> >  	get_random_bytes(&ptr_key, sizeof(ptr_key));
> 
> You'll need to add a comment here to describe what ordering the memory
> barrier is used against. That is, somewhere else there's something that
> needs to see the "true" after the "get_random_bytes" update. Describe
> it here.

Got it, thanks Tobin.

Re: [PATCH V8 0/2] printk: hash addresses printed with %p

[Joe Perches]

On Tue, 2017-10-31 at 09:33 +1100, Tobin C. Harding wrote:
> On Mon, Oct 30, 2017 at 03:03:21PM -0700, Kees Cook wrote:
> > On Wed, Oct 25, 2017 at 7:53 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > > Here is the behaviour that this set implements.
> > > 
> > > For kpt_restrict==0
> > > 
> > > Randomness not ready:
> > >   printed with %p:              (pointer)          # NOTE: with padding
> > > Valid pointer:
> > >   printed with %pK:             deadbeefdeadbeef
> > >   printed with %p:              0xdeadbeef
> > >   malformed specifier (eg %i):  0xdeadbeef
> > 
> > I really think we can't include SPECIAL unless _every_ callsite of %p
> > is actually doing "0x%p", and then we're replacing all of those. We're
> > not doing that, though...
> > 
> > $ git grep '%p\b' | wc -l
> > 12766
> > $ git grep '0x%p\b' | wc -l
> > 18370x
> > 
> > If we need some kind of special marking that this is a hashed
> > variable, that should be something other than "0x". If we're using the
> > existing "(null)" and new "(pointer)" text, maybe "(hash:xxxxxx)"
> > should be used instead? Then the (rare) callers with 0x become
> > "0x(hash:xxxx)" and naked callers produce "(hash:xxxx)".
> > 
> > I think the first step for this is to just leave SPECIAL out.
> 
> Thanks Kees. V9 leaves SPECIAL out. Also V9 prints the whole 64 bit
> address with the first 32 bits masked to zero. The intent being to _not_
> change the output format from what it currently is. So it will look like
> this; 
> 
> 	00000000c09e81d0
> 
> What do you think?
> 
> Amusingly I think this whole conversation is going to come up again
> when we do %pa, in inverse, since %pa currently does us SPECIAL.

I once sent a patch set to remove SPECIAL from %pa
and add 0x where necessary.

https://patchwork.kernel.org/patch/3875471/

After that didn't happen, I removed the duplicated
0x%pa with a sed.

https://patchwork.kernel.org/patch/8509421/

Sending a treewide sed patch would be fine with me.

Re: [PATCH V8 0/2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Mon, Oct 30, 2017 at 07:08:48PM -0700, Joe Perches wrote:
> On Tue, 2017-10-31 at 09:33 +1100, Tobin C. Harding wrote:
> > On Mon, Oct 30, 2017 at 03:03:21PM -0700, Kees Cook wrote:
> > > On Wed, Oct 25, 2017 at 7:53 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > > > Here is the behaviour that this set implements.
> > > > 
> > > > For kpt_restrict==0
> > > > 
> > > > Randomness not ready:
> > > >   printed with %p:              (pointer)          # NOTE: with padding
> > > > Valid pointer:
> > > >   printed with %pK:             deadbeefdeadbeef
> > > >   printed with %p:              0xdeadbeef
> > > >   malformed specifier (eg %i):  0xdeadbeef
> > > 
> > > I really think we can't include SPECIAL unless _every_ callsite of %p
> > > is actually doing "0x%p", and then we're replacing all of those. We're
> > > not doing that, though...
> > > 
> > > $ git grep '%p\b' | wc -l
> > > 12766
> > > $ git grep '0x%p\b' | wc -l
> > > 18370x
> > > 
> > > If we need some kind of special marking that this is a hashed
> > > variable, that should be something other than "0x". If we're using the
> > > existing "(null)" and new "(pointer)" text, maybe "(hash:xxxxxx)"
> > > should be used instead? Then the (rare) callers with 0x become
> > > "0x(hash:xxxx)" and naked callers produce "(hash:xxxx)".
> > > 
> > > I think the first step for this is to just leave SPECIAL out.
> > 
> > Thanks Kees. V9 leaves SPECIAL out. Also V9 prints the whole 64 bit
> > address with the first 32 bits masked to zero. The intent being to _not_
> > change the output format from what it currently is. So it will look like
> > this; 
> > 
> > 	00000000c09e81d0
> > 
> > What do you think?
> > 
> > Amusingly I think this whole conversation is going to come up again
> > when we do %pa, in inverse, since %pa currently does us SPECIAL.
> 
> I once sent a patch set to remove SPECIAL from %pa
> and add 0x where necessary.
> 
> https://patchwork.kernel.org/patch/3875471/
> 
> After that didn't happen, I removed the duplicated
> 0x%pa with a sed.
> 
> https://patchwork.kernel.org/patch/8509421/
> 
> Sending a treewide sed patch would be fine with me.

Cool, thanks Joe I'll keep this in mind for when we get to %pa.

thanks,
Tobin.

Re: [PATCH V8 0/2] printk: hash addresses printed with %p

[Joe Perches]

On Wed, 2017-11-01 at 10:16 +1100, Tobin C. Harding wrote:
> On Mon, Oct 30, 2017 at 07:08:48PM -0700, Joe Perches wrote:
> > On Tue, 2017-10-31 at 09:33 +1100, Tobin C. Harding wrote:
> > > On Mon, Oct 30, 2017 at 03:03:21PM -0700, Kees Cook wrote:
> > > > On Wed, Oct 25, 2017 at 7:53 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > > > > Here is the behaviour that this set implements.
> > > > > 
> > > > > For kpt_restrict==0
> > > > > 
> > > > > Randomness not ready:
> > > > >   printed with %p:              (pointer)          # NOTE: with padding
> > > > > Valid pointer:
> > > > >   printed with %pK:             deadbeefdeadbeef
> > > > >   printed with %p:              0xdeadbeef
> > > > >   malformed specifier (eg %i):  0xdeadbeef
> > > > 
> > > > I really think we can't include SPECIAL unless _every_ callsite of %p
> > > > is actually doing "0x%p", and then we're replacing all of those. We're
> > > > not doing that, though...
> > > > 
> > > > $ git grep '%p\b' | wc -l
> > > > 12766
> > > > $ git grep '0x%p\b' | wc -l
> > > > 18370x
> > > > 
> > > > If we need some kind of special marking that this is a hashed
> > > > variable, that should be something other than "0x". If we're using the
> > > > existing "(null)" and new "(pointer)" text, maybe "(hash:xxxxxx)"
> > > > should be used instead? Then the (rare) callers with 0x become
> > > > "0x(hash:xxxx)" and naked callers produce "(hash:xxxx)".
> > > > 
> > > > I think the first step for this is to just leave SPECIAL out.
> > > 
> > > Thanks Kees. V9 leaves SPECIAL out. Also V9 prints the whole 64 bit
> > > address with the first 32 bits masked to zero. The intent being to _not_
> > > change the output format from what it currently is. So it will look like
> > > this; 
> > > 
> > > 	00000000c09e81d0
> > > 
> > > What do you think?
> > > 
> > > Amusingly I think this whole conversation is going to come up again
> > > when we do %pa, in inverse, since %pa currently does us SPECIAL.
> > 
> > I once sent a patch set to remove SPECIAL from %pa
> > and add 0x where necessary.
> > 
> > https://patchwork.kernel.org/patch/3875471/
> > 
> > After that didn't happen, I removed the duplicated
> > 0x%pa with a sed.
> > 
> > https://patchwork.kernel.org/patch/8509421/
> > 
> > Sending a treewide sed patch would be fine with me.
> 
> Cool, thanks Joe I'll keep this in mind for when we get to %pa.

fyi:  There are more of these misuses of 0x%pa now:

$ git grep -E -n "0[xX]%pa[dp]?\b"
drivers/dma/at_hdmac_regs.h:388:                 "  desc: s%pad d%pad ctrl0x%x:0x%x l0x%pad\n",
drivers/dma/coh901318.c:1322:           dev_vdbg(COHC_2_DEV(cohc), "i %d, lli %p, ctrl 0x%x, src 0x%pad"
drivers/dma/coh901318.c:1323:                    ", dst 0x%pad, link 0x%pad virt_link_addr 0x%p\n",
drivers/dma/coh901318.c:2234:            "[%s] channel %d src 0x%pad dest 0x%pad size %zu\n",
drivers/media/platform/sti/delta/delta-mem.c:35:                "%s allocate %d bytes of HW memory @(virt=0x%p, phy=0x%pad): %s\n",
drivers/media/platform/sti/delta/delta-mem.c:46:                "%s     free %d bytes of HW memory @(virt=0x%p, phy=0x%pad): %s\n",
drivers/media/platform/sti/delta/delta-v4l2.c:1147:             dev_dbg(delta->dev, "%s au[%d] prepared; virt=0x%p, phy=0x%pad\n",
drivers/media/platform/sti/delta/delta-v4l2.c:1503:                     "%s frame[%d] prepared; virt=0x%p, phy=0x%pad\n",
drivers/media/platform/stm32/stm32-dcmi.c:486:          dev_dbg(dcmi->dev, "buffer[%d] phy=0x%pad size=%zu\n",
drivers/media/platform/ti-vpe/cal.c:496:        cal_info(dev, "CAL Registers @ 0x%pa:\n", &dev->res->start);

Re: [PATCH V8 0/2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Fri, Oct 27, 2017 at 10:33:01PM +0900, Sergey Senozhatsky wrote:
> On (10/26/17 13:53), 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.
> > 
> > With this version we include hashing of malformed specifiers also.
> > Malformed specifiers include incomplete (e.g %pi) and also non-existent
> > specifiers. checkpatch should warn for non-existent specifiers but
> > AFAICT won't warn for incomplete specifiers.
> > 
> > Here is the behaviour that this set implements.
> > 
> > For kpt_restrict==0
> > 
> > Randomness not ready:
> >   printed with %p: 		(pointer)          # NOTE: with padding
> > Valid pointer:
> >   printed with %pK: 		deadbeefdeadbeef
> >   printed with %p: 		0xdeadbeef
> >   malformed specifier (eg %i):  0xdeadbeef
> > NULL pointer:
> >   printed with %pK: 		0000000000000000
> >   printed with %p: 		(null)               # NOTE: no padding
> >   malformed specifier (eg %i):  (null)
> 
> a quick question:
>  do we care about cases when kernel pointers are printed with %x/%X and
>  not with %p?

Yes. The question has been raised will we be here again in 6 years time
trying to fix all the uses of %x. And there are already 29K uses of
%[xX] in tree, which of these are leaking addresses? This is why Linus'
has commented that really effort should be directed at finding the leaks
as they happen (in procfs, sysfs, dmesg) instead of fixing this in
the code. So far I haven't been able to come up with any meaningful way
to do this on 32 bit machines. There is a patch adding a script to catch
leaks on 64 bit machines in flight.

This patch needs to be a small part of a continued effort to stop the
leaks if we want to have any hope of stopping them.

If you have any suggestions on dealing with %x please do say. We have
code changes, compiler warnings, and checkpatch - none of which
immediately seem great.

thanks,
Tobin.

Re: [PATCH V8 0/2] printk: hash addresses printed with %p

[Sergey Senozhatsky]

On (11/01/17 10:35), Tobin C. Harding wrote:
[..]
> Yes. The question has been raised will we be here again in 6 years time
> trying to fix all the uses of %x. And there are already 29K uses of
> %[xX] in tree, which of these are leaking addresses? This is why Linus'
> has commented that really effort should be directed at finding the leaks
> as they happen (in procfs, sysfs, dmesg) instead of fixing this in
> the code.

got it. thanks.

> So far I haven't been able to come up with any meaningful way
> to do this on 32 bit machines. There is a patch adding a script to catch
> leaks on 64 bit machines in flight.

who is expected to run that script?

BTW, can BPF/eBPF printk addresses?

> This patch needs to be a small part of a continued effort to stop the
> leaks if we want to have any hope of stopping them.
> 
> If you have any suggestions on dealing with %x please do say. We have
> code changes, compiler warnings, and checkpatch - none of which
> immediately seem great.

hm... just a huge pile of if's

	if (is_vmalloc_addr(addr))
		do_hashing(addr);
	else if (__module_address(addr))
		do_hashing(addr);
	else if (is_kernel(addr) || is_kernel_inittext(addr))
	...

but that's going to be really messy and "iffy".

	-ss

Re: [PATCH V8 0/2] printk: hash addresses printed with %p

[Tobin C. Harding]

On Thu, Nov 02, 2017 at 05:23:44PM +0900, Sergey Senozhatsky wrote:
> On (11/01/17 10:35), Tobin C. Harding wrote:
> [..]
> > Yes. The question has been raised will we be here again in 6 years time
> > trying to fix all the uses of %x. And there are already 29K uses of
> > %[xX] in tree, which of these are leaking addresses? This is why Linus'
> > has commented that really effort should be directed at finding the leaks
> > as they happen (in procfs, sysfs, dmesg) instead of fixing this in
> > the code.
> 
> got it. thanks.
> 
> > So far I haven't been able to come up with any meaningful way
> > to do this on 32 bit machines. There is a patch adding a script to catch
> > leaks on 64 bit machines in flight.
> 
> who is expected to run that script?

If one person runs it and finds one leaking address, I'd say it wast
worth writing. If a bunch of people with different set ups run it and we
find a bunch of leaking addresses, WIN!

Your comment did give me the idea of adding some output to the command
offering an email address to send suspicious output for those who do not
wish to investigate it further. I can put my email address if there is
not a better option. 

> BTW, can BPF/eBPF printk addresses?

I know absolutely zero about BPF/eBPF. I guess now is a good time to learn.

> > This patch needs to be a small part of a continued effort to stop the
> > leaks if we want to have any hope of stopping them.
> > 
> > If you have any suggestions on dealing with %x please do say. We have
> > code changes, compiler warnings, and checkpatch - none of which
> > immediately seem great.
> 
> hm... just a huge pile of if's
> 
> 	if (is_vmalloc_addr(addr))
> 		do_hashing(addr);
> 	else if (__module_address(addr))
> 		do_hashing(addr);
> 	else if (is_kernel(addr) || is_kernel_inittext(addr))
> 	...
> 
> but that's going to be really messy and "iffy".

This is the only suggestion we have so far.

thanks,
Tobin.

RE: [PATCH V8 0/2] printk: hash addresses printed with %p

[Roberts, William C]

> -----Original Message-----
> From: Tobin C. Harding [mailto:me@tobin.cc]
> Sent: Thursday, November 2, 2017 3:15 AM
> To: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
> Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>; kernel-
> hardening@lists.openwall.com; Jason A. Donenfeld <Jason@zx2c4.com>;
> Theodore Ts'o <tytso@mit.edu>; 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>; Catalin Marinas
> <catalin.marinas@arm.com>; Will Deacon <wilal.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: Re: [PATCH V8 0/2] printk: hash addresses printed with %p
> 
> On Thu, Nov 02, 2017 at 05:23:44PM +0900, Sergey Senozhatsky wrote:
> > On (11/01/17 10:35), Tobin C. Harding wrote:
> > [..]
> > > Yes. The question has been raised will we be here again in 6 years
> > > time trying to fix all the uses of %x. And there are already 29K
> > > uses of %[xX] in tree, which of these are leaking addresses? This is why Linus'
> > > has commented that really effort should be directed at finding the
> > > leaks as they happen (in procfs, sysfs, dmesg) instead of fixing
> > > this in the code.
> >
> > got it. thanks.
> >
> > > So far I haven't been able to come up with any meaningful way to do
> > > this on 32 bit machines. There is a patch adding a script to catch
> > > leaks on 64 bit machines in flight.
> >
> > who is expected to run that script?
> 
> If one person runs it and finds one leaking address, I'd say it wast worth writing. If
> a bunch of people with different set ups run it and we find a bunch of leaking
> addresses, WIN!

I wonder if the 0 day testing robot could run it....

> 
> Your comment did give me the idea of adding some output to the command
> offering an email address to send suspicious output for those who do not wish to
> investigate it further. I can put my email address if there is not a better option.
> 
> > BTW, can BPF/eBPF printk addresses?
> 
> I know absolutely zero about BPF/eBPF. I guess now is a good time to learn.
> 
> > > This patch needs to be a small part of a continued effort to stop
> > > the leaks if we want to have any hope of stopping them.
> > >
> > > If you have any suggestions on dealing with %x please do say. We
> > > have code changes, compiler warnings, and checkpatch - none of which
> > > immediately seem great.
> >
> > hm... just a huge pile of if's
> >
> > 	if (is_vmalloc_addr(addr))
> > 		do_hashing(addr);
> > 	else if (__module_address(addr))
> > 		do_hashing(addr);
> > 	else if (is_kernel(addr) || is_kernel_inittext(addr))
> > 	...
> >
> > but that's going to be really messy and "iffy".
> 
> This is the only suggestion we have so far.
> 
> thanks,
> Tobin.

Re: [PATCH V8 0/2] printk: hash addresses printed with %p

[Sergey Senozhatsky]

On (11/02/17 21:14), Tobin C. Harding wrote:
[..]
> I can put my email address if there is not a better option.

sounds good.


> > hm... just a huge pile of if's
> > 
> > 	if (is_vmalloc_addr(addr))
> > 		do_hashing(addr);
> > 	else if (__module_address(addr))
> > 		do_hashing(addr);
> > 	else if (is_kernel(addr) || is_kernel_inittext(addr))
> > 	...
> > 
> > but that's going to be really messy and "iffy".
> 
> This is the only suggestion we have so far.
> 

well... one more: check if we can safely dereference it. if so
it's a pointer, probably :)

	if (!probe_kernel_address(addr, p))
		do_hashing(addr);


just an idea.

	-ss

Re: [PATCH V8 0/2] printk: hash addresses printed with %p

[Vinod Koul]

On Tue, Oct 31, 2017 at 04:33:13PM -0700, Joe Perches wrote:
> On Wed, 2017-11-01 at 10:16 +1100, Tobin C. Harding wrote:

> > Cool, thanks Joe I'll keep this in mind for when we get to %pa.
> 
> fyi:  There are more of these misuses of 0x%pa now:
> 
> $ git grep -E -n "0[xX]%pa[dp]?\b"
> drivers/dma/at_hdmac_regs.h:388:                 "  desc: s%pad d%pad ctrl0x%x:0x%x l0x%pad\n",
> drivers/dma/coh901318.c:1322:           dev_vdbg(COHC_2_DEV(cohc), "i %d, lli %p, ctrl 0x%x, src 0x%pad"
> drivers/dma/coh901318.c:1323:                    ", dst 0x%pad, link 0x%pad virt_link_addr 0x%p\n",
> drivers/dma/coh901318.c:2234:            "[%s] channel %d src 0x%pad dest 0x%pad size %zu\n",

Fixed and sent patches for these two.

-- 
~Vinod