[PATCH v7 0/4] enable early printing of hashed pointers

[PATCH v7 0/4] enable early printing of hashed pointers

[Tobin C. Harding]

Currently printing pointers early in the boot sequence can result in a
dummy string '(____ptrval____)' being printed.  While resolving this
issue it was noticed that we can use the hw RNG if available for hashing
pointers.

Patch one and two do the ground work to be able to use hw RNG removing
from get_random_bytes_arch() the call to get_random_bytes() and
returning the number of bytes of random material successfully returned. 

Patch three uses the hw RNG to get keying material if it is available.

Patch four further assists debugging early in the boot sequence for
machines that do not have a hw RNG by adding a command line option
'debug_boot_weak_hash'.  If enabled, non-cryptographically secure hashing
is used instead of siphash so we can hash at any time. 

No changes to logic since v6.

thanks,
Tobin.

v7
 - Remove unused variable, clearing compiler warning (found by Stephen
   Rothwell's linux-next build infrastructure).

v6
 - Rebase on top of Steve's patch (fixing race condition).  Uses static
   branch instead of memory barrier.

v5
 - Use 'upside-down-xmas-tree' style to declare local variables (Steve)
 - Added Reviewed-by tag from Steve (patch 2 and 3).

v4
 - remove last patch of series (command line option patch)

v3
 - Add __ro_after_init (suggested by Kees).

v2
 - Use min_t() instead of min() (thanks checkpatch).
 - Add __must_check to function declaration (thanks Steve).
 - Use hw RNG by default if available (as originally suggested by Kees).
 - Add command line option to use cryptographically insecure hashing.
   If debug_early_boot is enabled use hash_long() instead of siphash
   (as requested by Steve, and solves original problem for Anna-Maria).
 - Added Acked-by tag from Ted (patch 1 and 2)


Tobin C. Harding (4):
  random: Fix whitespace pre random-bytes work
  random: Return nbytes filled from hw RNG
  vsprintf: Use hw RNG for ptr_key
  vsprintf: Add command line option debug_boot_weak_hash

 Documentation/admin-guide/kernel-parameters.txt |  9 ++++++++
 drivers/char/random.c                           | 19 +++++++++--------
 include/linux/random.h                          |  2 +-
 lib/vsprintf.c                                  | 28 ++++++++++++++++++++++++-
 4 files changed, 47 insertions(+), 11 deletions(-)

-- 
2.7.4

[PATCH v7 1/4] random: Fix whitespace pre random-bytes work

[Tobin C. Harding]

There are a couple of whitespace issues around the function
get_random_bytes_arch().  In preparation for patching this function
let's clean them up.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
Acked-by: Theodore Ts'o <tytso@mit.edu>
---
 drivers/char/random.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index a8fb0020ba5c..ed679099afba 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1736,7 +1736,7 @@ void get_random_bytes_arch(void *buf, int nbytes)
 
 		if (!arch_get_random_long(&v))
 			break;
-		
+
 		memcpy(p, &v, chunk);
 		p += chunk;
 		nbytes -= chunk;
@@ -1747,7 +1747,6 @@ void get_random_bytes_arch(void *buf, int nbytes)
 }
 EXPORT_SYMBOL(get_random_bytes_arch);
 
-
 /*
  * init_std_data - initialize pool with system data
  *
-- 
2.7.4

[PATCH v7 2/4] random: Return nbytes filled from hw RNG

[Tobin C. Harding]

Currently the function get_random_bytes_arch() has return value 'void'.
If the hw RNG fails we currently fall back to using get_random_bytes().
This defeats the purpose of requesting random material from the hw RNG
in the first place.

There are currently no intree users of get_random_bytes_arch().

Only get random bytes from the hw RNG, make function return the number
of bytes retrieved from the hw RNG.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
Acked-by: Theodore Ts'o <tytso@mit.edu>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 drivers/char/random.c  | 16 +++++++++-------
 include/linux/random.h |  2 +-
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index ed679099afba..bd3d3fbea632 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1724,26 +1724,28 @@ EXPORT_SYMBOL(del_random_ready_callback);
  * key known by the NSA).  So it's useful if we need the speed, but
  * only if we're willing to trust the hardware manufacturer not to
  * have put in a back door.
+ *
+ * Return number of bytes filled in.
  */
-void get_random_bytes_arch(void *buf, int nbytes)
+int __must_check get_random_bytes_arch(void *buf, int nbytes)
 {
+	int left = nbytes;
 	char *p = buf;
 
-	trace_get_random_bytes_arch(nbytes, _RET_IP_);
-	while (nbytes) {
+	trace_get_random_bytes_arch(left, _RET_IP_);
+	while (left) {
 		unsigned long v;
-		int chunk = min(nbytes, (int)sizeof(unsigned long));
+		int chunk = min_t(int, left, (int)sizeof(unsigned long));
 
 		if (!arch_get_random_long(&v))
 			break;
 
 		memcpy(p, &v, chunk);
 		p += chunk;
-		nbytes -= chunk;
+		left -= chunk;
 	}
 
-	if (nbytes)
-		get_random_bytes(p, nbytes);
+	return nbytes - left;
 }
 EXPORT_SYMBOL(get_random_bytes_arch);
 
diff --git a/include/linux/random.h b/include/linux/random.h
index 2ddf13b4281e..f1c9bc5cd231 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -38,7 +38,7 @@ extern void get_random_bytes(void *buf, int nbytes);
 extern int wait_for_random_bytes(void);
 extern int add_random_ready_callback(struct random_ready_callback *rdy);
 extern void del_random_ready_callback(struct random_ready_callback *rdy);
-extern void get_random_bytes_arch(void *buf, int nbytes);
+extern int __must_check get_random_bytes_arch(void *buf, int nbytes);
 
 #ifndef MODULE
 extern const struct file_operations random_fops, urandom_fops;
-- 
2.7.4

Re: [PATCH v7 2/4] random: Return nbytes filled from hw RNG

[Andy Shevchenko]

On Tue, Jun 12, 2018 at 3:39 AM, Tobin C. Harding <me@tobin.cc> wrote:
> Currently the function get_random_bytes_arch() has return value 'void'.
> If the hw RNG fails we currently fall back to using get_random_bytes().
> This defeats the purpose of requesting random material from the hw RNG
> in the first place.

> -               int chunk = min(nbytes, (int)sizeof(unsigned long));
> +               int chunk = min_t(int, left, (int)sizeof(unsigned long));

Isn't this (int) implied when you use min_t(int, ...) ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 2/4] random: Return nbytes filled from hw RNG

[Steven Rostedt]

On Tue, 12 Jun 2018 11:39:56 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Jun 12, 2018 at 3:39 AM, Tobin C. Harding <me@tobin.cc> wrote:
> > Currently the function get_random_bytes_arch() has return value 'void'.
> > If the hw RNG fails we currently fall back to using get_random_bytes().
> > This defeats the purpose of requesting random material from the hw RNG
> > in the first place.  
> 
> > -               int chunk = min(nbytes, (int)sizeof(unsigned long));
> > +               int chunk = min_t(int, left, (int)sizeof(unsigned long));  
> 
> Isn't this (int) implied when you use min_t(int, ...) ?
> 

I would say so...

 #define min_t(type, x, y)	__careful_cmp((type)(x), (type)(y), <)

-- Steve

Re: [PATCH v7 2/4] random: Return nbytes filled from hw RNG

[Tobin C. Harding]

On Tue, Jun 12, 2018 at 11:39:56AM +0300, Andy Shevchenko wrote:.
> On Tue, Jun 12, 2018 at 3:39 AM, Tobin C. Harding <me@tobin.cc> wrote:
> > Currently the function get_random_bytes_arch() has return value 'void'.
> > If the hw RNG fails we currently fall back to using get_random_bytes().
> > This defeats the purpose of requesting random material from the hw RNG
> > in the first place.
> 
> > -               int chunk = min(nbytes, (int)sizeof(unsigned long));
> > +               int chunk = min_t(int, left, (int)sizeof(unsigned long));
> 
> Isn't this (int) implied when you use min_t(int, ...) ?

thanks Andy, will spin another version.

	Tobin

[PATCH v7 3/4] vsprintf: Use hw RNG for ptr_key

[Tobin C. Harding]

Currently we must wait for enough entropy to become available before
hashed pointers can be printed.  We can remove this wait by using the
hw RNG if available.

Use hw RNG to get keying material.

Cc: Steven Rostedt (VMware) <rostedt@goodmis.org>
Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 lib/vsprintf.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a48aaa79d352..c445f9f28760 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1675,8 +1675,16 @@ static struct random_ready_callback random_ready = {
 
 static int __init initialize_ptr_random(void)
 {
-	int ret = add_random_ready_callback(&random_ready);
+	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) {
-- 
2.7.4

Re: [PATCH v7 3/4] vsprintf: Use hw RNG for ptr_key

[Steven Rostedt]

On Tue, 12 Jun 2018 10:39:13 +1000
"Tobin C. Harding" <me@tobin.cc> wrote:

> Currently we must wait for enough entropy to become available before
> hashed pointers can be printed.  We can remove this wait by using the
> hw RNG if available.
> 
> Use hw RNG to get keying material.
> 
> Cc: Steven Rostedt (VMware) <rostedt@goodmis.org>

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> ---
>  lib/vsprintf.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index a48aaa79d352..c445f9f28760 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1675,8 +1675,16 @@ static struct random_ready_callback random_ready = {
>  
>  static int __init initialize_ptr_random(void)
>  {
> -	int ret = add_random_ready_callback(&random_ready);
> +	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) {

Re: [PATCH v7 3/4] vsprintf: Use hw RNG for ptr_key

[Tobin C. Harding]

On Tue, Jun 12, 2018 at 02:44:58PM -0400, Steven Rostedt wrote:
> On Tue, 12 Jun 2018 10:39:13 +1000
> "Tobin C. Harding" <me@tobin.cc> wrote:
> 
> > Currently we must wait for enough entropy to become available before
> > hashed pointers can be printed.  We can remove this wait by using the
> > hw RNG if available.
> > 
> > Use hw RNG to get keying material.
> > 
> > Cc: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Thanks for reviewing this patch again Steve.  My mistake, I forgot to
add your reviewed-by from the last version.

thanks for your patience.

	Tobin

[PATCH v7 4/4] vsprintf: Add command line option debug_boot_weak_hash

[Tobin C. Harding]

Currently printing [hashed] pointers requires enough entropy to be
available.  Early in the boot sequence this may not be the case
resulting in a dummy string '(____ptrval____)' being printed.  This
makes debugging the early boot sequence difficult.  We can relax the
requirement to use cryptographically secure hashing during debugging.
This enables debugging while keeping development/production kernel
behaviour the same.

If new command line option debug_boot_weak_hash is enabled use
cryptographically insecure hashing and hash pointer value immediately.

Cc: Anna-Maria Gleixner <anna-maria@linutronix.de>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 Documentation/admin-guide/kernel-parameters.txt |  9 +++++++++
 lib/vsprintf.c                                  | 18 ++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 638342d0a095..a116fc0366b0 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -748,6 +748,15 @@
 
 	debug		[KNL] Enable kernel debugging (events log level).
 
+	debug_boot_weak_hash
+			[KNL] Enable printing pointers early in the boot
+			sequence.  If enabled, we use a weak hash instead of
+			siphash to hash pointers.  Use this option if you need
+			to see pointer values during early boot (i.e you are
+			seeing instances of '(___ptrval___)').
+			Cryptographically insecure, please do not use on
+			production kernels.
+
 	debug_locks_verbose=
 			[KNL] verbose self-tests
 			Format=<0|1>
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index c445f9f28760..6d2479eb7c2b 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1651,6 +1651,18 @@ 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;
+EXPORT_SYMBOL(debug_boot_weak_hash);
+
+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;
 
@@ -1703,6 +1715,12 @@ 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 */
-- 
2.7.4

Re: [PATCH v7 4/4] vsprintf: Add command line option debug_boot_weak_hash

[Steven Rostedt]

On Tue, 12 Jun 2018 10:39:14 +1000
"Tobin C. Harding" <me@tobin.cc> wrote:


> +/* Make pointers available for printing early in the boot sequence. */
> +static int debug_boot_weak_hash __ro_after_init;
> +EXPORT_SYMBOL(debug_boot_weak_hash);
> +
> +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;
>  
> @@ -1703,6 +1715,12 @@ 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)) {

I was hoping to find a way to make the debug_boot_weak_hash into a
static_branch, but its setting is too early to have for a jump label.

I toyed with nesting this inside a static_branch too, but it seems too
much overkill.

Hence, I'll just punt and accept this patch as is ;-)

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

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

Re: [PATCH v7 4/4] vsprintf: Add command line option debug_boot_weak_hash

[Steven Rostedt]

On Tue, 12 Jun 2018 10:39:14 +1000
"Tobin C. Harding" <me@tobin.cc> wrote:


> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index c445f9f28760..6d2479eb7c2b 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1651,6 +1651,18 @@ 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;
> +EXPORT_SYMBOL(debug_boot_weak_hash);


I was too busy trying to make this use a static_branch, that I forgot
to mention this in my review.

Why did you add that EXPORT_SYMBOL()? It's useless with a static
variable. I think you can nuke that line.

-- Steve