[PATCH V11 0/5] hash addresses printed with %p

Currently there exist approximately 14 000 places in the Kernel where
addresses are being printed using an unadorned %p. This potentially
leaks sensitive information regarding the Kernel layout in memory. Many
of these calls are stale, instead of fixing every call lets hash the
address by default before printing. This will of course break some
users, forcing code printing needed addresses to be updated. We can add
a printk specifier for this purpose (%px) to give developers a clear
upgrade path for breakages caused by applying this patch set.

The added advantage of hashing %p is that security is now opt-out, if
you _really_ want the address you have to work a little harder and use
%px.

The idea for creating the printk specifier %px to print the actual
address was suggested by Kees Cook (see below for email threads by
subject).

Newbie question: I don't know who is potentially going to want to apply
this, I've CC'd Andrew Morton. I'm guessing this should go into
linux-next so we can see what breaks? I do not know exactly how code
gets into linux-next. I've CC'd Stephen Rothwell.

Here is the behaviour that this series implements.

For kpt_restrict==0

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

For kpt_restrict==2

Valid pointer:
  printed with %pK:             0000000000000000

All other output as for kptr_restrict==0

V11:
 - Add patch fixing %pK documentation.
 - Refactor %pK as a separate patch.
 - Add patch adding printk specifier %px, which prints the actual
   address (i.e replaces original %p behaviour). 
 - Use %px for KASAN patch.

V10:
 - Add patch so KASAN uses %pK instead of %p. 
 - Add documentation to Documentation/printk-formats.txt
 - Add tests to lib/test_printf.c
 - Change "(pointer value)" -> "(ptrval)" to fit within columns on 32
   bit machines.

V9:
 - Drop the initial patch from V8, leaving null pointer handling as is.
 - Print the hashed ID _without_ a '0x' suffix.
 - Mask the first 32 bits of the hashed ID to all zeros on 64 bit
   architectures.

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 0/5] add printk specifier %px, unique identifier
[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 (5):
  docs: correct documentation for %pK
  vsprintf: refactor %pK code out of pointer()
  printk: hash addresses printed with %p
  vsprintf: add printk specifier %px
  kasan: use %px to print addresses instead of %p

 Documentation/printk-formats.txt |  31 ++++++-
 lib/test_printf.c                | 108 ++++++++++++++--------
 lib/vsprintf.c                   | 194 +++++++++++++++++++++++++++++----------
 mm/kasan/report.c                |   8 +-
 scripts/checkpatch.pl            |   2 +-
 5 files changed, 248 insertions(+), 95 deletions(-)

-- 
2.7.4

Current documentation indicates that %pK prints a leading '0x'. This is
not the case.

Correct documentation for printk specifier %pK.

Signed-off-by: Tobin C. Harding <redacted>
---
 Documentation/printk-formats.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 361789df51ec..71b62db7eca2 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt

@@ -85,13 +85,12 @@ Examples::
 	printk("Faulted at %pS\n", (void *)regs->ip);
 	printk(" %s%pB\n", (reliable ? "" : "? "), (void *)*stack);
 
-
 Kernel Pointers
 ===============
 
 ::
 
-	%pK	0x01234567 or 0x0123456789abcdef
+	%pK	01234567 or 0123456789abcdef
 
 For printing kernel pointers which should be hidden from unprivileged
 users. The behaviour of ``%pK`` depends on the ``kptr_restrict sysctl`` - see

-- 
2.7.4

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

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

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

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

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

Signed-off-by: Tobin C. Harding <redacted>
---
 Documentation/printk-formats.txt |  12 ++++-
 lib/test_printf.c                | 108 +++++++++++++++++++++++++--------------
 lib/vsprintf.c                   |  81 ++++++++++++++++++++++++++---
 3 files changed, 155 insertions(+), 46 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 71b62db7eca2..b4e668ac4fe3 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt

@@ -5,7 +5,6 @@ How to get printk format specifiers right
 :Author: Randy Dunlap <rdunlap@infradead.org>
 :Author: Andrew Murray <amurray@mpc-data.co.uk>
 
-
 Integer types
 =============
 

@@ -45,6 +44,17 @@ return from vsnprintf.
 Raw pointer value SHOULD be printed with %p. The kernel supports
 the following extended format specifiers for pointer types:
 
+Pointer Types
+=============
+
+Pointers printed without a specifier extension (i.e unadorned %p) are
+hashed to give a unique identifier without leaking kernel addresses to user
+space. On 64 bit machines the first 32 bits are zeroed.
+
+::
+
+	%p	abcdef12 or 00000000abcdef12
+
 Symbols/Function Pointers
 =========================
 

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 563f10e6876a..71ebfa43ad05 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c

@@ -24,24 +24,6 @@
 #define PAD_SIZE 16
 #define FILL_CHAR '$'
 
-#define PTR1 ((void*)0x01234567)
-#define PTR2 ((void*)(long)(int)0xfedcba98)
-
-#if BITS_PER_LONG == 64
-#define PTR1_ZEROES "000000000"
-#define PTR1_SPACES "         "
-#define PTR1_STR "1234567"
-#define PTR2_STR "fffffffffedcba98"
-#define PTR_WIDTH 16
-#else
-#define PTR1_ZEROES "0"
-#define PTR1_SPACES " "
-#define PTR1_STR "1234567"
-#define PTR2_STR "fedcba98"
-#define PTR_WIDTH 8
-#endif
-#define PTR_WIDTH_STR stringify(PTR_WIDTH)
-
 static unsigned total_tests __initdata;
 static unsigned failed_tests __initdata;
 static char *test_buffer __initdata;

@@ -217,30 +199,79 @@ test_string(void)
 	test("a  |   |   ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, "c");
 }
 
+#define PLAIN_BUF_SIZE 64	/* leave some space so we don't oops */
+
+#if BITS_PER_LONG == 64
+
+#define PTR_WIDTH 16
+#define PTR ((void *)0xffff0123456789ab)
+#define PTR_STR "ffff0123456789ab"
+#define ZEROS "00000000"	/* hex 32 zero bits */
+
+static int __init
+plain_format(void)
+{
+	char buf[PLAIN_BUF_SIZE];
+	int nchars;
+
+	nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR);
+
+	if (nchars != PTR_WIDTH || strncmp(buf, ZEROS, strlen(ZEROS)) != 0)
+		return -1;
+
+	return 0;
+}
+
+#else
+
+#define PTR_WIDTH 8
+#define PTR ((void *)0x456789ab)
+#define PTR_STR "456789ab"
+
+static int __init
+plain_format(void)
+{
+	/* Format is implicitly tested for 32 bit machines by plain_hash() */
+	return 0;
+}
+
+#endif	/* BITS_PER_LONG == 64 */
+
+static int __init
+plain_hash(void)
+{
+	char buf[PLAIN_BUF_SIZE];
+	int nchars;
+
+	nchars = snprintf(buf, PLAIN_BUF_SIZE, "%p", PTR);
+
+	if (nchars != PTR_WIDTH || strncmp(buf, PTR_STR, PTR_WIDTH) == 0)
+		return -1;
+
+	return 0;
+}
+
+/*
+ * We can't use test() to test %p because we don't know what output to expect
+ * after an address is hashed.
+ */
 static void __init
 plain(void)
 {
-	test(PTR1_ZEROES PTR1_STR " " PTR2_STR, "%p %p", PTR1, PTR2);
-	/*
-	 * The field width is overloaded for some %p extensions to
-	 * pass another piece of information. For plain pointers, the
-	 * behaviour is slightly odd: One cannot pass either the 0
-	 * flag nor a precision to %p without gcc complaining, and if
-	 * one explicitly gives a field width, the number is no longer
-	 * zero-padded.
-	 */
-	test("|" PTR1_STR PTR1_SPACES "  |  " PTR1_SPACES PTR1_STR "|",
-	     "|%-*p|%*p|", PTR_WIDTH+2, PTR1, PTR_WIDTH+2, PTR1);
-	test("|" PTR2_STR "  |  " PTR2_STR "|",
-	     "|%-*p|%*p|", PTR_WIDTH+2, PTR2, PTR_WIDTH+2, PTR2);
+	int err;
 
-	/*
-	 * Unrecognized %p extensions are treated as plain %p, but the
-	 * alphanumeric suffix is ignored (that is, does not occur in
-	 * the output.)
-	 */
-	test("|"PTR1_ZEROES PTR1_STR"|", "|%p0y|", PTR1);
-	test("|"PTR2_STR"|", "|%p0y|", PTR2);
+	err = plain_hash();
+	if (err) {
+		pr_warn("plain 'p' does not appear to be hashed\n");
+		failed_tests++;
+		return;
+	}
+
+	err = plain_format();
+	if (err) {
+		pr_warn("hashing plain 'p' has unexpected format\n");
+		failed_tests++;
+	}
 }
 
 static void __init

@@ -251,6 +282,7 @@ symbol_ptr(void)
 static void __init
 kernel_ptr(void)
 {
+	/* We can't test this without access to kptr_restrict. */
 }
 
 static void __init

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 8dc5cf85cef4..d69452a0f2fa 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

@@ -1644,6 +1646,73 @@ 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 __read_mostly;
+static siphash_key_t ptr_key __read_mostly;
+
+static void fill_random_ptr_key(struct random_ready_callback *unused)
+{
+	get_random_bytes(&ptr_key, sizeof(ptr_key));
+	/*
+	 * have_filled_random_ptr_key==true is dependent on get_random_bytes().
+	 * ptr_to_id() needs to see have_filled_random_ptr_key==true
+	 * after get_random_bytes() returns.
+	 */
+	smp_mb();
+	WRITE_ONCE(have_filled_random_ptr_key, true);
+}
+
+static struct random_ready_callback random_ready = {
+	.func = fill_random_ptr_key
+};
+
+static int __init initialize_ptr_random(void)
+{
+	int ret = add_random_ready_callback(&random_ready);
+
+	if (!ret) {
+		return 0;
+	} else if (ret == -EALREADY) {
+		fill_random_ptr_key(&random_ready);
+		return 0;
+	}
+
+	return ret;
+}
+early_initcall(initialize_ptr_random);
+
+/* Maps a pointer to a 32 bit unique identifier. */
+static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
+{
+	unsigned long hashval;
+	const int default_width = 2 * sizeof(ptr);
+
+	if (unlikely(!have_filled_random_ptr_key)) {
+		spec.field_width = default_width;
+		/* string length must be less than default_width */
+		return string(buf, end, "(ptrval)", spec);
+	}
+
+#ifdef CONFIG_64BIT
+	hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
+	/*
+	 * Mask off the first 32 bits, this makes explicit that we have
+	 * modified the address (and 32 bits is plenty for a unique ID).
+	 */
+	hashval = hashval & 0xffffffff;
+#else
+	hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
+#endif
+
+	spec.flags |= SMALL;
+	if (spec.field_width == -1) {
+		spec.field_width = default_width;
+		spec.flags |= ZEROPAD;
+	}
+	spec.base = 16;
+
+	return number(buf, end, hashval, spec);
+}
+
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format

@@ -1754,6 +1823,9 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
  * 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,

@@ -1869,14 +1941,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

printk specifier %p now hashes all addresses before printing. Sometimes
we need to see the actual unmodified address. This can be achieved using
%lx but then we face the risk that if in future we want to change the
way the Kernel handles printing of pointers we will have to grep through
the already existent 50 000 %lx call sites. Let's add specifier %px as a
clear, opt-in, way to print a pointer and maintain some level of
isolation from all the other hex integer output within the Kernel.

Add printk specifier %px to print the actual unmodified address.

Signed-off-by: Tobin C. Harding <redacted>
---
 Documentation/printk-formats.txt | 18 +++++++++++++++++-
 lib/vsprintf.c                   | 18 ++++++++++++++++++
 scripts/checkpatch.pl            |  2 +-
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index b4e668ac4fe3..aa0a776c817a 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt

@@ -49,7 +49,8 @@ Pointer Types
 
 Pointers printed without a specifier extension (i.e unadorned %p) are
 hashed to give a unique identifier without leaking kernel addresses to user
-space. On 64 bit machines the first 32 bits are zeroed.
+space. On 64 bit machines the first 32 bits are zeroed. If you _really_
+want the address see %px below.
 
 ::
 

@@ -106,6 +107,21 @@ For printing kernel pointers which should be hidden from unprivileged
 users. The behaviour of ``%pK`` depends on the ``kptr_restrict sysctl`` - see
 Documentation/sysctl/kernel.txt for more details.
 
+Unmodified Addresses
+====================
+
+::
+
+	%px	01234567 or 0123456789abcdef
+
+For printing pointers when you _really_ want to print the address. Please
+consider whether or not you are leaking sensitive information about the
+Kernel layout in memory before printing pointers with %px. %px is
+functionally equivalent to %lx. %px is preferred to %lx because it is more
+uniquely grep'able. If, in the future, we need to modify the way the Kernel
+handles printing pointers it will be nice to be able to find the call
+sites.
+
 Struct Resources
 ================
 

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d69452a0f2fa..d960aead0336 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c

@@ -1646,6 +1646,20 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
+static noinline_for_stack
+char *pointer_string(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(ptr);
+		spec.flags |= ZEROPAD;
+	}
+
+	return number(buf, end, (unsigned long int)ptr, spec);
+}
+
 static bool have_filled_random_ptr_key __read_mostly;
 static siphash_key_t ptr_key __read_mostly;
 

@@ -1818,6 +1832,8 @@ static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
  *                        c major compatible string
  *                        C full compatible string
  *
+ * - 'x' For printing the address. Equivalent to "%lx".
+ *
  * ** Please update also Documentation/printk-formats.txt when making changes **
  *
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64

@@ -1940,6 +1956,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		case 'F':
 			return device_node_string(buf, end, ptr, spec, fmt + 1);
 		}
+	case 'x':
+		return pointer_string(buf, end, ptr, spec);
 	}
 
 	/* default is to _not_ leak addresses, hash before printing */

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 95cda3ecc66b..040aa79e1d9d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl

@@ -5753,7 +5753,7 @@ sub process {
 		        for (my $count = $linenr; $count <= $lc; $count++) {
 				my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0));
 				$fmt =~ s/%%//g;
-				if ($fmt =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGNO]).)/) {
+				if ($fmt =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGNOx]).)/) {
 					$bad_extension = $1;
 					last;
 				}

-- 
2.7.4

Pointers printed with %p are now hashed by default. Kasan needs the
actual address. We can use the new printk specifier %px for this
purpose.

Use %px instead of %p to print addresses.

Signed-off-by: Tobin C. Harding <redacted>
---
 mm/kasan/report.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 6bcfb01ba038..410c8235e671 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c

@@ -134,7 +134,7 @@ static void print_error_description(struct kasan_access_info *info)
 
 	pr_err("BUG: KASAN: %s in %pS\n",
 		bug_type, (void *)info->ip);
-	pr_err("%s of size %zu at addr %p by task %s/%d\n",
+	pr_err("%s of size %zu at addr %px by task %s/%d\n",
 		info->is_write ? "Write" : "Read", info->access_size,
 		info->access_addr, current->comm, task_pid_nr(current));
 }

@@ -206,7 +206,7 @@ static void describe_object_addr(struct kmem_cache *cache, void *object,
 	const char *rel_type;
 	int rel_bytes;
 
-	pr_err("The buggy address belongs to the object at %p\n"
+	pr_err("The buggy address belongs to the object at %px\n"
 	       " which belongs to the cache %s of size %d\n",
 		object, cache->name, cache->object_size);
 

@@ -225,7 +225,7 @@ static void describe_object_addr(struct kmem_cache *cache, void *object,
 	}
 
 	pr_err("The buggy address is located %d bytes %s of\n"
-	       " %d-byte region [%p, %p)\n",
+	       " %d-byte region [%px, %px)\n",
 		rel_bytes, rel_type, cache->object_size, (void *)object_addr,
 		(void *)(object_addr + cache->object_size));
 }

@@ -302,7 +302,7 @@ static void print_shadow_for_address(const void *addr)
 		char shadow_buf[SHADOW_BYTES_PER_ROW];
 
 		snprintf(buffer, sizeof(buffer),
-			(i == 0) ? ">%p: " : " %p: ", kaddr);
+			(i == 0) ? ">%px: " : " %px: ", kaddr);
 		/*
 		 * We should not pass a shadow pointer to generic
 		 * function, because generic functions may try to

-- 
2.7.4

Currently code to handle %pK is all within the switch statement in
pointer(). This is the wrong level of abstraction. Each of the other switch
clauses call a helper function, pK should do the same.

Refactor code out of pointer() to new function restricted_pointer().

Signed-off-by: Tobin C. Harding <redacted>
---
 lib/vsprintf.c | 97 ++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 54 insertions(+), 43 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 1746bae94d41..8dc5cf85cef4 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c

@@ -1343,6 +1343,59 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 	return string(buf, end, uuid, spec);
 }
 
+int kptr_restrict __read_mostly;
+
+static noinline_for_stack
+char *restricted_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(ptr);
+		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)
 {

@@ -1591,8 +1644,6 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
-int kptr_restrict __read_mostly;
-
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format

@@ -1792,47 +1843,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 restricted_pointer(buf, end, ptr, spec);
 	case 'N':
 		return netdev_bits(buf, end, ptr, fmt);
 	case 'a':

-- 
2.7.4

On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding [off-list ref] wrote:

   Let's add specifier %px as a
clear, opt-in, way to print a pointer and maintain some level of
isolation from all the other hex integer output within the Kernel.

Yes, I like this model. It's easy and it's obvious ("'x' for hex"),
and it gives people a good way to say "yes, I really want the actual
address as hex" for if/when the hashed pointer doesn't work for some
reason.

So me likey.

And as with the address leaking script, I'd like it even more if you
made it a git tree and I'll pull it.

Thanks,

                      Linus

On Wed, 29 Nov 2017 13:05:02 +1100
"Tobin C. Harding" [off-list ref] wrote:

+		/*
+		 * 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())

This could be replaced with:

		if (!in_task())

Which is actually more efficient.

-- Steve

+			return string(buf, end, "pK-error", spec);

On Tue, Nov 28, 2017 at 09:39:57PM -0500, Steven Rostedt wrote:

On Wed, 29 Nov 2017 13:05:02 +1100
"Tobin C. Harding" [off-list ref] wrote:

quoted

+		/*
+		 * 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())

This could be replaced with:

		if (!in_task())

Which is actually more efficient.

thanks for the comment Steve. At this late stage in the game do you mind
if I don't include this change in this set. The code line in question is
only in the series because of refactoring. I'm comfortable arguing that
improving efficiency is out of scope ;)

thanks,
Tobin.

On Tue, Nov 28, 2017 at 06:29:02PM -0800, Linus Torvalds wrote:

On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding [off-list ref] wrote:

quoted

   Let's add specifier %px as a
clear, opt-in, way to print a pointer and maintain some level of
isolation from all the other hex integer output within the Kernel.

Yes, I like this model. It's easy and it's obvious ("'x' for hex"),
and it gives people a good way to say "yes, I really want the actual
address as hex" for if/when the hashed pointer doesn't work for some
reason.

So me likey.

BOOM!

And as with the address leaking script, I'd like it even more if you
made it a git tree and I'll pull it.

Pull request to come.

thanks,
Tobin.

From: Linus Torvalds

Sent: 29 November 2017 02:29

On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding [off-list ref] wrote:

quoted

   Let's add specifier %px as a
clear, opt-in, way to print a pointer and maintain some level of
isolation from all the other hex integer output within the Kernel.

Yes, I like this model. It's easy and it's obvious ("'x' for hex"),
and it gives people a good way to say "yes, I really want the actual
address as hex" for if/when the hashed pointer doesn't work for some
reason.

Remind me to change every %p to %px on kernels that support it.

Although the absolute values of pointers may not be useful, knowing
that two pointer differ by a small amount is useful.
It is also useful to know whether pointers are to stack, code, static
data or heap.

This change to %p is going to make debugging a nightmare.

	David

On Wed, 29 Nov 2017 15:27:46 +1100
"Tobin C. Harding" [off-list ref] wrote:

On Tue, Nov 28, 2017 at 09:39:57PM -0500, Steven Rostedt wrote:

quoted

On Wed, 29 Nov 2017 13:05:02 +1100
"Tobin C. Harding" [off-list ref] wrote:
  

quoted

+		/*
+		 * 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())  

This could be replaced with:

		if (!in_task())

Which is actually more efficient.  

thanks for the comment Steve. At this late stage in the game do you mind
if I don't include this change in this set. The code line in question is
only in the series because of refactoring. I'm comfortable arguing that
improving efficiency is out of scope ;)

No problem. In fact, I can send this as a separate patch myself, on top
of your series.

-- Steve

On Wed, Nov 29, 2017 at 2:07 AM, David Laight [off-list ref] wrote:

From: Linus Torvalds

quoted

Sent: 29 November 2017 02:29

On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding [off-list ref] wrote:

quoted

   Let's add specifier %px as a
clear, opt-in, way to print a pointer and maintain some level of
isolation from all the other hex integer output within the Kernel.

Yes, I like this model. It's easy and it's obvious ("'x' for hex"),
and it gives people a good way to say "yes, I really want the actual
address as hex" for if/when the hashed pointer doesn't work for some
reason.

Remind me to change every %p to %px on kernels that support it.

Although the absolute values of pointers may not be useful, knowing
that two pointer differ by a small amount is useful.
It is also useful to know whether pointers are to stack, code, static
data or heap.

This change to %p is going to make debugging a nightmare.

In the future, maybe we could have a knob: unhashed, hashed (default),
or zeroed.

-Kees

-- 
Kees Cook
Pixel Security

-----Original Message-----
From: keescook@google.com [mailto:keescook@google.com] On Behalf Of Kees
Cook
Sent: Wednesday, November 29, 2017 2:28 PM
To: David Laight <redacted>
Cc: Linus Torvalds <torvalds@linux-foundation.org>; Tobin C. Harding
[off-list ref]; kernel-hardening@lists.openwall.com; Jason A. Donenfeld
[off-list ref]; Theodore Ts'o [off-list ref]; Paolo Bonzini
[off-list ref]; Tycho Andersen [off-list ref]; Roberts, William C
[off-list ref]; Tejun Heo [off-list ref]; Jordan Glover
[off-list ref]; Greg KH [off-list ref];
Petr Mladek [off-list ref]; Joe Perches [off-list ref]; Ian
Campbell [off-list ref]; Sergey Senozhatsky
[off-list ref]; Catalin Marinas [off-list ref];
Will Deacon [off-list ref]; Steven Rostedt [off-list ref];
Chris Fries [off-list ref]; Dave Weinstein [off-list ref]; Daniel
Micay [off-list ref]; Djalal Harouni [off-list ref]; Radim
Krcmár [off-list ref]; Linux Kernel Mailing List <linux-
kernel@vger.kernel.org>; Network Development [off-list ref];
David Miller [off-list ref]; Stephen Rothwell
[off-list ref]; Andrey Ryabinin [off-list ref];
Alexander Potapenko [off-list ref]; Dmitry Vyukov
[off-list ref]; Andrew Morton [off-list ref]
Subject: Re: [PATCH V11 4/5] vsprintf: add printk specifier %px

On Wed, Nov 29, 2017 at 2:07 AM, David Laight [off-list ref]
wrote:

quoted

From: Linus Torvalds

quoted

Sent: 29 November 2017 02:29

On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding [off-list ref] wrote:

quoted

   Let's add specifier %px as a
clear, opt-in, way to print a pointer and maintain some level of
isolation from all the other hex integer output within the Kernel.

Yes, I like this model. It's easy and it's obvious ("'x' for hex"),
and it gives people a good way to say "yes, I really want the actual
address as hex" for if/when the hashed pointer doesn't work for some
reason.

Remind me to change every %p to %px on kernels that support it.

Although the absolute values of pointers may not be useful, knowing
that two pointer differ by a small amount is useful.
It is also useful to know whether pointers are to stack, code, static
data or heap.

This change to %p is going to make debugging a nightmare.

In the future, maybe we could have a knob: unhashed, hashed (default), or
zeroed.

Isn't that just kptr_restrict and get us right back to the simpler patches I proposed?

-Kees

--
Kees Cook
Pixel Security

On Wed, Nov 29, 2017 at 2:28 PM, Kees Cook [off-list ref] wrote:

In the future, maybe we could have a knob: unhashed, hashed (default),
or zeroed.

I haven't actually seen a case for that yet.

Let's see if there are actually any debug issues at all, and how big
they are before worrying about it.

                   Linus

On Wed, 29 Nov 2017 13:05:00 +1100 "Tobin C. Harding" [off-list ref] wrote:

Currently there exist approximately 14 000 places in the Kernel where
addresses are being printed using an unadorned %p. This potentially
leaks sensitive information regarding the Kernel layout in memory. Many
of these calls are stale, instead of fixing every call lets hash the
address by default before printing. This will of course break some
users, forcing code printing needed addresses to be updated. We can add
a printk specifier for this purpose (%px) to give developers a clear
upgrade path for breakages caused by applying this patch set.

The added advantage of hashing %p is that security is now opt-out, if
you _really_ want the address you have to work a little harder and use
%px.

The idea for creating the printk specifier %px to print the actual
address was suggested by Kees Cook (see below for email threads by
subject).

Maybe I'm being thick, but...  if we're rendering these addresses
unusable by hashing them, why not just print something like
"<obscured>" in their place?  That loses the uniqueness thing but I
wonder how valuable that is in practice?

On Wed, 29 Nov 2017 13:05:04 +1100 "Tobin C. Harding" [off-list ref] wrote:

printk specifier %p now hashes all addresses before printing. Sometimes
we need to see the actual unmodified address. This can be achieved using
%lx but then we face the risk that if in future we want to change the
way the Kernel handles printing of pointers we will have to grep through
the already existent 50 000 %lx call sites. Let's add specifier %px as a
clear, opt-in, way to print a pointer and maintain some level of
isolation from all the other hex integer output within the Kernel.

Add printk specifier %px to print the actual unmodified address.

...

+Unmodified Addresses
+====================
+
+::
+
+	%px	01234567 or 0123456789abcdef
+
+For printing pointers when you _really_ want to print the address. Please
+consider whether or not you are leaking sensitive information about the
+Kernel layout in memory before printing pointers with %px. %px is
+functionally equivalent to %lx. %px is preferred to %lx because it is more
+uniquely grep'able. If, in the future, we need to modify the way the Kernel
+handles printing pointers it will be nice to be able to find the call
+sites.
+

You might want to add a checkpatch rule which emits a stern
do-you-really-want-to-do-this warning when someone uses %px.

On Wed, 29 Nov 2017 13:05:03 +1100 "Tobin C. Harding" [off-list ref] wrote:

I don't think I'm seeing anything which prevents two CPUs from
initializing ptr_key at the same time.  Probably doesn't matter much...

On Wed, Nov 29, 2017 at 03:20:58PM -0800, Andrew Morton wrote:

On Wed, 29 Nov 2017 13:05:04 +1100 "Tobin C. Harding" [off-list ref] wrote:

quoted

printk specifier %p now hashes all addresses before printing. Sometimes
we need to see the actual unmodified address. This can be achieved using
%lx but then we face the risk that if in future we want to change the
way the Kernel handles printing of pointers we will have to grep through
the already existent 50 000 %lx call sites. Let's add specifier %px as a
clear, opt-in, way to print a pointer and maintain some level of
isolation from all the other hex integer output within the Kernel.

Add printk specifier %px to print the actual unmodified address.

...

+Unmodified Addresses
+====================
+
+::
+
+	%px	01234567 or 0123456789abcdef
+
+For printing pointers when you _really_ want to print the address. Please
+consider whether or not you are leaking sensitive information about the
+Kernel layout in memory before printing pointers with %px. %px is
+functionally equivalent to %lx. %px is preferred to %lx because it is more
+uniquely grep'able. If, in the future, we need to modify the way the Kernel
+handles printing pointers it will be nice to be able to find the call
+sites.
+

You might want to add a checkpatch rule which emits a stern
do-you-really-want-to-do-this warning when someone uses %px.

Oh, nice idea. It has to be a CHECK but right? By stern, you mean use
stern language?

thanks,
Tobin.

On Wed, Nov 29, 2017 at 03:20:40PM -0800, Andrew Morton wrote:

On Wed, 29 Nov 2017 13:05:00 +1100 "Tobin C. Harding" [off-list ref] wrote:

quoted

Currently there exist approximately 14 000 places in the Kernel where
addresses are being printed using an unadorned %p. This potentially
leaks sensitive information regarding the Kernel layout in memory. Many
of these calls are stale, instead of fixing every call lets hash the
address by default before printing. This will of course break some
users, forcing code printing needed addresses to be updated. We can add
a printk specifier for this purpose (%px) to give developers a clear
upgrade path for breakages caused by applying this patch set.

The added advantage of hashing %p is that security is now opt-out, if
you _really_ want the address you have to work a little harder and use
%px.

The idea for creating the printk specifier %px to print the actual
address was suggested by Kees Cook (see below for email threads by
subject).

Maybe I'm being thick, but...  if we're rendering these addresses
unusable by hashing them, why not just print something like
"<obscured>" in their place?  That loses the uniqueness thing but I
wonder how valuable that is in practice?

The discussion on this has been fragmented over _at least_ 5 patch sets
with totally different subjects. And I only just added you to the CC
list, my apologies if this is a bit confusing.

Consensus was that if we provide a unique identifier (the hashed
address) then this is useful for debugging (i.e differentiating between
structs when you have a list of them).

The first 32 bits (on 64 bit machines) were zeroed out because

1. they are unnecessary in achieving the aim.
2. it reduces noise.
3. makes explicit some funny business was going on.

And bonus points, hopefully we don't break userland tools that parse
addresses if the format is still the same.

thanks,
Tobin.

On Thu, 2017-11-30 at 10:26 +1100, Tobin C. Harding wrote:

On Wed, Nov 29, 2017 at 03:20:58PM -0800, Andrew Morton wrote:

quoted

On Wed, 29 Nov 2017 13:05:04 +1100 "Tobin C. Harding" [off-list ref] wrote:

quoted

printk specifier %p now hashes all addresses before printing. Sometimes
we need to see the actual unmodified address. This can be achieved using
%lx but then we face the risk that if in future we want to change the
way the Kernel handles printing of pointers we will have to grep through
the already existent 50 000 %lx call sites. Let's add specifier %px as a
clear, opt-in, way to print a pointer and maintain some level of
isolation from all the other hex integer output within the Kernel.

Add printk specifier %px to print the actual unmodified address.

...

+Unmodified Addresses
+====================
+
+::
+
+	%px	01234567 or 0123456789abcdef
+
+For printing pointers when you _really_ want to print the address. Please
+consider whether or not you are leaking sensitive information about the
+Kernel layout in memory before printing pointers with %px. %px is
+functionally equivalent to %lx. %px is preferred to %lx because it is more
+uniquely grep'able. If, in the future, we need to modify the way the Kernel
+handles printing pointers it will be nice to be able to find the call
+sites.
+

You might want to add a checkpatch rule which emits a stern
do-you-really-want-to-do-this warning when someone uses %px.

Oh, nice idea. It has to be a CHECK but right?

No, it has to be something that's not --strict
so a WARN would probably be best.

By stern, you mean use stern language?

I hope he doesn't mean tweet.

Something like:
---
 scripts/checkpatch.pl | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 0ce249f157a1..9d789cbe7df5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl

@@ -5758,21 +5758,40 @@ sub process {
 		    defined $stat &&
 		    $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s &&
 		    $1 !~ /^_*volatile_*$/) {
+			my $complete_extension = "";
+			my $extension = "";
 			my $bad_extension = "";
 			my $lc = $stat =~ tr@\n@@;
 			$lc = $lc + $linenr;
+			my $stat_real;
 		        for (my $count = $linenr; $count <= $lc; $count++) {
 				my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0));
 				$fmt =~ s/%%//g;
-				if ($fmt =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGNO]).)/) {
-					$bad_extension = $1;
-					last;
+				while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) {
+					$complete_extension = $1;
+					$extension = $2;
+					if ($extension !~ /[FfSsBKRraEhMmIiUDdgVCbGNOx]/) {
+						$bad_extension = $complete_extension;
+						last;
+					}
+					if ($extension eq "x") {
+						if (!defined($stat_real)) {
+							$stat_real = raw_line($linenr, 0);
+							for (my $count = $linenr + 1; $count <= $lc; $count++) {
+								$stat_real = $stat_real . "\n" . raw_line($count, 0);
+							}
+						}
+						WARN("VSPRINTF_POINTER_PX",
+						     "Using vsprintf pointer extension '$complete_extension' exposes kernel address for possible hacking\n" . "$here\n$stat_real\n");
+					}
 				}
 			}
 			if ($bad_extension ne "") {
-				my $stat_real = raw_line($linenr, 0);
-				for (my $count = $linenr + 1; $count <= $lc; $count++) {
-					$stat_real = $stat_real . "\n" . raw_line($count, 0);
+				if (!defined($stat_real)) {
+					$stat_real = raw_line($linenr, 0);
+					for (my $count = $linenr + 1; $count <= $lc; $count++) {
+						$stat_real = $stat_real . "\n" . raw_line($count, 0);
+					}
 				}
 				WARN("VSPRINTF_POINTER_EXTENSION",
 				     "Invalid vsprintf pointer extension '$bad_extension'\n" . "$here\n$stat_real\n");

On Wed, Nov 29, 2017 at 07:58:26PM -0800, Joe Perches wrote:

On Thu, 2017-11-30 at 10:26 +1100, Tobin C. Harding wrote:

quoted

On Wed, Nov 29, 2017 at 03:20:58PM -0800, Andrew Morton wrote:

quoted

On Wed, 29 Nov 2017 13:05:04 +1100 "Tobin C. Harding" [off-list ref] wrote:

quoted

printk specifier %p now hashes all addresses before printing. Sometimes
we need to see the actual unmodified address. This can be achieved using
%lx but then we face the risk that if in future we want to change the
way the Kernel handles printing of pointers we will have to grep through
the already existent 50 000 %lx call sites. Let's add specifier %px as a
clear, opt-in, way to print a pointer and maintain some level of
isolation from all the other hex integer output within the Kernel.

Add printk specifier %px to print the actual unmodified address.

...

+Unmodified Addresses
+====================
+
+::
+
+	%px	01234567 or 0123456789abcdef
+
+For printing pointers when you _really_ want to print the address. Please
+consider whether or not you are leaking sensitive information about the
+Kernel layout in memory before printing pointers with %px. %px is
+functionally equivalent to %lx. %px is preferred to %lx because it is more
+uniquely grep'able. If, in the future, we need to modify the way the Kernel
+handles printing pointers it will be nice to be able to find the call
+sites.
+

You might want to add a checkpatch rule which emits a stern
do-you-really-want-to-do-this warning when someone uses %px.

Oh, nice idea. It has to be a CHECK but right?

No, it has to be something that's not --strict
so a WARN would probably be best.

quoted

By stern, you mean use stern language?

I hope he doesn't mean tweet.

/me says tweet tweet (like a bird)

Awesome. So moving forward, I should apply this code. Test it, commit it
with a log message stating you wrote it and I just tested it then submit
the patch, right?

thanks,
Tobin.

On Thu, 2017-11-30 at 15:18 +1100, Tobin C. Harding wrote:

On Wed, Nov 29, 2017 at 07:58:26PM -0800, Joe Perches wrote:

quoted

On Thu, 2017-11-30 at 10:26 +1100, Tobin C. Harding wrote:

quoted

On Wed, Nov 29, 2017 at 03:20:58PM -0800, Andrew Morton wrote:

quoted

On Wed, 29 Nov 2017 13:05:04 +1100 "Tobin C. Harding" [off-list ref] wrote:

quoted

printk specifier %p now hashes all addresses before printing. Sometimes
we need to see the actual unmodified address. This can be achieved using
%lx but then we face the risk that if in future we want to change the
way the Kernel handles printing of pointers we will have to grep through
the already existent 50 000 %lx call sites. Let's add specifier %px as a
clear, opt-in, way to print a pointer and maintain some level of
isolation from all the other hex integer output within the Kernel.

Add printk specifier %px to print the actual unmodified address.

...

+Unmodified Addresses
+====================
+
+::
+
+	%px	01234567 or 0123456789abcdef
+
+For printing pointers when you _really_ want to print the address. Please
+consider whether or not you are leaking sensitive information about the
+Kernel layout in memory before printing pointers with %px. %px is
+functionally equivalent to %lx. %px is preferred to %lx because it is more
+uniquely grep'able. If, in the future, we need to modify the way the Kernel
+handles printing pointers it will be nice to be able to find the call
+sites.
+

You might want to add a checkpatch rule which emits a stern
do-you-really-want-to-do-this warning when someone uses %px.

Oh, nice idea. It has to be a CHECK but right?

No, it has to be something that's not --strict
so a WARN would probably be best.

quoted

By stern, you mean use stern language?

I hope he doesn't mean tweet.

/me says tweet tweet (like a bird)

quoted

Something like:
---
 scripts/checkpatch.pl | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 0ce249f157a1..9d789cbe7df5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl

@@ -5758,21 +5758,40 @@ sub process {
 		    defined $stat &&
 		    $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s &&
 		    $1 !~ /^_*volatile_*$/) {
+			my $complete_extension = "";
+			my $extension = "";
 			my $bad_extension = "";
 			my $lc = $stat =~ tr@\n@@;
 			$lc = $lc + $linenr;
+			my $stat_real;
 		        for (my $count = $linenr; $count <= $lc; $count++) {
 				my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0));
 				$fmt =~ s/%%//g;
-				if ($fmt =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGNO]).)/) {
-					$bad_extension = $1;
-					last;
+				while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) {
+					$complete_extension = $1;
+					$extension = $2;
+					if ($extension !~ /[FfSsBKRraEhMmIiUDdgVCbGNOx]/) {
+						$bad_extension = $complete_extension;
+						last;
+					}
+					if ($extension eq "x") {
+						if (!defined($stat_real)) {
+							$stat_real = raw_line($linenr, 0);
+							for (my $count = $linenr + 1; $count <= $lc; $count++) {
+								$stat_real = $stat_real . "\n" . raw_line($count, 0);
+							}
+						}
+						WARN("VSPRINTF_POINTER_PX",
+						     "Using vsprintf pointer extension '$complete_extension' exposes kernel address for possible hacking\n" . "$here\n$stat_real\n");
+					}
 				}
 			}
 			if ($bad_extension ne "") {
-				my $stat_real = raw_line($linenr, 0);
-				for (my $count = $linenr + 1; $count <= $lc; $count++) {
-					$stat_real = $stat_real . "\n" . raw_line($count, 0);
+				if (!defined($stat_real)) {
+					$stat_real = raw_line($linenr, 0);
+					for (my $count = $linenr + 1; $count <= $lc; $count++) {
+						$stat_real = $stat_real . "\n" . raw_line($count, 0);
+					}
 				}
 				WARN("VSPRINTF_POINTER_EXTENSION",
 				     "Invalid vsprintf pointer extension '$bad_extension'\n" . "$here\n$stat_real\n");

Awesome. So moving forward, I should apply this code. Test it,

I didn't sign it and just trivially tested it.

So test it locally, see if it doesn't work
and check if the wording could be improved.

One possible negative is that if the format
contains multiple %px uses, then each use is
warned.

Maybe it should be
				if ($extension eq "x" && !defined($stat_real)) {
					...
					WARN("VSPRINTF_POINTER_PX", ...)
				}
so that only the first %px is warned.

If/when the %px series is applied, then this
can go in via whatever tree.

On Wed, Nov 29, 2017 at 08:41:36PM -0800, Joe Perches wrote:

On Thu, 2017-11-30 at 15:18 +1100, Tobin C. Harding wrote:

quoted

On Wed, Nov 29, 2017 at 07:58:26PM -0800, Joe Perches wrote:

quoted

On Thu, 2017-11-30 at 10:26 +1100, Tobin C. Harding wrote:

quoted

On Wed, Nov 29, 2017 at 03:20:58PM -0800, Andrew Morton wrote:

quoted

On Wed, 29 Nov 2017 13:05:04 +1100 "Tobin C. Harding" [off-list ref] wrote:

quoted

printk specifier %p now hashes all addresses before printing. Sometimes
we need to see the actual unmodified address. This can be achieved using
%lx but then we face the risk that if in future we want to change the
way the Kernel handles printing of pointers we will have to grep through
the already existent 50 000 %lx call sites. Let's add specifier %px as a
clear, opt-in, way to print a pointer and maintain some level of
isolation from all the other hex integer output within the Kernel.

Add printk specifier %px to print the actual unmodified address.

...

+Unmodified Addresses
+====================
+
+::
+
+	%px	01234567 or 0123456789abcdef
+
+For printing pointers when you _really_ want to print the address. Please
+consider whether or not you are leaking sensitive information about the
+Kernel layout in memory before printing pointers with %px. %px is
+functionally equivalent to %lx. %px is preferred to %lx because it is more
+uniquely grep'able. If, in the future, we need to modify the way the Kernel
+handles printing pointers it will be nice to be able to find the call
+sites.
+

You might want to add a checkpatch rule which emits a stern
do-you-really-want-to-do-this warning when someone uses %px.

Oh, nice idea. It has to be a CHECK but right?

No, it has to be something that's not --strict
so a WARN would probably be best.

quoted

By stern, you mean use stern language?

I hope he doesn't mean tweet.

/me says tweet tweet (like a bird)

quoted

Something like:
---
 scripts/checkpatch.pl | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 0ce249f157a1..9d789cbe7df5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl

@@ -5758,21 +5758,40 @@ sub process {
 		    defined $stat &&
 		    $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s &&
 		    $1 !~ /^_*volatile_*$/) {
+			my $complete_extension = "";
+			my $extension = "";
 			my $bad_extension = "";
 			my $lc = $stat =~ tr@\n@@;
 			$lc = $lc + $linenr;
+			my $stat_real;
 		        for (my $count = $linenr; $count <= $lc; $count++) {
 				my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0));
 				$fmt =~ s/%%//g;
-				if ($fmt =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGNO]).)/) {
-					$bad_extension = $1;
-					last;
+				while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) {
+					$complete_extension = $1;
+					$extension = $2;
+					if ($extension !~ /[FfSsBKRraEhMmIiUDdgVCbGNOx]/) {
+						$bad_extension = $complete_extension;
+						last;
+					}
+					if ($extension eq "x") {
+						if (!defined($stat_real)) {
+							$stat_real = raw_line($linenr, 0);
+							for (my $count = $linenr + 1; $count <= $lc; $count++) {
+								$stat_real = $stat_real . "\n" . raw_line($count, 0);
+							}
+						}
+						WARN("VSPRINTF_POINTER_PX",
+						     "Using vsprintf pointer extension '$complete_extension' exposes kernel address for possible hacking\n" . "$here\n$stat_real\n");
+					}
 				}
 			}
 			if ($bad_extension ne "") {
-				my $stat_real = raw_line($linenr, 0);
-				for (my $count = $linenr + 1; $count <= $lc; $count++) {
-					$stat_real = $stat_real . "\n" . raw_line($count, 0);
+				if (!defined($stat_real)) {
+					$stat_real = raw_line($linenr, 0);
+					for (my $count = $linenr + 1; $count <= $lc; $count++) {
+						$stat_real = $stat_real . "\n" . raw_line($count, 0);
+					}
 				}
 				WARN("VSPRINTF_POINTER_EXTENSION",
 				     "Invalid vsprintf pointer extension '$bad_extension'\n" . "$here\n$stat_real\n");

Awesome. So moving forward, I should apply this code. Test it,

I didn't sign it and just trivially tested it.

So test it locally, see if it doesn't work
and check if the wording could be improved.

One possible negative is that if the format
contains multiple %px uses, then each use is
warned.

Maybe it should be
				if ($extension eq "x" && !defined($stat_real)) {
					...
					WARN("VSPRINTF_POINTER_PX", ...)
				}
so that only the first %px is warned.

Ok, will do as suggested.

If/when the %px series is applied, then this
can go in via whatever tree.

The %px series is in Linus' mainline now. I'll get this stuff to you and
Andy for ack'ing (and LKML) soon as its done.

thanks,
Tobin.

From: Andrew Morton

Sent: 29 November 2017 23:21

quoted

The added advantage of hashing %p is that security is now opt-out, if
you _really_ want the address you have to work a little harder and use
%px.

You need a system-wide opt-out that prints the actual values.
Otherwise developers will use something else to print addresses and
the code will remain in the released drivers.

quoted

The idea for creating the printk specifier %px to print the actual
address was suggested by Kees Cook (see below for email threads by
subject).

Maybe I'm being thick, but...  if we're rendering these addresses
unusable by hashing them, why not just print something like
"<obscured>" in their place?  That loses the uniqueness thing but I
wonder how valuable that is in practice?

My worry is that is you get a kernel 'oops' print with actual register
values you have no easy way of tying an address or address+offset to
the corresponding hash(address) printed elsewhere.

	David

On (11/30/17 10:23), David Laight wrote:
[..]

quoted

Maybe I'm being thick, but...  if we're rendering these addresses
unusable by hashing them, why not just print something like
"<obscured>" in their place?  That loses the uniqueness thing but I
wonder how valuable that is in practice?

My worry is that is you get a kernel 'oops' print with actual register
values you have no easy way of tying an address or address+offset to
the corresponding hash(address) printed elsewhere.

print the existing hash:pointer mappings in panic()? [if we can do that]

	-ss

From: Kees Cook

Sent: 29 November 2017 22:28
On Wed, Nov 29, 2017 at 2:07 AM, David Laight [off-list ref] wrote:

quoted

From: Linus Torvalds

quoted

Sent: 29 November 2017 02:29

On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding [off-list ref] wrote:

quoted

   Let's add specifier %px as a
clear, opt-in, way to print a pointer and maintain some level of
isolation from all the other hex integer output within the Kernel.

Yes, I like this model. It's easy and it's obvious ("'x' for hex"),
and it gives people a good way to say "yes, I really want the actual
address as hex" for if/when the hashed pointer doesn't work for some
reason.

Remind me to change every %p to %px on kernels that support it.

Although the absolute values of pointers may not be useful, knowing
that two pointer differ by a small amount is useful.
It is also useful to know whether pointers are to stack, code, static
data or heap.

This change to %p is going to make debugging a nightmare.

In the future, maybe we could have a knob: unhashed, hashed (default),
or zeroed.

Add a 4th, hashed_page+offset.

Isn't there already a knob for %pK, bits in the same value could be used.
That would make it easy to ensure that %pK is more restructive than %p.

	David

On (11/30/17 19:26), Sergey Senozhatsky wrote:

On (11/30/17 10:23), David Laight wrote:
[..]

quoted

quoted

Maybe I'm being thick, but...  if we're rendering these addresses
unusable by hashing them, why not just print something like
"<obscured>" in their place?  That loses the uniqueness thing but I
wonder how valuable that is in practice?

My worry is that is you get a kernel 'oops' print with actual register
values you have no easy way of tying an address or address+offset to
the corresponding hash(address) printed elsewhere.

print the existing hash:pointer mappings in panic()? [if we can do that]

by this I meant
	"when oops_in_progress == 1 then print hash:pointer for %p,
	 not just hash".

	-ss

Hi Tobin,

On Wed, Nov 29, 2017 at 3:05 AM, Tobin C. Harding [off-list ref] wrote:

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

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

+/* Maps a pointer to a 32 bit unique identifier. */
+static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
+{
+       unsigned long hashval;
+       const int default_width = 2 * sizeof(ptr);
+
+       if (unlikely(!have_filled_random_ptr_key)) {
+               spec.field_width = default_width;
+               /* string length must be less than default_width */
+               return string(buf, end, "(ptrval)", spec);
+       }
+
+#ifdef CONFIG_64BIT
+       hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
+       /*
+        * Mask off the first 32 bits, this makes explicit that we have
+        * modified the address (and 32 bits is plenty for a unique ID).
+        */
+       hashval = hashval & 0xffffffff;
+#else
+       hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
+#endif

Would it make sense to keep the 3 lowest bits of the address?

Currently printed pointers no longer have any correlation with the actual
alignment in memory of the object, which is a typical cause of a class of bugs.

Gr{oetje,eeting}s,

                        Geert

From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Tue, 5 Dec 2017 21:20:57 +0100

Hi Tobin,

On Wed, Nov 29, 2017 at 3:05 AM, Tobin C. Harding [off-list ref] wrote:

quoted

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

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

quoted

--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c

quoted

+/* Maps a pointer to a 32 bit unique identifier. */
+static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
+{
+       unsigned long hashval;
+       const int default_width = 2 * sizeof(ptr);
+
+       if (unlikely(!have_filled_random_ptr_key)) {
+               spec.field_width = default_width;
+               /* string length must be less than default_width */
+               return string(buf, end, "(ptrval)", spec);
+       }
+
+#ifdef CONFIG_64BIT
+       hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
+       /*
+        * Mask off the first 32 bits, this makes explicit that we have
+        * modified the address (and 32 bits is plenty for a unique ID).
+        */
+       hashval = hashval & 0xffffffff;
+#else
+       hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
+#endif

Would it make sense to keep the 3 lowest bits of the address?

Currently printed pointers no longer have any correlation with the actual
alignment in memory of the object, which is a typical cause of a class of bugs.

Yeah, this is driving people nuts who wonder why pointers are aligned
all weird now.

On Tue, Dec 05, 2017 at 09:20:57PM +0100, Geert Uytterhoeven wrote:

Hi Tobin,

On Wed, Nov 29, 2017 at 3:05 AM, Tobin C. Harding [off-list ref] wrote:

quoted

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

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

quoted

--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c

quoted

+/* Maps a pointer to a 32 bit unique identifier. */
+static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
+{
+       unsigned long hashval;
+       const int default_width = 2 * sizeof(ptr);
+
+       if (unlikely(!have_filled_random_ptr_key)) {
+               spec.field_width = default_width;
+               /* string length must be less than default_width */
+               return string(buf, end, "(ptrval)", spec);
+       }
+
+#ifdef CONFIG_64BIT
+       hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
+       /*
+        * Mask off the first 32 bits, this makes explicit that we have
+        * modified the address (and 32 bits is plenty for a unique ID).
+        */
+       hashval = hashval & 0xffffffff;
+#else
+       hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
+#endif

Would it make sense to keep the 3 lowest bits of the address?

Currently printed pointers no longer have any correlation with the actual
alignment in memory of the object, which is a typical cause of a class of bugs.

We'd have to keep the lowest 4 since we are printing in hex, right? This
is easy enough to add. I wasn't the architect behind the hashing but I
can do up a patch and see if anyone who knows crypto objects.

thanks,
Tobin.

On 11/30/2017 02:38 AM, David Laight wrote:

From: Kees Cook

quoted

Sent: 29 November 2017 22:28
On Wed, Nov 29, 2017 at 2:07 AM, David Laight [off-list ref] wrote:

quoted

From: Linus Torvalds

quoted

Sent: 29 November 2017 02:29

On Tue, Nov 28, 2017 at 6:05 PM, Tobin C. Harding [off-list ref] wrote:

quoted

   Let's add specifier %px as a
clear, opt-in, way to print a pointer and maintain some level of
isolation from all the other hex integer output within the Kernel.

Yes, I like this model. It's easy and it's obvious ("'x' for hex"),
and it gives people a good way to say "yes, I really want the actual
address as hex" for if/when the hashed pointer doesn't work for some
reason.

Remind me to change every %p to %px on kernels that support it.

Although the absolute values of pointers may not be useful, knowing
that two pointer differ by a small amount is useful.
It is also useful to know whether pointers are to stack, code, static
data or heap.

This change to %p is going to make debugging a nightmare.

In the future, maybe we could have a knob: unhashed, hashed (default),
or zeroed.

Add a 4th, hashed_page+offset.

Isn't there already a knob for %pK, bits in the same value could be used.
That would make it easy to ensure that %pK is more restructive than %p.

(yeah, I'm kind of behind on this thread.)

This kind of option (with default hashed) is what I was just thinking of
after having seen a few unhelpful traces.  But then the knob might not be
changed in time for the traces either. :(


-- 
~Randy

On Tue, Dec 5, 2017 at 1:08 PM, Randy Dunlap [off-list ref] wrote:

This kind of option (with default hashed) is what I was just thinking of
after having seen a few unhelpful traces.  But then the knob might not be
changed in time for the traces either. :(

.. I really dislike the idea of such a knob.

First off, the traces I've seen that had the new %p behavior, the
hashing didn't actually matter AT ALL. The only values that were
hashed were values that weren't actually useful for debugging the
oops.

Secondly, the notion that "we want a unhashed knob for debugging" is
exactly the wrong kind of mentality. 99% of all bug reports happen in
the wild - not on developer boxes. So by default, those bug reports
had better happen with hashing enabled, or it's all entirely
pointless.

If you have an oops that happens on your own box due to code that
you're writing yourself (and expect to debug yourself), then honestly,
the hashing is going to be the least of your issues. If you can't find
out the bug under those circumstances, and you're confused by the tiny
detail of hashing, you're doing something wrong.

So the case that matters is when an oops comes from some outside
source that won't have turned the knob off anyway.

So no. We're not adding a knob. It is fundamentally pointless.

It's not like those hex numbers were really helping people anyway.
We've turned off most of them on x86 oops reports long ago (and
entirely independently of the pointer hashing). Having stared at a lot
of oopses in my time, the only hex numbers that tend to be really
relevant are (a) the register contents (which aren't %p anyway), and
things like the faulting address (which is not, and never has been, %p
on x86, but might be on some other architecture).

Honestly, the next time anybody says "hashing makes debugging harder",
I'm going to require some actual proof of an actual oops where it
mattered that a particular value was hashed.

Not hand-waving.

Not "it surprised and confused me" because it looked different. You'll
get used to it.

So an actual "this was critical information that mattered for this
particular bug, and it was missing due to the hashing of this
particular value and debugging was harder in actual reality due to
that".

Because the actual example I have seen so far, not only didn't the
hashing matter AT ALL, most of the _unhashed_ values shouldn't have
been there either, and were due to arm still printing stuff that
shouldn't have been printed at all and just made the oops more complex
and harder to read and report.

            Linus

Hi Tobin,

On Tue, Dec 5, 2017 at 9:44 PM, Tobin C. Harding [off-list ref] wrote:

On Tue, Dec 05, 2017 at 09:20:57PM +0100, Geert Uytterhoeven wrote:

quoted

On Wed, Nov 29, 2017 at 3:05 AM, Tobin C. Harding [off-list ref] wrote:

quoted

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

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

quoted

--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c

quoted

+/* Maps a pointer to a 32 bit unique identifier. */
+static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
+{
+       unsigned long hashval;
+       const int default_width = 2 * sizeof(ptr);
+
+       if (unlikely(!have_filled_random_ptr_key)) {
+               spec.field_width = default_width;
+               /* string length must be less than default_width */
+               return string(buf, end, "(ptrval)", spec);
+       }
+
+#ifdef CONFIG_64BIT
+       hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
+       /*
+        * Mask off the first 32 bits, this makes explicit that we have
+        * modified the address (and 32 bits is plenty for a unique ID).
+        */
+       hashval = hashval & 0xffffffff;
+#else
+       hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key);
+#endif

Would it make sense to keep the 3 lowest bits of the address?

Currently printed pointers no longer have any correlation with the actual
alignment in memory of the object, which is a typical cause of a class of bugs.

We'd have to keep the lowest 4 since we are printing in hex, right? This
is easy enough to add. I wasn't the architect behind the hashing but I
can do up a patch and see if anyone who knows crypto objects.

Lowest 3 is good enough for all natural types, up to long long.
We may still receive complaints from people who care about seeing if
a pointer is cacheline-aligned or not. Fixing that may need up to 7 bits, I'm
afraid, which is a bit too much to give up.

Gr{oetje,eeting}s,

                        Geert

On Tue, Dec 5, 2017 at 2:57 PM, Geert Uytterhoeven [off-list ref] wrote:

Lowest 3 is good enough for all natural types, up to long long.
We may still receive complaints from people who care about seeing if
a pointer is cacheline-aligned or not. Fixing that may need up to 7 bits, I'm
afraid, which is a bit too much to give up.

I really think even the lowest three is a bit too much.

Who really cares? If it's something that is particularly _about_
alignment (ie an alignment trap), maybe just print out those bits
separately.

And for everything else? It's purely about getting used to it.

I will just cut-and-paste what I wrote in another thread about the hashing:

  I'm going to require some actual proof of an actual case where it
  mattered that a particular value was hashed.

  Not hand-waving.

  Not "it surprised and confused me" because it looked different. You'll
  get used to it.

  So an actual "this was critical information that mattered for this
  particular bug, and it was missing due to the hashing of this
  particular value and debugging was harder in actual reality due to
  that".

  Because the actual example I have seen so far, not only didn't the
  hashing matter AT ALL, most of the _unhashed_ values shouldn't have
  been there either, and were due to arm still printing stuff that
  shouldn't have been printed at all and just made the oops more complex
  and harder to read and report.

This subject is really easy to bike-shed around. Everybody can have an
opinion. I want actual hard data and facts, not opinions.

And if the hashing _really_ is a problem, we'll just change that
particular thing to %px. But it needs actual hard data and real
reasons first, ok?

              Linus

          Linus

Hello,

On (12/05/17 13:22), Linus Torvalds wrote:
[..]

It's not like those hex numbers were really helping people anyway.
We've turned off most of them on x86 oops reports long ago (and
entirely independently of the pointer hashing). Having stared at a lot
of oopses in my time, the only hex numbers that tend to be really
relevant are (a) the register contents (which aren't %p anyway), and
things like the faulting address (which is not, and never has been, %p
on x86, but might be on some other architecture).

I see some %p-s being used in _supposedly_ important output,
like arch/x86/mm/fault.c

show_fault_oops(struct pt_regs *regs, unsigned long error_code,
		unsigned long address)
...
	printk(KERN_CONT " at %p\n", (void *) address);
	printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip);


a quick %p grep gives me the following list:

arch/arm/mm/fault.c:    pr_alert("pgd = %p\n", mm->pgd);
arch/arm64/mm/fault.c:  pr_alert("%s pgtable: %luk pages, %u-bit VAs, pgd = %p\n",
arch/arm64/mm/fault.c:          pr_info_ratelimited("%s[%d]: %s exception: pc=%p sp=%p\n",
arch/m68k/mm/fault.c:   pr_debug("send_fault_sig: %p,%d,%d\n", siginfo.si_addr,
arch/m68k/mm/fault.c:           pr_cont(" at virtual address %p\n", siginfo.si_addr);
arch/m68k/mm/fault.c:   pr_debug("do page fault:\nregs->sr=%#x, regs->pc=%#lx, address=%#lx, %ld, %p\n",
arch/microblaze/mm/fault.c:             pr_emerg("Page fault in user mode with faulthandler_disabled(), mm = %p\n",
arch/mn10300/mm/fault.c:        printk(KERN_DEBUG "pgd entry %p: %016Lx\n",
arch/mn10300/mm/fault.c:        printk(KERN_DEBUG "pmd entry %p: %016Lx\n",
arch/mn10300/mm/fault.c:        printk(KERN_DEBUG "pte entry %p: %016Lx\n",
arch/mn10300/mm/fault.c:        printk(KERN_DEBUG "--- do_page_fault(%p,%s:%04lx,%08lx)\n",
arch/powerpc/mm/fault.c:                                           " mm=%p\n",
arch/sh/mm/fault.c:     printk(KERN_ALERT "pgd = %p\n", pgd);
arch/unicore32/mm/fault.c:      printk(KERN_ALERT "pgd = %p\n", mm->pgd);
arch/x86/mm/fault.c:    printk(KERN_CONT " at %p\n", (void *) address);
arch/x86/mm/fault.c:    printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip);
arch/x86/mm/fault.c:    printk("%s%s[%d]: segfault at %lx ip %p sp %p error %lx",


or is it OK to show hashes instead of pgd or pmd pointers?

	-ss

On Tue, Dec 5, 2017 at 5:36 PM, Sergey Senozhatsky
[off-list ref] wrote:

I see some %p-s being used in _supposedly_ important output,
like arch/x86/mm/fault.c

show_fault_oops(struct pt_regs *regs, unsigned long error_code,
                unsigned long address)
...
        printk(KERN_CONT " at %p\n", (void *) address);
        printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip);

So %pS isn't %p, and shows the symbolic name.

But yes, that "at %p" should definitely be %px.

In fact, it used to be a "%08lx" - and the value we print out is
"unsigned long - but then when we unified the 32- and 64-bit
architectures, using "%p" and a cast was a convenient way to unify the
32-bit %08lx and the 16-bit %016lx formats.

Will fix.

a quick %p grep gives me the following list:

...

or is it OK to show hashes instead of pgd or pmd pointers?

So my gut feel is that those printouts should probably just be
removed. They have some very old historical reasons: we've printed out
the page directory pointers (and followed the page tables) since at
least back in the 1.1.x days. This is from the 1.1.7 patch, back when
mm/memory.c was all about x86:

+       printk(KERN_ALERT "current->tss.cr3 = %08lx, %%cr3 = %08lx\n",
+               current->tss.cr3, user_esp);
+       user_esp = ((unsigned long *) user_esp)[address >> 22];
+       printk(KERN_ALERT "*pde = %08lx\n", user_esp);

so it's more historical than sensible, I think.

               Linus

On (12/05/17 17:59), Linus Torvalds wrote:
[..]

On Tue, Dec 5, 2017 at 5:36 PM, Sergey Senozhatsky
[off-list ref] wrote:

quoted

I see some %p-s being used in _supposedly_ important output,
like arch/x86/mm/fault.c

show_fault_oops(struct pt_regs *regs, unsigned long error_code,
                unsigned long address)
...
        printk(KERN_CONT " at %p\n", (void *) address);
        printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip);

So %pS isn't %p, and shows the symbolic name.

sure, agreed. by "some %p-s being used" I meant the grep result,
not just x86 show_fault_oops().

But yes, that "at %p" should definitely be %px.

more %p grepping [filtering out all `%ps %pf %pb' variants] gives
a huge number of print outs that potentially can be broken now

arch/x86/kernel/kprobes/core.c:		printk(KERN_WARNING "Unrecoverable kprobe detected at %p.\n",
arch/x86/kernel/kprobes/core.c:			       "current sp %p does not match saved sp %p\n",
arch/x86/kernel/kprobes/core.c:			printk(KERN_ERR "Saved registers for jprobe %p\n", jp);

arch/x86/kernel/head_32.S:	.asciz "Unknown interrupt or fault at: %p %p %p\n"
arch/x86/kernel/irq_32.c:	printk(KERN_DEBUG "CPU %u irqstacks, hard=%p soft=%p\n",

arch/x86/kernel/smpboot.c:	pr_debug("Stack at about %p\n", &cpuid);
arch/x86/kernel/traps.c:	printk(KERN_EMERG "BUG: stack guard page was hit at %p (stack is %p..%p)\n",


so I'm not in position to suggest the removal of those print outs or to
decide if those are important at all, just saying that that "I'm confused
by pointer values and can't debug" might be more likely that we thought.

So my gut feel is that those printouts should probably just be
removed. They have some very old historical reasons: we've printed out
the page directory pointers (and followed the page tables) since at
least back in the 1.1.x days. This is from the 1.1.7 patch, back when
mm/memory.c was all about x86:

I see, thanks.

	-ss

Hi Linus,

On Wed, Dec 6, 2017 at 2:59 AM, Linus Torvalds
[off-list ref] wrote:

On Tue, Dec 5, 2017 at 5:36 PM, Sergey Senozhatsky
[off-list ref] wrote:

quoted

I see some %p-s being used in _supposedly_ important output,
like arch/x86/mm/fault.c

show_fault_oops(struct pt_regs *regs, unsigned long error_code,
                unsigned long address)
...
        printk(KERN_CONT " at %p\n", (void *) address);
        printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip);

So %pS isn't %p, and shows the symbolic name.

If the symbolic name is available.
Else it prints the non-hashed pointer value (FTR).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

On (12/06/17 09:32), Geert Uytterhoeven wrote:
[..]

quoted

quoted

show_fault_oops(struct pt_regs *regs, unsigned long error_code,
                unsigned long address)
...
        printk(KERN_CONT " at %p\n", (void *) address);
        printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip);

So %pS isn't %p, and shows the symbolic name.

If the symbolic name is available.
Else it prints the non-hashed pointer value (FTR).

hm, indeed. and !CONFIG_KALLSYMS config turns %pS/%ps
into special_hex_number().

	-ss

Hi Linus,

On Wed, Dec 6, 2017 at 12:33 AM, Linus Torvalds
[off-list ref] wrote:

On Tue, Dec 5, 2017 at 2:57 PM, Geert Uytterhoeven [off-list ref] wrote:

quoted

Lowest 3 is good enough for all natural types, up to long long.
We may still receive complaints from people who care about seeing if
a pointer is cacheline-aligned or not. Fixing that may need up to 7 bits, I'm
afraid, which is a bit too much to give up.

I really think even the lowest three is a bit too much.

Who really cares? If it's something that is particularly _about_
alignment (ie an alignment trap), maybe just print out those bits
separately.

If I'm not mistaken, some architectures don't generate alignment traps, but
just zero the LSBs.

And for everything else? It's purely about getting used to it.

Yes, we will get used to it.

I agree that for debugging during development, we will just use %px and be
fine.

Storm in a glass of water, everybody will have forgotten by the time v4.16 is
released.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

From: David Miller

Sent: 05 December 2017 20:31

...

quoted

Would it make sense to keep the 3 lowest bits of the address?

Currently printed pointers no longer have any correlation with the actual
alignment in memory of the object, which is a typical cause of a class of bugs.

Yeah, this is driving people nuts who wonder why pointers are aligned
all weird now.

I can also image issues where you want to know whether 2 pointers point
into the same structure (like an skb).
Useful if you suspect that code is using a stale pointer because the
skb got reallocated by some internal function.
I'm not sure such pointers are printed by default though.

I know I have debugged things that required hexdumps of memory areas
referenced by traced pointers.
I won't have done that for anything written with the kernel printf because
getting any more info for a linux kernel oops is actually quite hard.

	David

On Wed, Dec 6, 2017 at 2:31 AM, David Laight [off-list ref] wrote:

From: David Miller

quoted

Sent: 05 December 2017 20:31

...

quoted

quoted

Would it make sense to keep the 3 lowest bits of the address?

Currently printed pointers no longer have any correlation with the actual
alignment in memory of the object, which is a typical cause of a class of bugs.

Yeah, this is driving people nuts who wonder why pointers are aligned
all weird now.

I can also image issues where you want to know whether 2 pointers point
into the same structure (like an skb).

This is already retained due to the hashing. i.e. the same pointer
value will produce the same hash value, so that kind of direct
comparison still works.

Useful if you suspect that code is using a stale pointer because the
skb got reallocated by some internal function.
I'm not sure such pointers are printed by default though.

I hope not. :) skb used to be exported for INET_DIAG, but that got
fixed a while back.

-Kees

-- 
Kees Cook
Pixel Security

On Wed, Dec 6, 2017 at 3:21 PM, Kees Cook [off-list ref] wrote:

On Wed, Dec 6, 2017 at 2:31 AM, David Laight [off-list ref] wrote:

quoted

I can also image issues where you want to know whether 2 pointers point
into the same structure (like an skb).

This is already retained due to the hashing. i.e. the same pointer
value will produce the same hash value, so that kind of direct
comparison still works.

DavidL isn't talking about the _same_ pointer, he's talking about it
being offset from the same base.

But realistically, I don't think we ever hash things like that anyway.

Things like that do show up in register dumps etc during oopses, but
those aren't hashed.

            Linus

On Wed, Dec 06, 2017 at 09:32:14AM +0100, Geert Uytterhoeven wrote:

Hi Linus,

On Wed, Dec 6, 2017 at 2:59 AM, Linus Torvalds
[off-list ref] wrote:

quoted

On Tue, Dec 5, 2017 at 5:36 PM, Sergey Senozhatsky
[off-list ref] wrote:

quoted

I see some %p-s being used in _supposedly_ important output,
like arch/x86/mm/fault.c

show_fault_oops(struct pt_regs *regs, unsigned long error_code,
                unsigned long address)
...
        printk(KERN_CONT " at %p\n", (void *) address);
        printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip);

So %pS isn't %p, and shows the symbolic name.

If the symbolic name is available.
Else it prints the non-hashed pointer value (FTR).

I'm trying to fix this :)

[RFC 0/3] kallsyms: don't leak address when printing symbol

thanks,
Tobin.

On Wed, Dec 06, 2017 at 05:45:37PM +0900, Sergey Senozhatsky wrote:

On (12/06/17 09:32), Geert Uytterhoeven wrote:
[..]

quoted

quoted

quoted

show_fault_oops(struct pt_regs *regs, unsigned long error_code,
                unsigned long address)
...
        printk(KERN_CONT " at %p\n", (void *) address);
        printk(KERN_ALERT "IP: %pS\n", (void *)regs->ip);

So %pS isn't %p, and shows the symbolic name.

If the symbolic name is available.
Else it prints the non-hashed pointer value (FTR).

Well, this

[RFC 0/3] kallsyms: don't leak address when printing symbol

_trys_ to fix that

hm, indeed. and !CONFIG_KALLSYMS config turns %pS/%ps
into special_hex_number().

But totally misses this :(

"<no-sym>" would be better returned when !CONFIG_KALLSYMS, right?

thanks,
Tobin.

On Dec 6, 2017 21:17, "Tobin C. Harding" [off-list ref] wrote:

hm, indeed. and !CONFIG_KALLSYMS config turns %pS/%ps
into special_hex_number().

But totally misses this :(

"<no-sym>" would be better returned when !CONFIG_KALLSYMS, right?


No, we still want those configurations to be debuggable.

If !CONFIG_KALLSYMS, we'll just continue print the hex number. It's not a
normal configuration, leaking an address there is fine, and preferable to
"cannot debug"

   Linus

On (12/07/17 16:17), Tobin C. Harding wrote:
[..]

quoted

hm, indeed. and !CONFIG_KALLSYMS config turns %pS/%ps
into special_hex_number().

But totally misses this :(

"<no-sym>" would be better returned when !CONFIG_KALLSYMS, right?

I guess I'll take back my comment.

I assume there are tons of embedded devices that have !CONFIG_KALLSYMS
in 'release' builds, yet those devices still warn/oops sometimes; having
pointers/hex numbers is really the only way to make any sense out of
backtraces... yet it, basically, means that we are leaking kernel pointers.

	-ss