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

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

[Tobin C. Harding]

Ted,

Version 8 changes patch 2 which you previously acked.  Change is to
remove integer cast as suggested on LKML.  I've kept the ACK.

Changes made to patch with reviewed-by tag from Steve were acknowledged
on LKML, I've also kept Steve's tag.



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. 


thanks,
Tobin.

v8
 - Remove pointless EXPORT_SYMBOL on static variable (thanks Steve).
 - Remove unnecessary integer cast from min_t() argument (thanks Andy).

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

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

-- 
2.17.1

[PATCH v8 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.

Acked-by: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 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.17.1

[PATCH v8 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.

Acked-by: Theodore Ts'o <tytso@mit.edu>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 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..e98fa03cdb91 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, 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.17.1

[PATCH v8 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.

Reviewed-by: 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.17.1

[PATCH v8 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.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 Documentation/admin-guide/kernel-parameters.txt |  9 +++++++++
 lib/vsprintf.c                                  | 17 +++++++++++++++++
 2 files changed, 26 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..17ebe076ae41 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1651,6 +1651,17 @@ 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;
 
@@ -1703,6 +1714,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.17.1

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

[Randy Dunlap]

On 06/19/2018 09:20 PM, Tobin C. Harding wrote:
> 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.
> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  9 +++++++++
>  lib/vsprintf.c                                  | 17 +++++++++++++++++
>  2 files changed, 26 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

maybe:
			to see hashed pointer values
i.e., not raw pointers.

> +			seeing instances of '(___ptrval___)').
> +			Cryptographically insecure, please do not use on
> +			production kernels.
> +
>  	debug_locks_verbose=
>  			[KNL] verbose self-tests
>  			Format=<0|1>

thanks,
-- 
~Randy

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

[Tobin C. Harding]

On Wed, Jun 20, 2018 at 09:09:49AM -0700, Randy Dunlap wrote:
> On 06/19/2018 09:20 PM, Tobin C. Harding wrote:
> > 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.
> > 
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > ---
> >  Documentation/admin-guide/kernel-parameters.txt |  9 +++++++++
> >  lib/vsprintf.c                                  | 17 +++++++++++++++++
> >  2 files changed, 26 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
> 
> maybe:
> 			to see hashed pointer values
> i.e., not raw pointers.

You cannot see 'raw pointers' anyways?

> 
> > +			seeing instances of '(___ptrval___)').
> > +			Cryptographically insecure, please do not use on
> > +			production kernels.

thanks for the review, I don't quiet see how to use your suggestion to
make the text clearer.  If you still feel this change is needed perhaps
you could write so I understand i.e 'Use this option if ...'


thanks,
Tobin.

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

[Randy Dunlap]

On 06/20/2018 03:30 PM, Tobin C. Harding wrote:
> On Wed, Jun 20, 2018 at 09:09:49AM -0700, Randy Dunlap wrote:
>> On 06/19/2018 09:20 PM, Tobin C. Harding wrote:
>>> 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.
>>>
>>> Signed-off-by: Tobin C. Harding <me@tobin.cc>
>>> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>>> ---
>>>  Documentation/admin-guide/kernel-parameters.txt |  9 +++++++++
>>>  lib/vsprintf.c                                  | 17 +++++++++++++++++
>>>  2 files changed, 26 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
>>
>> maybe:
>> 			to see hashed pointer values
>> i.e., not raw pointers.
> 
> You cannot see 'raw pointers' anyways?

only if using %px ?

Maybe it's just terminology.  I don't consider a hashed value as a pointer value.
It's just a key or handle or some other number, but it's not a pointer.

>>
>>> +			seeing instances of '(___ptrval___)').
>>> +			Cryptographically insecure, please do not use on
>>> +			production kernels.
> 
> thanks for the review, I don't quiet see how to use your suggestion to
> make the text clearer.  If you still feel this change is needed perhaps
> you could write so I understand i.e 'Use this option if ...'


OK, if you are good with it, I am too.  :)

thanks,
-- 
~Randy

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

[Tobin C. Harding]

On Wed, Jun 20, 2018 at 03:36:44PM -0700, Randy Dunlap wrote:
> On 06/20/2018 03:30 PM, Tobin C. Harding wrote:
> > On Wed, Jun 20, 2018 at 09:09:49AM -0700, Randy Dunlap wrote:
> >> On 06/19/2018 09:20 PM, Tobin C. Harding wrote:
> >>> 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.
> >>>
> >>> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> >>> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> >>> ---
> >>>  Documentation/admin-guide/kernel-parameters.txt |  9 +++++++++
> >>>  lib/vsprintf.c                                  | 17 +++++++++++++++++
> >>>  2 files changed, 26 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
> >>
> >> maybe:
> >> 			to see hashed pointer values
> >> i.e., not raw pointers.
> > 
> > You cannot see 'raw pointers' anyways?
> 
> only if using %px ?
> 
> Maybe it's just terminology.  I don't consider a hashed value as a pointer value.
> It's just a key or handle or some other number, but it's not a pointer.
> 
> >>
> >>> +			seeing instances of '(___ptrval___)').
> >>> +			Cryptographically insecure, please do not use on
> >>> +			production kernels.
> > 
> > thanks for the review, I don't quiet see how to use your suggestion to
> > make the text clearer.  If you still feel this change is needed perhaps
> > you could write so I understand i.e 'Use this option if ...'
> 
> 
> OK, if you are good with it, I am too.  :)

I get you know.  I agree, how about this

			[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 print pointers with %px during early boot
			(i.e you are seeing instances of '(___ptrval___)').
			Cryptographically insecure, please do not use on
			production kernels.


thanks for clarifying,
Tobin.

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

[Randy Dunlap]

On 06/20/2018 04:22 PM, Tobin C. Harding wrote:
> On Wed, Jun 20, 2018 at 03:36:44PM -0700, Randy Dunlap wrote:
>> On 06/20/2018 03:30 PM, Tobin C. Harding wrote:
>>> On Wed, Jun 20, 2018 at 09:09:49AM -0700, Randy Dunlap wrote:
>>>> On 06/19/2018 09:20 PM, Tobin C. Harding wrote:
>>>>> 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.
>>>>>
>>>>> Signed-off-by: Tobin C. Harding <me@tobin.cc>
>>>>> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>>>>> ---
>>>>>  Documentation/admin-guide/kernel-parameters.txt |  9 +++++++++
>>>>>  lib/vsprintf.c                                  | 17 +++++++++++++++++
>>>>>  2 files changed, 26 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
>>>>
>>>> maybe:
>>>> 			to see hashed pointer values
>>>> i.e., not raw pointers.
>>>
>>> You cannot see 'raw pointers' anyways?
>>
>> only if using %px ?
>>
>> Maybe it's just terminology.  I don't consider a hashed value as a pointer value.
>> It's just a key or handle or some other number, but it's not a pointer.
>>
>>>>
>>>>> +			seeing instances of '(___ptrval___)').
>>>>> +			Cryptographically insecure, please do not use on
>>>>> +			production kernels.
>>>
>>> thanks for the review, I don't quiet see how to use your suggestion to
>>> make the text clearer.  If you still feel this change is needed perhaps
>>> you could write so I understand i.e 'Use this option if ...'
>>
>>
>> OK, if you are good with it, I am too.  :)
> 
> I get you know.  I agree, how about this
> 
> 			[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 print pointers with %px during early boot
> 			(i.e you are seeing instances of '(___ptrval___)').
> 			Cryptographically insecure, please do not use on
> 			production kernels.

Sorry, I'm still confused by this paragraph.  It seems to say two different
things.

(a) If [this option] is enabled, we use a weak hash instead of siphash.
(b) Use this option to print pointers with %px [during early boot].

Maybe they aren't contradictory?  Is (b) in effect until there is enough
entropy, then (a) takes over?


and nit:  s/i.e/i.e./

thanks,
-- 
~Randy

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

[Tobin C. Harding]

On Wed, Jun 20, 2018 at 04:38:05PM -0700, Randy Dunlap wrote:
> On 06/20/2018 04:22 PM, Tobin C. Harding wrote:
> > On Wed, Jun 20, 2018 at 03:36:44PM -0700, Randy Dunlap wrote:
> >> On 06/20/2018 03:30 PM, Tobin C. Harding wrote:
> >>> On Wed, Jun 20, 2018 at 09:09:49AM -0700, Randy Dunlap wrote:
> >>>> On 06/19/2018 09:20 PM, Tobin C. Harding wrote:
> >>>>> 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.
> >>>>>
> >>>>> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> >>>>> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> >>>>> ---
> >>>>>  Documentation/admin-guide/kernel-parameters.txt |  9 +++++++++
> >>>>>  lib/vsprintf.c                                  | 17 +++++++++++++++++
> >>>>>  2 files changed, 26 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
> >>>>
> >>>> maybe:
> >>>> 			to see hashed pointer values
> >>>> i.e., not raw pointers.
> >>>
> >>> You cannot see 'raw pointers' anyways?
> >>
> >> only if using %px ?
> >>
> >> Maybe it's just terminology.  I don't consider a hashed value as a pointer value.
> >> It's just a key or handle or some other number, but it's not a pointer.
> >>
> >>>>
> >>>>> +			seeing instances of '(___ptrval___)').
> >>>>> +			Cryptographically insecure, please do not use on
> >>>>> +			production kernels.
> >>>
> >>> thanks for the review, I don't quiet see how to use your suggestion to
> >>> make the text clearer.  If you still feel this change is needed perhaps
> >>> you could write so I understand i.e 'Use this option if ...'
> >>
> >>
> >> OK, if you are good with it, I am too.  :)
> > 
> > I get you know.  I agree, how about this
> > 
> > 			[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 print pointers with %px during early boot
> > 			(i.e you are seeing instances of '(___ptrval___)').
> > 			Cryptographically insecure, please do not use on
> > 			production kernels.
> 
> Sorry, I'm still confused by this paragraph.  It seems to say two different
> things.

My bad, I got totally confused myself.  After all this time you would
think I knew which specifier hashed and which didn't.  My apologies,
how about this:

 			[KNL] Enable printing [hashed] pointers early in
			the boot sequence.  If enabled, we use a weak hash
			instead of siphash to hash pointers.  Use this option if
			you are seeing instances of '(___ptrval___)') and need
			to see a value (hashed pointer) instead. Cryptographically
			insecure, please do not use on production kernels.
 			 			

thanks for your patience,
Tobin.

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

[Randy Dunlap]

On 06/20/2018 08:15 PM, Tobin C. Harding wrote:
> On Wed, Jun 20, 2018 at 04:38:05PM -0700, Randy Dunlap wrote:
>> On 06/20/2018 04:22 PM, Tobin C. Harding wrote:
>>> On Wed, Jun 20, 2018 at 03:36:44PM -0700, Randy Dunlap wrote:
>>>> On 06/20/2018 03:30 PM, Tobin C. Harding wrote:
>>>>> On Wed, Jun 20, 2018 at 09:09:49AM -0700, Randy Dunlap wrote:
>>>>>> On 06/19/2018 09:20 PM, Tobin C. Harding wrote:
>>>>>>> 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.
>>>>>>>
>>>>>>> Signed-off-by: Tobin C. Harding <me@tobin.cc>
>>>>>>> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>>>>>>> ---
>>>>>>>  Documentation/admin-guide/kernel-parameters.txt |  9 +++++++++
>>>>>>>  lib/vsprintf.c                                  | 17 +++++++++++++++++
>>>>>>>  2 files changed, 26 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
>>>>>>
>>>>>> maybe:
>>>>>> 			to see hashed pointer values
>>>>>> i.e., not raw pointers.
>>>>>
>>>>> You cannot see 'raw pointers' anyways?
>>>>
>>>> only if using %px ?
>>>>
>>>> Maybe it's just terminology.  I don't consider a hashed value as a pointer value.
>>>> It's just a key or handle or some other number, but it's not a pointer.
>>>>
>>>>>>
>>>>>>> +			seeing instances of '(___ptrval___)').
>>>>>>> +			Cryptographically insecure, please do not use on
>>>>>>> +			production kernels.
>>>>>
>>>>> thanks for the review, I don't quiet see how to use your suggestion to
>>>>> make the text clearer.  If you still feel this change is needed perhaps
>>>>> you could write so I understand i.e 'Use this option if ...'
>>>>
>>>>
>>>> OK, if you are good with it, I am too.  :)
>>>
>>> I get you know.  I agree, how about this
>>>
>>> 			[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 print pointers with %px during early boot
>>> 			(i.e you are seeing instances of '(___ptrval___)').
>>> 			Cryptographically insecure, please do not use on
>>> 			production kernels.
>>
>> Sorry, I'm still confused by this paragraph.  It seems to say two different
>> things.
> 
> My bad, I got totally confused myself.  After all this time you would
> think I knew which specifier hashed and which didn't.  My apologies,
> how about this:
> 
>  			[KNL] Enable printing [hashed] pointers early in
> 			the boot sequence.  If enabled, we use a weak hash
> 			instead of siphash to hash pointers.  Use this option if
> 			you are seeing instances of '(___ptrval___)') and need
> 			to see a value (hashed pointer) instead. Cryptographically
> 			insecure, please do not use on production kernels.
>  			 			
> 
> thanks for your patience,
> Tobin.

Yes, that's good.  Thanks.

-- 
~Randy

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

[Tobin C. Harding]

On Wed, Jun 20, 2018 at 09:09:49PM -0700, Randy Dunlap wrote:
> On 06/20/2018 08:15 PM, Tobin C. Harding wrote:
> > On Wed, Jun 20, 2018 at 04:38:05PM -0700, Randy Dunlap wrote:
> >> On 06/20/2018 04:22 PM, Tobin C. Harding wrote:
> >>> On Wed, Jun 20, 2018 at 03:36:44PM -0700, Randy Dunlap wrote:
> >>>> On 06/20/2018 03:30 PM, Tobin C. Harding wrote:
> >>>>> On Wed, Jun 20, 2018 at 09:09:49AM -0700, Randy Dunlap wrote:
> >>>>>> On 06/19/2018 09:20 PM, Tobin C. Harding wrote:
> >>>>>>> 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.
> >>>>>>>
> >>>>>>> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> >>>>>>> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> >>>>>>> ---
> >>>>>>>  Documentation/admin-guide/kernel-parameters.txt |  9 +++++++++
> >>>>>>>  lib/vsprintf.c                                  | 17 +++++++++++++++++
> >>>>>>>  2 files changed, 26 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
> >>>>>>
> >>>>>> maybe:
> >>>>>> 			to see hashed pointer values
> >>>>>> i.e., not raw pointers.
> >>>>>
> >>>>> You cannot see 'raw pointers' anyways?
> >>>>
> >>>> only if using %px ?
> >>>>
> >>>> Maybe it's just terminology.  I don't consider a hashed value as a pointer value.
> >>>> It's just a key or handle or some other number, but it's not a pointer.
> >>>>
> >>>>>>
> >>>>>>> +			seeing instances of '(___ptrval___)').
> >>>>>>> +			Cryptographically insecure, please do not use on
> >>>>>>> +			production kernels.
> >>>>>
> >>>>> thanks for the review, I don't quiet see how to use your suggestion to
> >>>>> make the text clearer.  If you still feel this change is needed perhaps
> >>>>> you could write so I understand i.e 'Use this option if ...'
> >>>>
> >>>>
> >>>> OK, if you are good with it, I am too.  :)
> >>>
> >>> I get you know.  I agree, how about this
> >>>
> >>> 			[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 print pointers with %px during early boot
> >>> 			(i.e you are seeing instances of '(___ptrval___)').
> >>> 			Cryptographically insecure, please do not use on
> >>> 			production kernels.
> >>
> >> Sorry, I'm still confused by this paragraph.  It seems to say two different
> >> things.
> > 
> > My bad, I got totally confused myself.  After all this time you would
> > think I knew which specifier hashed and which didn't.  My apologies,
> > how about this:
> > 
> >  			[KNL] Enable printing [hashed] pointers early in
> > 			the boot sequence.  If enabled, we use a weak hash
> > 			instead of siphash to hash pointers.  Use this option if
> > 			you are seeing instances of '(___ptrval___)') and need
> > 			to see a value (hashed pointer) instead. Cryptographically
> > 			insecure, please do not use on production kernels.
> >  			 			
> > 
> > thanks for your patience,
> > Tobin.
> 
> Yes, that's good.  Thanks.

Awesome, v9 on it's way :)

thanks,
Tobin.