[PATCH v2 0/3] lib/vsprintf: Hash remaining raw addresses

[PATCH v2 0/3] lib/vsprintf: Hash remaining raw addresses

[Geert Uytterhoeven]

	Hi all,

There are still two format specifiers that print unhanced kernel
addresses, potentially leaking sensitive information regarding the
kernel layout in memory.

This patch series fixes this by printing hashed addresses instead.
    
Changes compared to v1:
  - Get rid of the forward declaration for ptr_to_id(),
  - Add Reviewed-by.

Thanks!

Geert Uytterhoeven (3):
  lib/vsprintf: Prepare for more general use of ptr_to_id()
  lib/vsprintf: Hash legacy clock addresses
  lib/vsprintf: Hash printed address for netdev bits fallback

 Documentation/core-api/printk-formats.rst |   5 +-
 lib/vsprintf.c                            | 216 +++++++++++-----------
 2 files changed, 110 insertions(+), 111 deletions(-)

-- 
2.17.1

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

[PATCH v2 1/3] lib/vsprintf: Prepare for more general use of ptr_to_id()

[Geert Uytterhoeven]

  - Make the ptr argument const, to avoid adding casts in future
    callers,
  - Move the function and its dependencies up, so it can be called from
    special pointer type formatting routines.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Get rid of the forward declaration for ptr_to_id().
---
 lib/vsprintf.c | 205 +++++++++++++++++++++++++------------------------
 1 file changed, 103 insertions(+), 102 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 095a677f89c02442..b4e4ab274bde2397 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -612,6 +612,109 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 	return widen_string(buf, len, 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);
+}
+
+/* Make pointers available for printing early in the boot sequence. */
+static int debug_boot_weak_hash __ro_after_init;
+
+static int __init debug_boot_weak_hash_enable(char *str)
+{
+	debug_boot_weak_hash = 1;
+	pr_info("debug_boot_weak_hash enabled\n");
+	return 0;
+}
+early_param("debug_boot_weak_hash", debug_boot_weak_hash_enable);
+
+static DEFINE_STATIC_KEY_TRUE(not_filled_random_ptr_key);
+static siphash_key_t ptr_key __read_mostly;
+
+static void enable_ptr_key_workfn(struct work_struct *work)
+{
+	get_random_bytes(&ptr_key, sizeof(ptr_key));
+	/* Needs to run from preemptible context */
+	static_branch_disable(&not_filled_random_ptr_key);
+}
+
+static DECLARE_WORK(enable_ptr_key_work, enable_ptr_key_workfn);
+
+static void fill_random_ptr_key(struct random_ready_callback *unused)
+{
+	/* This may be in an interrupt handler. */
+	queue_work(system_unbound_wq, &enable_ptr_key_work);
+}
+
+static struct random_ready_callback random_ready = {
+	.func = fill_random_ptr_key
+};
+
+static int __init initialize_ptr_random(void)
+{
+	int key_size = sizeof(ptr_key);
+	int ret;
+
+	/* Use hw RNG if available. */
+	if (get_random_bytes_arch(&ptr_key, key_size) == key_size) {
+		static_branch_disable(&not_filled_random_ptr_key);
+		return 0;
+	}
+
+	ret = add_random_ready_callback(&random_ready);
+	if (!ret) {
+		return 0;
+	} else if (ret == -EALREADY) {
+		/* This is in preemptible context */
+		enable_ptr_key_workfn(&enable_ptr_key_work);
+		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, const void *ptr,
+		       struct printf_spec spec)
+{
+	const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
+	unsigned long hashval;
+
+	/* When debugging early boot use non-cryptographically secure hash. */
+	if (unlikely(debug_boot_weak_hash)) {
+		hashval = hash_long((unsigned long)ptr, 32);
+		return pointer_string(buf, end, (const void *)hashval, spec);
+	}
+
+	if (static_branch_unlikely(&not_filled_random_ptr_key)) {
+		spec.field_width = 2 * sizeof(ptr);
+		/* string length must be less than default_width */
+		return string(buf, end, str, 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
+	return pointer_string(buf, end, (const void *)hashval, spec);
+}
+
 static noinline_for_stack
 char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_spec spec,
 		  const char *fmt)
@@ -1357,20 +1460,6 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 	return string(buf, end, uuid, 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);
-}
-
 int kptr_restrict __read_mostly;
 
 static noinline_for_stack
@@ -1656,94 +1745,6 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
-/* Make pointers available for printing early in the boot sequence. */
-static int debug_boot_weak_hash __ro_after_init;
-
-static int __init debug_boot_weak_hash_enable(char *str)
-{
-	debug_boot_weak_hash = 1;
-	pr_info("debug_boot_weak_hash enabled\n");
-	return 0;
-}
-early_param("debug_boot_weak_hash", debug_boot_weak_hash_enable);
-
-static DEFINE_STATIC_KEY_TRUE(not_filled_random_ptr_key);
-static siphash_key_t ptr_key __read_mostly;
-
-static void enable_ptr_key_workfn(struct work_struct *work)
-{
-	get_random_bytes(&ptr_key, sizeof(ptr_key));
-	/* Needs to run from preemptible context */
-	static_branch_disable(&not_filled_random_ptr_key);
-}
-
-static DECLARE_WORK(enable_ptr_key_work, enable_ptr_key_workfn);
-
-static void fill_random_ptr_key(struct random_ready_callback *unused)
-{
-	/* This may be in an interrupt handler. */
-	queue_work(system_unbound_wq, &enable_ptr_key_work);
-}
-
-static struct random_ready_callback random_ready = {
-	.func = fill_random_ptr_key
-};
-
-static int __init initialize_ptr_random(void)
-{
-	int key_size = sizeof(ptr_key);
-	int ret;
-
-	/* Use hw RNG if available. */
-	if (get_random_bytes_arch(&ptr_key, key_size) == key_size) {
-		static_branch_disable(&not_filled_random_ptr_key);
-		return 0;
-	}
-
-	ret = add_random_ready_callback(&random_ready);
-	if (!ret) {
-		return 0;
-	} else if (ret == -EALREADY) {
-		/* This is in preemptible context */
-		enable_ptr_key_workfn(&enable_ptr_key_work);
-		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)
-{
-	const char *str = sizeof(ptr) == 8 ? "(____ptrval____)" : "(ptrval)";
-	unsigned long hashval;
-
-	/* When debugging early boot use non-cryptographically secure hash. */
-	if (unlikely(debug_boot_weak_hash)) {
-		hashval = hash_long((unsigned long)ptr, 32);
-		return pointer_string(buf, end, (const void *)hashval, spec);
-	}
-
-	if (static_branch_unlikely(&not_filled_random_ptr_key)) {
-		spec.field_width = 2 * sizeof(ptr);
-		/* string length must be less than default_width */
-		return string(buf, end, str, 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
-	return pointer_string(buf, end, (const void *)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
-- 
2.17.1

[PATCH v2 2/3] lib/vsprintf: Hash legacy clock addresses

[Geert Uytterhoeven]

On platforms using the Common Clock Framework, "%pC" prints the clock's
name. On legacy platforms, it prints the unhashed clock's address,
potentially leaking sensitive information regarding the kernel layout in
memory.

Avoid this leak by printing the hashed address instead.  To distinguish
between clocks, a 32-bit unique identifier is as good as an actual
pointer value.

Fixes: ad67b74d2469d9b8 ("printk: hash addresses printed with %p")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
v2:
  - Add Reviewed-by.
---
 Documentation/core-api/printk-formats.rst | 5 ++---
 lib/vsprintf.c                            | 2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 25dc591cb1108790..d39798c2558500d5 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -420,9 +420,8 @@ struct clk
 	%pC	pll1
 	%pCn	pll1
 
-For printing struct clk structures. %pC and %pCn print the name
-(Common Clock Framework) or address (legacy clock framework) of the
-structure.
+For printing struct clk structures. %pC and %pCn print the name of the clock
+(Common Clock Framework) or a unique 32-bit ID (legacy clock framework).
 
 Passed by reference.
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b4e4ab274bde2397..3ea2119b85ac2e4f 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1563,7 +1563,7 @@ char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec,
 #ifdef CONFIG_COMMON_CLK
 		return string(buf, end, __clk_get_name(clk), spec);
 #else
-		return special_hex_number(buf, end, (unsigned long)clk, sizeof(unsigned long));
+		return ptr_to_id(buf, end, clk, spec);
 #endif
 	}
 }
-- 
2.17.1

[PATCH v2 3/3] lib/vsprintf: Hash printed address for netdev bits fallback

[Geert Uytterhoeven]

The handler for "%pN" falls back to printing the raw pointer value when
using a different format than the (sole supported) special format
"%pNF", potentially leaking sensitive information regarding the kernel
layout in memory.

Avoid this leak by printing the hashed address instead.
Note that there are no in-tree users of the fallback.

Fixes: ad67b74d2469d9b8 ("printk: hash addresses printed with %p")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
v2:
  - Add Reviewed-by.
---
 lib/vsprintf.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3ea2119b85ac2e4f..4c75d847c1453ca4 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1510,7 +1510,8 @@ char *restricted_pointer(char *buf, char *end, const void *ptr,
 }
 
 static noinline_for_stack
-char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt)
+char *netdev_bits(char *buf, char *end, const void *addr,
+		  struct printf_spec spec,  const char *fmt)
 {
 	unsigned long long num;
 	int size;
@@ -1521,9 +1522,7 @@ char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt)
 		size = sizeof(netdev_features_t);
 		break;
 	default:
-		num = (unsigned long)addr;
-		size = sizeof(unsigned long);
-		break;
+		return ptr_to_id(buf, end, addr, spec);
 	}
 
 	return special_hex_number(buf, end, num, size);
@@ -1949,7 +1948,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			break;
 		return restricted_pointer(buf, end, ptr, spec);
 	case 'N':
-		return netdev_bits(buf, end, ptr, fmt);
+		return netdev_bits(buf, end, ptr, spec, fmt);
 	case 'a':
 		return address_val(buf, end, ptr, fmt);
 	case 'd':
-- 
2.17.1

Re: [PATCH v2 0/3] lib/vsprintf: Hash remaining raw addresses

[Andy Shevchenko]

On Thu, Oct 11, 2018 at 10:42:46AM +0200, Geert Uytterhoeven wrote:
> 	Hi all,
> 
> There are still two format specifiers that print unhanced kernel
> addresses, potentially leaking sensitive information regarding the
> kernel layout in memory.
> 
> This patch series fixes this by printing hashed addresses instead.

Looks nice!

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

>     
> Changes compared to v1:
>   - Get rid of the forward declaration for ptr_to_id(),
>   - Add Reviewed-by.
> 
> Thanks!
> 
> Geert Uytterhoeven (3):
>   lib/vsprintf: Prepare for more general use of ptr_to_id()
>   lib/vsprintf: Hash legacy clock addresses
>   lib/vsprintf: Hash printed address for netdev bits fallback
> 
>  Documentation/core-api/printk-formats.rst |   5 +-
>  lib/vsprintf.c                            | 216 +++++++++++-----------
>  2 files changed, 110 insertions(+), 111 deletions(-)
> 
> -- 
> 2.17.1
> 
> 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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] lib/vsprintf: Prepare for more general use of ptr_to_id()

[Petr Mladek]

On Thu 2018-10-11 10:42:47, Geert Uytterhoeven wrote:
>   - Make the ptr argument const, to avoid adding casts in future
>     callers,
>   - Move the function and its dependencies up, so it can be called from
>     special pointer type formatting routines.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

I have pushed the changes into printk.git,
branch for-4.20-vsprintf-hash-fixes.

Note that I have split it into two separate commits. The first one
adds the const modifier. The second commit does a pure move of
the code without any "hidden" changes. I forgot to ask for this
when reviewing v1.

You might see the result at
https://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git/log/?h=for-4.20-vsprintf-hash-fixes

Best Regards,
Petr

Re: [PATCH v2 2/3] lib/vsprintf: Hash legacy clock addresses

[Petr Mladek]

On Thu 2018-10-11 10:42:48, Geert Uytterhoeven wrote:
> On platforms using the Common Clock Framework, "%pC" prints the clock's
> name. On legacy platforms, it prints the unhashed clock's address,
> potentially leaking sensitive information regarding the kernel layout in
> memory.
> 
> Avoid this leak by printing the hashed address instead.  To distinguish
> between clocks, a 32-bit unique identifier is as good as an actual
> pointer value.
> 
> Fixes: ad67b74d2469d9b8 ("printk: hash addresses printed with %p")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Petr Mladek <pmladek@suse.com>

I have pushed this patch into printk.git,
branch for-4.20-vsprintf-hash-fixes.

Best Regards,
Petr

Re: [PATCH v2 3/3] lib/vsprintf: Hash printed address for netdev bits fallback

[Petr Mladek]

On Thu 2018-10-11 10:42:49, Geert Uytterhoeven wrote:
> The handler for "%pN" falls back to printing the raw pointer value when
> using a different format than the (sole supported) special format
> "%pNF", potentially leaking sensitive information regarding the kernel
> layout in memory.
> 
> Avoid this leak by printing the hashed address instead.
> Note that there are no in-tree users of the fallback.
> 
> Fixes: ad67b74d2469d9b8 ("printk: hash addresses printed with %p")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Petr Mladek <pmladek@suse.com>

I have pushed this patch into printk.git,
branch for-4.20-vsprintf-hash-fixes.

Best Regards,
Petr

Re: [PATCH v2 1/3] lib/vsprintf: Prepare for more general use of ptr_to_id()

[Geert Uytterhoeven]

Hi Petr,

On Fri, Oct 12, 2018 at 12:39 PM Petr Mladek <pmladek@suse.com> wrote:
> On Thu 2018-10-11 10:42:47, Geert Uytterhoeven wrote:
> >   - Make the ptr argument const, to avoid adding casts in future
> >     callers,
> >   - Move the function and its dependencies up, so it can be called from
> >     special pointer type formatting routines.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> I have pushed the changes into printk.git,
> branch for-4.20-vsprintf-hash-fixes.
>
> Note that I have split it into two separate commits. The first one
> adds the const modifier. The second commit does a pure move of
> the code without any "hidden" changes. I forgot to ask for this
> when reviewing v1.
>
> You might see the result at
> https://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git/log/?h=for-4.20-vsprintf-hash-fixes

Thanks, looks good!

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