[PATCH 0/3] enable early printing of hashed pointers

[PATCH 0/3] enable early printing of hashed pointers

[Tobin C. Harding]

Currently if an attempt is made to print a pointer before there is
enough entropy then '(____ptrval____)' is printed.  This makes debugging
stack traces during early boot difficult.

It was observed that we can relax the requirement for cryptographically
secure hashing when debugging while still maintaining pointer hashing
behaviour.  This allows kernels to be debugged without developers
relying on different pointer printing behavior.

Most of this code (excluding the command line option stuff) was written
by Kees and Linus on LKML.  Any mistakes introduced are however my own.

For the command line option stuff I copied crypto/fips.c mixed with
kernel/power/hibernate.c 

Taking suggestions please on the get_random_bytes_arch() local variable
name 'left' (I thought of 'togo' and 'rem' (as in remaining) as well).

Testing

The command line option stuff has been verified to work.  I have built
and booted a patched kernel on a machine without RDRAND (qemu) and on a
machine with RDRAND (intel core M5Y70).  The code appears to work as it
is written.  I am unsure however whether this solves the initial
problem.  I tried printing a pointer from within 
init/main.c:kernel_init() and that did _not_ work.  I do not know how
early is early, or in other words, when we are hoping for this to work.

thanks,
Tobin.


Tobin C. Harding (3):
  random: Fix whitespace pre random-bytes work
  random: Return nbytes filled from hw RNG
  vsprintf: Add use-early-random-bytes cmd line option

 drivers/char/random.c  | 19 ++++++++++---------
 include/linux/random.h |  2 +-
 lib/vsprintf.c         | 50 +++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 58 insertions(+), 13 deletions(-)

-- 
2.7.4

[PATCH 1/3] 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>
---
 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 cd888d4ee605..031d18b31e0f 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1737,7 +1737,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;
@@ -1748,7 +1748,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 2/3] 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.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.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 031d18b31e0f..3a66507ea60b 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1725,26 +1725,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)
 {
 	char *p = buf;
+	int left = nbytes;
 
-	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(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..308168c0a637 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 get_random_bytes_arch(void *buf, int nbytes);
 
 #ifndef MODULE
 extern const struct file_operations random_fops, urandom_fops;
-- 
2.7.4

[PATCH 3/3] vsprintf: Add use-early-random-bytes cmd line option

[Tobin C. Harding]

Currently if an attempt is made to print a pointer before there is
enough entropy then '(____ptrval____)' is printed.  This makes debugging
early stage stack traces difficult.  We can relax the requirement for
cryptographically secure hashing when debugging while still maintaining
pointer hashing behaviour.

Add a command line option 'use-early-random-bytes'.  When enabled get
key material from the hw RNG if available.

This option should NOT be enabled on production kernels.

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 lib/vsprintf.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 3 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b82f0c6c2aec..7b9cf6bb9fd2 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1654,12 +1654,39 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
+/*
+ * Command line option use_early_random_bytes allows debugging early in
+ * the boot sequence, if enabled we attempt to use the hw RNG to get
+ * ptr_key material.  Do NOT use on production kernels.
+ */
+static int use_early_random_bytes;
+EXPORT_SYMBOL(use_early_random_bytes);
+
+/*
+ * Process kernel command-line parameter at boot time.
+ * use_early_random_bytes=0 or use_early_random_bytes=1
+ */
+static int __init use_early_random_bytes_enable(char *str)
+{
+	long option;
+	int ret;
+
+	ret = !!kstrtol(str, 10, &option);
+	if (ret == 0 && option != 0)
+		use_early_random_bytes = 1;
+
+	pr_info("use_early_random_bytes: %s\n",
+	        use_early_random_bytes ? "enabled" : "disabled");
+
+	return 1;
+}
+__setup("use-early-random-bytes=", use_early_random_bytes_enable);
+
 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)
+static void ptr_key_ready(void)
 {
-	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
@@ -1669,13 +1696,30 @@ static void fill_random_ptr_key(struct random_ready_callback *unused)
 	WRITE_ONCE(have_filled_random_ptr_key, true);
 }
 
+static void fill_random_ptr_key(struct random_ready_callback *unused)
+{
+	get_random_bytes(&ptr_key, sizeof(ptr_key));
+	ptr_key_ready();
+}
+
 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);
+	int ret;
+
+	if (use_early_random_bytes) {
+		int nbytes = sizeof(ptr_key);
+
+		if (get_random_bytes_arch(&ptr_key, nbytes) == nbytes) {
+			ptr_key_ready();
+			return 0;
+		}
+	}
+
+	ret = add_random_ready_callback(&random_ready);
 
 	if (!ret) {
 		return 0;
-- 
2.7.4

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

[Tobin C. Harding]

On Wed, May 02, 2018 at 09:33:39AM +1000, Tobin C. Harding 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.
> 
> 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.
> 
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.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 031d18b31e0f..3a66507ea60b 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1725,26 +1725,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)
>  {
>  	char *p = buf;
> +	int left = nbytes;
>  
> -	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(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..308168c0a637 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 get_random_bytes_arch(void *buf, int nbytes);
>  
>  #ifndef MODULE
>  extern const struct file_operations random_fops, urandom_fops;
> -- 
> 2.7.4
> 

Tested by loading a module with this init funciton

static int __init test_init(void)
{
	const int size = 256;
	char buf[size];
	int got;

	pr_info("test: module inserted\n");

	got = get_random_bytes_arch(buf, size);
	if (got != size)
		pr_warn("get_random_bytes_arch() failed, got %d bytes\n", got);

	return 0;
}
module_init(test_init);


Fails on machine without RDRAND and succeeds on machine with RDRAND

thanks,
Tobin.

Re: [PATCH 1/3] random: Fix whitespace pre random-bytes work

[Theodore Y. Ts'o]

On Wed, May 02, 2018 at 09:33:38AM +1000, Tobin C. Harding wrote:
> 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 cd888d4ee605..031d18b31e0f 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1737,7 +1737,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;
> @@ -1748,7 +1748,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
> 

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

[Theodore Y. Ts'o]

On Wed, May 02, 2018 at 09:33:39AM +1000, Tobin C. Harding 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.
> 
> 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.
> 
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Tobin C. Harding <me@tobin.cc>

Acked-by: Theodore Ts'o <tytso@mit.edu>

Re: [PATCH 3/3] vsprintf: Add use-early-random-bytes cmd line option

[Linus Torvalds]

On Tue, May 1, 2018 at 4:34 PM Tobin C. Harding <me@tobin.cc> wrote:


> This option should NOT be enabled on production kernels.

I think with your fixes to get_random_bytes_arch(), it's perfectly fine to
use on production kernels (and doesn't even need a kernel command line
option).

It was only with the "use weak crypto" (that get_random_bytes_arch() used
to fall back on) that it was a problem. That fixed "verify that
get_random_bytes_arch() really uses hw crypto" is certainly not weak crypto.

              Linus

Re: [PATCH 3/3] vsprintf: Add use-early-random-bytes cmd line option

[tcharding]

On Wed, May 02, 2018 at 01:02:34AM +0000, Linus Torvalds wrote:
> On Tue, May 1, 2018 at 4:34 PM Tobin C. Harding <me@tobin.cc> wrote:
> 
> 
> > This option should NOT be enabled on production kernels.
> 
> I think with your fixes to get_random_bytes_arch(), it's perfectly fine to
> use on production kernels (and doesn't even need a kernel command line
> option).
> 
> It was only with the "use weak crypto" (that get_random_bytes_arch() used
> to fall back on) that it was a problem. That fixed "verify that
> get_random_bytes_arch() really uses hw crypto" is certainly not weak crypto.

Ok, I'll wait to see if anyone with a more paranoid disposition adds to
this otherwise will implement as suggested.

thanks,
Tobin.

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

[Steven Rostedt]

On Wed,  2 May 2018 09:33:39 +1000
"Tobin C. Harding" <me@tobin.cc> wrote:

> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 031d18b31e0f..3a66507ea60b 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1725,26 +1725,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)

The "__must_check" makes no sense in the C file. It belongs in the
header file. There wont be any complaint about it here.

-- Steve


>  {
>  	char *p = buf;



> diff --git a/include/linux/random.h b/include/linux/random.h
> index 2ddf13b4281e..308168c0a637 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 get_random_bytes_arch(void *buf, int nbytes);
>  
>  #ifndef MODULE
>  extern const struct file_operations random_fops, urandom_fops;

Re: [PATCH 3/3] vsprintf: Add use-early-random-bytes cmd line option

[Steven Rostedt]

On Wed, 2 May 2018 11:27:58 +1000
tcharding <me@tobin.cc> wrote:

> On Wed, May 02, 2018 at 01:02:34AM +0000, Linus Torvalds wrote:
> > On Tue, May 1, 2018 at 4:34 PM Tobin C. Harding <me@tobin.cc> wrote:
> > 
> >   
> > > This option should NOT be enabled on production kernels.  
> > 
> > I think with your fixes to get_random_bytes_arch(), it's perfectly fine to
> > use on production kernels (and doesn't even need a kernel command line
> > option).
> > 
> > It was only with the "use weak crypto" (that get_random_bytes_arch() used
> > to fall back on) that it was a problem. That fixed "verify that
> > get_random_bytes_arch() really uses hw crypto" is certainly not weak crypto.  

Except for where hardware vendors control what random bytes you
actually get ;-)

> 
> Ok, I'll wait to see if anyone with a more paranoid disposition adds to
> this otherwise will implement as suggested.

I still test on a lot of old boxes (my old workstations turn into my
test boxes). I haven't tried to see which machines have RDRAND support.
But regardless, can we still have a command line option that forces
early randomization even if RDRAND isn't supported?

-- Steve

Re: [PATCH 3/3] vsprintf: Add use-early-random-bytes cmd line option

[tcharding]

On Tue, May 01, 2018 at 09:45:07PM -0400, Steven Rostedt wrote:
> On Wed, 2 May 2018 11:27:58 +1000
> tcharding <me@tobin.cc> wrote:
> 
> > On Wed, May 02, 2018 at 01:02:34AM +0000, Linus Torvalds wrote:
> > > On Tue, May 1, 2018 at 4:34 PM Tobin C. Harding <me@tobin.cc> wrote:
> > > 
> > >   
> > > > This option should NOT be enabled on production kernels.  
> > > 
> > > I think with your fixes to get_random_bytes_arch(), it's perfectly fine to
> > > use on production kernels (and doesn't even need a kernel command line
> > > option).
> > > 
> > > It was only with the "use weak crypto" (that get_random_bytes_arch() used
> > > to fall back on) that it was a problem. That fixed "verify that
> > > get_random_bytes_arch() really uses hw crypto" is certainly not weak crypto.  
> 
> Except for where hardware vendors control what random bytes you
> actually get ;-)
> 
> > 
> > Ok, I'll wait to see if anyone with a more paranoid disposition adds to
> > this otherwise will implement as suggested.
> 
> I still test on a lot of old boxes (my old workstations turn into my
> test boxes). I haven't tried to see which machines have RDRAND support.
> But regardless, can we still have a command line option that forces
> early randomization even if RDRAND isn't supported?

This is now two different issues, right?

1) get_random_bytes_arch() was broken for this use case - this set fixes
   that.  We can just use the hw RNG if available for key material to
   hash pointers with.

2) Early stage debugging is still an issue on boxes without RDRAND.  If
   we are agreed that we don't need cryptographically secure hashing on
   test kernels then we could just use a simple hashing algorithm, for
   example multiply the address by a prime and take the high 32 bits (as
   long as it was guarded by a command line option).

thanks,
Tobin.

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

[Tobin C. Harding]

On Tue, May 01, 2018 at 09:39:02PM -0400, Steven Rostedt wrote:
> On Wed,  2 May 2018 09:33:39 +1000
> "Tobin C. Harding" <me@tobin.cc> wrote:
> 
> > diff --git a/drivers/char/random.c b/drivers/char/random.c
> > index 031d18b31e0f..3a66507ea60b 100644
> > --- a/drivers/char/random.c
> > +++ b/drivers/char/random.c
> > @@ -1725,26 +1725,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)
> 
> The "__must_check" makes no sense in the C file. It belongs in the
> header file. There wont be any complaint about it here.

Oh thanks.

/steve schooling noobs all day long

Re: [PATCH 3/3] vsprintf: Add use-early-random-bytes cmd line option

[Linus Torvalds]

On Tue, May 1, 2018 at 6:45 PM Steven Rostedt <rostedt@goodmis.org> wrote:

> Except for where hardware vendors control what random bytes you
> actually get ;-)

In which case you should just use "nordrand" and be done with it.

.. and you might also want to reconsider your other life choices, because
honestly, there are bigger problems you should worry about.

                Linus

Re: [PATCH 3/3] vsprintf: Add use-early-random-bytes cmd line option

[Andrew Morton]

On Wed,  2 May 2018 09:33:40 +1000 "Tobin C. Harding" <me@tobin.cc> wrote:

> Currently if an attempt is made to print a pointer before there is
> enough entropy then '(____ptrval____)' is printed.  This makes debugging
> early stage stack traces difficult.  We can relax the requirement for
> cryptographically secure hashing when debugging while still maintaining
> pointer hashing behaviour.
> 
> Add a command line option 'use-early-random-bytes'.  When enabled get
> key material from the hw RNG if available.

Documentation/admin-guide/kernel-parameters.rst, please.

> This option should NOT be enabled on production kernels.

And the documentation should explain why this recommendation is made. 
Here was I scratching my head wondering why this feature isn't just
permanently enabled.  Still scratching away...

Re: [PATCH 3/3] vsprintf: Add use-early-random-bytes cmd line option

[Tobin C. Harding]

On Wed, May 02, 2018 at 02:56:45PM -0700, Andrew Morton wrote:
> On Wed,  2 May 2018 09:33:40 +1000 "Tobin C. Harding" <me@tobin.cc> wrote:
> 
> > Currently if an attempt is made to print a pointer before there is
> > enough entropy then '(____ptrval____)' is printed.  This makes debugging
> > early stage stack traces difficult.  We can relax the requirement for
> > cryptographically secure hashing when debugging while still maintaining
> > pointer hashing behaviour.
> > 
> > Add a command line option 'use-early-random-bytes'.  When enabled get
> > key material from the hw RNG if available.
> 
> Documentation/admin-guide/kernel-parameters.rst, please.
> 
> > This option should NOT be enabled on production kernels.
> 
> And the documentation should explain why this recommendation is made. 
> Here was I scratching my head wondering why this feature isn't just
> permanently enabled.  Still scratching away...

Thanks for the review.  I think v2 covers your concerns, it patches
Documentation/kernel-parameters

If you are still scratching after reading v2 please do say so, I
appreciate the feedback.

thanks,
Tobin.