[PATCH v5] printk: hash addresses printed with %p

[PATCH v5] printk: hash addresses printed with %p

[Tobin C. Harding]

Currently there are many places in the kernel where addresses are being
printed using an unadorned %p. Kernel pointers should be printed using
%pK allowing some control via the kptr_restrict sysctl. Exposing addresses
gives attackers sensitive information about the kernel layout in memory.

We can reduce the attack surface by hashing all addresses printed with
%p. This will of course break some users, forcing code printing needed
addresses to be updated.

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

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

Add function ptr_to_id() to map an address to a 32 bit unique identifier.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---

V5:
 - Remove spin lock.
 - Add Jason A. Donenfeld to CC list by request.
 - Add Theodore Ts'o to CC list due to comment on previous version.

V4:
 - Remove changes to siphash.{ch}
 - Do word size check, and return value cast, directly in ptr_to_id().
 - Use add_ready_random_callback() to guard call to get_random_bytes()

V3:
 - Use atomic_xchg() to guard setting [random] key.
 - Remove erroneous white space change.

V2:
 - Use SipHash to do the hashing.

The discussion related to this patch has been fragmented. There are
three threads associated with this patch. Email threads by subject:

[PATCH] printk: hash addresses printed with %p
[PATCH 0/3] add %pX specifier
[kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options

 lib/vsprintf.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 63 insertions(+), 3 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86c3385b9eb3..14d4c6653384 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -33,6 +33,7 @@
 #include <linux/uuid.h>
 #include <linux/of.h>
 #include <net/addrconf.h>
+#include <linux/siphash.h>
 #ifdef CONFIG_BLOCK
 #include <linux/blkdev.h>
 #endif
@@ -1591,6 +1592,63 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
+static siphash_key_t ptr_secret __read_mostly;
+static atomic_t have_key = ATOMIC_INIT(0);
+
+static void initialize_ptr_secret(void)
+{
+	if (atomic_read(&have_key) == 1)
+		return;
+
+	get_random_bytes(&ptr_secret, sizeof(ptr_secret));
+	atomic_set(&have_key, 1);
+}
+
+static void schedule_async_key_init(struct random_ready_callback *rdy)
+{
+	initialize_ptr_secret();
+}
+
+/* Maps a pointer to a 32 bit unique identifier. */
+static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
+{
+	static struct random_ready_callback random_ready;
+	unsigned int hashval;
+	int err;
+
+	if (atomic_read(&have_key) == 0) {
+		random_ready.owner = NULL;
+		random_ready.func = schedule_async_key_init;
+
+		err = add_random_ready_callback(&random_ready);
+
+		switch (err) {
+		case 0:
+			return "(pointer value)";
+
+		case -EALREADY:
+			initialize_ptr_secret();
+			break;
+
+		default:
+			/* shouldn't get here */
+			return "(ptr_to_id() error)";
+		}
+	}
+
+#ifdef CONFIG_64BIT
+	hashval = (unsigned int)siphash_1u64((u64)ptr, &ptr_secret);
+#else
+	hashval = (unsigned int)siphash_1u32((u32)ptr, &ptr_secret);
+#endif
+
+	spec.field_width = 2 + 2 * sizeof(unsigned int); /* 0x + hex */
+	spec.flags = SPECIAL | SMALL | ZEROPAD;
+	spec.base = 16;
+
+	return number(buf, end, hashval, spec);
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1703,6 +1761,9 @@ int kptr_restrict __read_mostly;
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a
  * pointer to the real address.
+ *
+ * Default behaviour (unadorned %p) is to hash the address, rendering it useful
+ * as a unique identifier.
  */
 static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
@@ -1858,14 +1919,13 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			return device_node_string(buf, end, ptr, spec, fmt + 1);
 		}
 	}
-	spec.flags |= SMALL;
+
 	if (spec.field_width == -1) {
 		spec.field_width = default_width;
 		spec.flags |= ZEROPAD;
 	}
-	spec.base = 16;
 
-	return number(buf, end, (unsigned long) ptr, spec);
+	return ptr_to_id(buf, end, ptr, spec);
 }
 
 /*
-- 
2.7.4

Re: [PATCH v5] printk: hash addresses printed with %p

[Kees Cook]

On Wed, Oct 18, 2017 at 2:30 PM, Tobin C. Harding <me@tobin.cc> wrote:
> Currently there are many places in the kernel where addresses are being
> printed using an unadorned %p. Kernel pointers should be printed using
> %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> gives attackers sensitive information about the kernel layout in memory.

Is it intended for %pK to be covered by the hash as well? (When a
disallowed user is looking at %pK output, like kallsyms, the same hash
is seen for all values, rather than just zero -- I assume since the
value hashed is zero.)

> We can reduce the attack surface by hashing all addresses printed with
> %p. This will of course break some users, forcing code printing needed
> addresses to be updated.
>
> For what it's worth, usage of unadorned %p can be broken down as
> follows (thanks to Joe Perches).
>
> $ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
>    1084 arch
>      20 block
>      10 crypto
>      32 Documentation
>    8121 drivers
>    1221 fs
>     143 include
>     101 kernel
>      69 lib
>     100 mm
>    1510 net
>      40 samples
>       7 scripts
>      11 security
>     166 sound
>     152 tools
>       2 virt
>
> Add function ptr_to_id() to map an address to a 32 bit unique identifier.
>
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> ---
>
> V5:
>  - Remove spin lock.
>  - Add Jason A. Donenfeld to CC list by request.
>  - Add Theodore Ts'o to CC list due to comment on previous version.
>
> V4:
>  - Remove changes to siphash.{ch}
>  - Do word size check, and return value cast, directly in ptr_to_id().
>  - Use add_ready_random_callback() to guard call to get_random_bytes()
>
> V3:
>  - Use atomic_xchg() to guard setting [random] key.
>  - Remove erroneous white space change.
>
> V2:
>  - Use SipHash to do the hashing.
>
> The discussion related to this patch has been fragmented. There are
> three threads associated with this patch. Email threads by subject:
>
> [PATCH] printk: hash addresses printed with %p
> [PATCH 0/3] add %pX specifier
> [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options
>
>  lib/vsprintf.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 63 insertions(+), 3 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 86c3385b9eb3..14d4c6653384 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -33,6 +33,7 @@
>  #include <linux/uuid.h>
>  #include <linux/of.h>
>  #include <net/addrconf.h>
> +#include <linux/siphash.h>
>  #ifdef CONFIG_BLOCK
>  #include <linux/blkdev.h>
>  #endif
> @@ -1591,6 +1592,63 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
>         return widen_string(buf, buf - buf_start, end, spec);
>  }
>
> +static siphash_key_t ptr_secret __read_mostly;
> +static atomic_t have_key = ATOMIC_INIT(0);
> +
> +static void initialize_ptr_secret(void)
> +{
> +       if (atomic_read(&have_key) == 1)
> +               return;
> +
> +       get_random_bytes(&ptr_secret, sizeof(ptr_secret));
> +       atomic_set(&have_key, 1);
> +}
> +
> +static void schedule_async_key_init(struct random_ready_callback *rdy)
> +{
> +       initialize_ptr_secret();
> +}
> +
> +/* Maps a pointer to a 32 bit unique identifier. */
> +static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
> +{
> +       static struct random_ready_callback random_ready;
> +       unsigned int hashval;
> +       int err;
> +
> +       if (atomic_read(&have_key) == 0) {
> +               random_ready.owner = NULL;
> +               random_ready.func = schedule_async_key_init;
> +
> +               err = add_random_ready_callback(&random_ready);
> +
> +               switch (err) {
> +               case 0:
> +                       return "(pointer value)";
> +
> +               case -EALREADY:
> +                       initialize_ptr_secret();
> +                       break;
> +
> +               default:
> +                       /* shouldn't get here */
> +                       return "(ptr_to_id() error)";
> +               }
> +       }
> +
> +#ifdef CONFIG_64BIT
> +       hashval = (unsigned int)siphash_1u64((u64)ptr, &ptr_secret);
> +#else
> +       hashval = (unsigned int)siphash_1u32((u32)ptr, &ptr_secret);
> +#endif
> +
> +       spec.field_width = 2 + 2 * sizeof(unsigned int); /* 0x + hex */
> +       spec.flags = SPECIAL | SMALL | ZEROPAD;

I don't think this should have SPECIAL. We end up changing things like
kallsyms (which didn't have 0x before) and printing with double 0x's:

        seq_printf(m, " 0x%pK", mod->core_layout.base);
...
# cat /proc/modules
test_module 16384 0 - Live 0x0xdf81cfb6

> +       spec.base = 16;
> +
> +       return number(buf, end, hashval, spec);
> +}
> +
>  int kptr_restrict __read_mostly;
>
>  /*
> @@ -1703,6 +1761,9 @@ int kptr_restrict __read_mostly;
>   * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
>   * function pointers are really function descriptors, which contain a
>   * pointer to the real address.
> + *
> + * Default behaviour (unadorned %p) is to hash the address, rendering it useful
> + * as a unique identifier.
>   */
>  static noinline_for_stack
>  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> @@ -1858,14 +1919,13 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>                         return device_node_string(buf, end, ptr, spec, fmt + 1);
>                 }
>         }
> -       spec.flags |= SMALL;
> +
>         if (spec.field_width == -1) {
>                 spec.field_width = default_width;
>                 spec.flags |= ZEROPAD;
>         }
> -       spec.base = 16;
>
> -       return number(buf, end, (unsigned long) ptr, spec);
> +       return ptr_to_id(buf, end, ptr, spec);
>  }
>
>  /*
> --
> 2.7.4
>

Getting closer! Thanks for continuing to work on it. :)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v5] printk: hash addresses printed with %p

[Tobin C. Harding]

On Wed, Oct 18, 2017 at 03:31:16PM -0700, Kees Cook wrote:
> On Wed, Oct 18, 2017 at 2:30 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > Currently there are many places in the kernel where addresses are being
> > printed using an unadorned %p. Kernel pointers should be printed using
> > %pK allowing some control via the kptr_restrict sysctl. Exposing addresses
> > gives attackers sensitive information about the kernel layout in memory.
> 
> Is it intended for %pK to be covered by the hash as well? (When a
> disallowed user is looking at %pK output, like kallsyms, the same hash
> is seen for all values, rather than just zero -- I assume since the
> value hashed is zero.)

Good catch, thanks. Have fixed for v6, will wait 24 hours before submitting.

> > We can reduce the attack surface by hashing all addresses printed with
> > %p. This will of course break some users, forcing code printing needed
> > addresses to be updated.
> >
> > For what it's worth, usage of unadorned %p can be broken down as
> > follows (thanks to Joe Perches).
> >
> > $ git grep -E '%p[^A-Za-z0-9]' | cut -f1 -d"/" | sort | uniq -c
> >    1084 arch
> >      20 block
> >      10 crypto
> >      32 Documentation
> >    8121 drivers
> >    1221 fs
> >     143 include
> >     101 kernel
> >      69 lib
> >     100 mm
> >    1510 net
> >      40 samples
> >       7 scripts
> >      11 security
> >     166 sound
> >     152 tools
> >       2 virt
> >
> > Add function ptr_to_id() to map an address to a 32 bit unique identifier.
> >
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > ---
> >
> > V5:
> >  - Remove spin lock.
> >  - Add Jason A. Donenfeld to CC list by request.
> >  - Add Theodore Ts'o to CC list due to comment on previous version.
> >
> > V4:
> >  - Remove changes to siphash.{ch}
> >  - Do word size check, and return value cast, directly in ptr_to_id().
> >  - Use add_ready_random_callback() to guard call to get_random_bytes()
> >
> > V3:
> >  - Use atomic_xchg() to guard setting [random] key.
> >  - Remove erroneous white space change.
> >
> > V2:
> >  - Use SipHash to do the hashing.
> >
> > The discussion related to this patch has been fragmented. There are
> > three threads associated with this patch. Email threads by subject:
> >
> > [PATCH] printk: hash addresses printed with %p
> > [PATCH 0/3] add %pX specifier
> > [kernel-hardening] [RFC V2 0/6] add more kernel pointer filter options
> >
> >  lib/vsprintf.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 63 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 86c3385b9eb3..14d4c6653384 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -33,6 +33,7 @@
> >  #include <linux/uuid.h>
> >  #include <linux/of.h>
> >  #include <net/addrconf.h>
> > +#include <linux/siphash.h>
> >  #ifdef CONFIG_BLOCK
> >  #include <linux/blkdev.h>
> >  #endif
> > @@ -1591,6 +1592,63 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
> >         return widen_string(buf, buf - buf_start, end, spec);
> >  }
> >
> > +static siphash_key_t ptr_secret __read_mostly;
> > +static atomic_t have_key = ATOMIC_INIT(0);
> > +
> > +static void initialize_ptr_secret(void)
> > +{
> > +       if (atomic_read(&have_key) == 1)
> > +               return;
> > +
> > +       get_random_bytes(&ptr_secret, sizeof(ptr_secret));
> > +       atomic_set(&have_key, 1);
> > +}
> > +
> > +static void schedule_async_key_init(struct random_ready_callback *rdy)
> > +{
> > +       initialize_ptr_secret();
> > +}
> > +
> > +/* Maps a pointer to a 32 bit unique identifier. */
> > +static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
> > +{
> > +       static struct random_ready_callback random_ready;
> > +       unsigned int hashval;
> > +       int err;
> > +
> > +       if (atomic_read(&have_key) == 0) {
> > +               random_ready.owner = NULL;
> > +               random_ready.func = schedule_async_key_init;
> > +
> > +               err = add_random_ready_callback(&random_ready);
> > +
> > +               switch (err) {
> > +               case 0:
> > +                       return "(pointer value)";
> > +
> > +               case -EALREADY:
> > +                       initialize_ptr_secret();
> > +                       break;
> > +
> > +               default:
> > +                       /* shouldn't get here */
> > +                       return "(ptr_to_id() error)";
> > +               }
> > +       }
> > +
> > +#ifdef CONFIG_64BIT
> > +       hashval = (unsigned int)siphash_1u64((u64)ptr, &ptr_secret);
> > +#else
> > +       hashval = (unsigned int)siphash_1u32((u32)ptr, &ptr_secret);
> > +#endif
> > +
> > +       spec.field_width = 2 + 2 * sizeof(unsigned int); /* 0x + hex */
> > +       spec.flags = SPECIAL | SMALL | ZEROPAD;
> 
> I don't think this should have SPECIAL. We end up changing things like
> kallsyms (which didn't have 0x before) and printing with double 0x's:

While on the topic, have you an opinion on whether SMALL is good here. My first thought was that
capitals _kind_of_ showed that it was an ID not an address, later contemplation made me think this
may only have meaning to myself from working on the patch so better to leave it SMALL like original.

Any thoughts appreciated.

> 
>         seq_printf(m, " 0x%pK", mod->core_layout.base);
> ...
> # cat /proc/modules
> test_module 16384 0 - Live 0x0xdf81cfb6
> 
> > +       spec.base = 16;
> > +
> > +       return number(buf, end, hashval, spec);
> > +}
> > +
> >  int kptr_restrict __read_mostly;
> >
> >  /*
> > @@ -1703,6 +1761,9 @@ int kptr_restrict __read_mostly;
> >   * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
> >   * function pointers are really function descriptors, which contain a
> >   * pointer to the real address.
> > + *
> > + * Default behaviour (unadorned %p) is to hash the address, rendering it useful
> > + * as a unique identifier.
> >   */
> >  static noinline_for_stack
> >  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> > @@ -1858,14 +1919,13 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> >                         return device_node_string(buf, end, ptr, spec, fmt + 1);
> >                 }
> >         }
> > -       spec.flags |= SMALL;
> > +
> >         if (spec.field_width == -1) {
> >                 spec.field_width = default_width;
> >                 spec.flags |= ZEROPAD;
> >         }
> > -       spec.base = 16;
> >
> > -       return number(buf, end, (unsigned long) ptr, spec);
> > +       return ptr_to_id(buf, end, ptr, spec);
> >  }
> >
> >  /*
> > --
> > 2.7.4
> >
> 
> Getting closer! Thanks for continuing to work on it. :)

We are having fun here! Cheers Kees.

Tobin.

Re: [PATCH v5] printk: hash addresses printed with %p

[Jason A. Donenfeld]

On Wed, Oct 18, 2017 at 11:30 PM, Tobin C. Harding <me@tobin.cc> wrote:
> +static siphash_key_t ptr_secret __read_mostly;
> +static atomic_t have_key = ATOMIC_INIT(0);
> +
> +static void initialize_ptr_secret(void)
> +{
> +       if (atomic_read(&have_key) == 1)
> +               return;
> +
> +       get_random_bytes(&ptr_secret, sizeof(ptr_secret));
> +       atomic_set(&have_key, 1);
> +}

> +               case -EALREADY:
> +                       initialize_ptr_secret();
> +                       break;

Unfortunately the above is racy, and the spinlock you had before was
actually correct (though using an atomic inside a spinlock wasn't
strictly necessary). The race is that two callers might hit
initialize_ptr_secret at the same time, and have_key will be zero at
the beginning for both. Then they'll both scribble over ptr_secret,
and might wind up using a different value after if one finishes before
the other. I see two ways of correcting this:

1) Go back to the spinlock yourself.
2) Use get_random_bytes_once(&ptr_secret, sizeof(ptr_secret)). I don't
know lib/once.c especially well, but from cursory look, it appears to
be taking a spinlock too, which means you're probably good.


+       if (atomic_read(&have_key) == 0) {
+               random_ready.owner = NULL;
+               random_ready.func = schedule_async_key_init;

You can probably take care of this part in the initialization:

static struct random_ready_callback random_ready = {
        .func = schedule_async_key_init
};

Alternatively, you could put the actual call to
add_random_ready_callback in an init function, but maybe how you have
it is easier.


Jason

Re: [PATCH v5] printk: hash addresses printed with %p

[Sergey Senozhatsky]

On (10/19/17 03:03), Jason A. Donenfeld wrote:
[..]
> 1) Go back to the spinlock yourself.

so we ruled out NMI deadlocks?

	-ss

Re: [PATCH v5] printk: hash addresses printed with %p

[Jason A. Donenfeld]

On Thu, Oct 19, 2017 at 3:31 AM, Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
> On (10/19/17 03:03), Jason A. Donenfeld wrote:
> [..]
>> 1) Go back to the spinlock yourself.
>
> so we ruled out NMI deadlocks?

Oh, right. No, I haven't thought through this enough to rule it out.
Indeed if that's an issue, the locks in the _once code will also be an
issue.

So if locking is totally impossible, then a race-free way of doing
this is with a tri-state compare and exchange. Things are either: in
state 1: no key, state 2: getting key, state 3: have key. If state 1
or 2, print the placeholder token. If state 3, do the hashing.

Re: [PATCH v5] printk: hash addresses printed with %p

[Tobin C. Harding]

On Thu, Oct 19, 2017 at 03:36:20AM +0200, Jason A. Donenfeld wrote:
> On Thu, Oct 19, 2017 at 3:31 AM, Sergey Senozhatsky
> <sergey.senozhatsky.work@gmail.com> wrote:
> > On (10/19/17 03:03), Jason A. Donenfeld wrote:
> > [..]
> >> 1) Go back to the spinlock yourself.
> >
> > so we ruled out NMI deadlocks?
> 
> Oh, right. No, I haven't thought through this enough to rule it out.
> Indeed if that's an issue, the locks in the _once code will also be an
> issue.
> 
> So if locking is totally impossible, then a race-free way of doing
> this is with a tri-state compare and exchange. Things are either: in
> state 1: no key, state 2: getting key, state 3: have key. If state 1
> or 2, print the placeholder token. If state 3, do the hashing.

Cool! That's the solution I've been looking for since day 1. You the man.

thanks,
Tobin.

Re: [PATCH v5] printk: hash addresses printed with %p

[Jason A. Donenfeld]

A small detail carried over from the other thread:

>
> but a bigger problem might the following thing:
>
> vscnprintf()
>  pointer()
>   ptr_to_id()
>    initialize_ptr_secret()
>     get_random_bytes()
>      _get_random_bytes()
>       extract_crng()
>        _extract_crng()
>         spin_lock_irqsave(&crng->lock, flags);   <<<<<
>
>
> this, once again, can deadlock. can it? just like before:

So, actually, then, we need to do this as an initcall. Fortunately,
that simplifies things greatly. Here's a rough sketch of what that
looks like, which you'll probably need to debug and refine:



static siphash_key_t ptr_secret __ro_after_init;
static DEFINE_STATIC_KEY_TRUE(no_ptr_secret);

static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
{
    if (static_branch_unlikely(&no_ptr_secret))
        return "(pointer value)";

    hashval = ....

}

static void fill_random_ptr_key(struct random_ready_callback *rdy)
{
    get_random_bytes(&ptr_secret, sizeof(ptr_secret));
    static_branch_disable(&no_ptr_secret);
}

static struct random_ready_callback random_ready = {
    .func = fill_random_ptr_key
};

static int __init initialize_ptr_random(void)
{
    int ret = add_random_ready_callback(&random_ready);

    if (!ret)
        return 0;
    else if (ret == -EALREADY) {
        fill_random_ptr_key(&random_ready);
        return 0;
    }

    return ret;
}
early_initcall(initialize_ptr_random);

Re: [PATCH v5] printk: hash addresses printed with %p

[Kees Cook]

On Wed, Oct 18, 2017 at 10:49 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> static void fill_random_ptr_key(struct random_ready_callback *rdy)
> {
>     get_random_bytes(&ptr_secret, sizeof(ptr_secret));
>     static_branch_disable(&no_ptr_secret);
> }
>
> static struct random_ready_callback random_ready = {
>     .func = fill_random_ptr_key
> };
>
> static int __init initialize_ptr_random(void)
> {
>     int ret = add_random_ready_callback(&random_ready);
>
>     if (!ret)
>         return 0;
>     else if (ret == -EALREADY) {
>         fill_random_ptr_key(&random_ready);
>         return 0;
>     }
>
>     return ret;
> }
> early_initcall(initialize_ptr_random);

Tangent: why is the random_ready API designed with -EALREADY? Couldn't
add_random_ready_callback() just perform the call itself and avoid
needing all the callers to check for -EALREADY?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v5] printk: hash addresses printed with %p

[Jason A. Donenfeld]

> Tangent: why is the random_ready API designed with -EALREADY? Couldn't
> add_random_ready_callback() just perform the call itself and avoid
> needing all the callers to check for -EALREADY?

Absolutely. I can submit a patch for this soon, though to avoid merge
dependencies, and given the usual delays in getting random.c things
merged, maybe I'll wait until after this patch hits Linus' tree.

This could even be renamed and made into an LD section, as
random_initcall, though that needs some more spec'ing out.

[PATCH 1/2] random: always call random ready function

[Jason A. Donenfeld]

As this interface becomes more heavily used, it will be painful for
callers to always need to check for -EALREADY.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 8ad92707e45f..e161acf35d4a 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1556,40 +1556,45 @@ EXPORT_SYMBOL(wait_for_random_bytes);
 
 /*
  * Add a callback function that will be invoked when the nonblocking
- * pool is initialised.
+ * pool is initialised, or calls that function if it already is.
  *
  * returns: 0 if callback is successfully added
- *	    -EALREADY if pool is already initialised (callback not called)
  *	    -ENOENT if module for callback is not alive
  */
 int add_random_ready_callback(struct random_ready_callback *rdy)
 {
 	struct module *owner;
 	unsigned long flags;
-	int err = -EALREADY;
 
-	if (crng_ready())
-		return err;
+	BUG_ON(!rdy->func);
+
+	if (crng_ready()) {
+		rdy->func(rdy);
+		rdy->func = NULL;
+		return 0;
+	}
 
 	owner = rdy->owner;
 	if (!try_module_get(owner))
 		return -ENOENT;
 
 	spin_lock_irqsave(&random_ready_list_lock, flags);
-	if (crng_ready())
+	if (crng_ready()) {
+		rdy->func(rdy);
+		rdy->func = NULL;
 		goto out;
+	}
 
 	owner = NULL;
 
 	list_add(&rdy->list, &random_ready_list);
-	err = 0;
 
 out:
 	spin_unlock_irqrestore(&random_ready_list_lock, flags);
 
 	module_put(owner);
 
-	return err;
+	return 0;
 }
 EXPORT_SYMBOL(add_random_ready_callback);
 
@@ -1601,6 +1606,9 @@ void del_random_ready_callback(struct random_ready_callback *rdy)
 	unsigned long flags;
 	struct module *owner = NULL;
 
+	if (!rdy->func)
+		return;
+
 	spin_lock_irqsave(&random_ready_list_lock, flags);
 	if (!list_empty(&rdy->list)) {
 		list_del_init(&rdy->list);
-- 
2.14.2

[PATCH 2/2] crypto/drbg: account for no longer returning -EALREADY

[Jason A. Donenfeld]

We now structure things in a way that assumes the seeding function is
always eventually called.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 crypto/drbg.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 70018397e59a..501e013cb96c 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1411,18 +1411,8 @@ static int drbg_prepare_hrng(struct drbg_state *drbg)
 
 	err = add_random_ready_callback(&drbg->random_ready);
 
-	switch (err) {
-	case 0:
-		break;
-
-	case -EALREADY:
-		err = 0;
-		/* fall through */
-
-	default:
-		drbg->random_ready.func = NULL;
+	if (err)
 		return err;
-	}
 
 	drbg->jent = crypto_alloc_rng("jitterentropy_rng", 0, 0);
 
@@ -1432,7 +1422,7 @@ static int drbg_prepare_hrng(struct drbg_state *drbg)
 	 */
 	drbg->reseed_threshold = 50;
 
-	return err;
+	return 0;
 }
 
 /*
@@ -1526,9 +1516,9 @@ static int drbg_instantiate(struct drbg_state *drbg, struct drbg_string *pers,
  */
 static int drbg_uninstantiate(struct drbg_state *drbg)
 {
-	if (drbg->random_ready.func) {
-		del_random_ready_callback(&drbg->random_ready);
-		cancel_work_sync(&drbg->seed_work);
+	del_random_ready_callback(&drbg->random_ready);
+	cancel_work_sync(&drbg->seed_work);
+	if (drbg->jent) {
 		crypto_free_rng(drbg->jent);
 		drbg->jent = NULL;
 	}
-- 
2.14.2

Re: [PATCH 2/2] crypto/drbg: account for no longer returning -EALREADY

[Stephan Mueller]

Am Donnerstag, 19. Oktober 2017, 22:45:06 CEST schrieb Jason A. Donenfeld:

Hi Jason,

> We now structure things in a way that assumes the seeding function is
> always eventually called.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  crypto/drbg.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/crypto/drbg.c b/crypto/drbg.c
> index 70018397e59a..501e013cb96c 100644
> --- a/crypto/drbg.c
> +++ b/crypto/drbg.c
> @@ -1411,18 +1411,8 @@ static int drbg_prepare_hrng(struct drbg_state *drbg)
> 
>  	err = add_random_ready_callback(&drbg->random_ready);
> 
> -	switch (err) {
> -	case 0:
> -		break;
> -
> -	case -EALREADY:
> -		err = 0;
> -		/* fall through */
> -
> -	default:
> -		drbg->random_ready.func = NULL;
> +	if (err)
>  		return err;

Don't you change the logic flow here? In case the add_random_ready_callback 
returns 0 because the ready function is already called due to crng_ready(), 
the new code above in the patch set continues. But with the current code, it 
will return at this point and do not perform the allocation below.



Ciao
Stephan

Re: [PATCH 1/2] random: always call random ready function

[Jason A. Donenfeld]

These are mostly untested, but I wanted to spec it out for a taste of
what it looks like.

Re: [PATCH 1/2] random: always call random ready function

[Kees Cook]

On Thu, Oct 19, 2017 at 1:45 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> As this interface becomes more heavily used, it will be painful for
> callers to always need to check for -EALREADY.
>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  drivers/char/random.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 8ad92707e45f..e161acf35d4a 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1556,40 +1556,45 @@ EXPORT_SYMBOL(wait_for_random_bytes);
>
>  /*
>   * Add a callback function that will be invoked when the nonblocking
> - * pool is initialised.
> + * pool is initialised, or calls that function if it already is.
>   *
>   * returns: 0 if callback is successfully added
> - *         -EALREADY if pool is already initialised (callback not called)
>   *         -ENOENT if module for callback is not alive
>   */
>  int add_random_ready_callback(struct random_ready_callback *rdy)
>  {
>         struct module *owner;
>         unsigned long flags;
> -       int err = -EALREADY;
>
> -       if (crng_ready())
> -               return err;
> +       BUG_ON(!rdy->func);

Better to WARN_ON() and return a failure.

> +
> +       if (crng_ready()) {
> +               rdy->func(rdy);
> +               rdy->func = NULL;
> +               return 0;
> +       }
>
>         owner = rdy->owner;
>         if (!try_module_get(owner))
>                 return -ENOENT;
>
>         spin_lock_irqsave(&random_ready_list_lock, flags);
> -       if (crng_ready())
> +       if (crng_ready()) {
> +               rdy->func(rdy);
> +               rdy->func = NULL;
>                 goto out;
> +       }
>
>         owner = NULL;
>
>         list_add(&rdy->list, &random_ready_list);
> -       err = 0;
>
>  out:
>         spin_unlock_irqrestore(&random_ready_list_lock, flags);
>
>         module_put(owner);
>
> -       return err;
> +       return 0;
>  }
>  EXPORT_SYMBOL(add_random_ready_callback);
>
> @@ -1601,6 +1606,9 @@ void del_random_ready_callback(struct random_ready_callback *rdy)
>         unsigned long flags;
>         struct module *owner = NULL;
>
> +       if (!rdy->func)
> +               return;

Perhaps add a note here about what a NULL func means here? (e.g.
"Already run, not in the list.")

> +
>         spin_lock_irqsave(&random_ready_list_lock, flags);
>         if (!list_empty(&rdy->list)) {
>                 list_del_init(&rdy->list);
> --
> 2.14.2
>

Otherwise, yeah, looks sensible.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 1/2] random: always call random ready function

[Jason A. Donenfeld]

Good tips, thanks. I'll wait for more comments before resubmitting v2,
but in-progress changes live here:
https://git.zx2c4.com/linux-dev/log/?h=jd/cleaner-add-random-ready

Re: [PATCH v5] printk: hash addresses printed with %p

[Tobin C. Harding]

On Thu, Oct 19, 2017 at 07:49:06AM +0200, Jason A. Donenfeld wrote:
> A small detail carried over from the other thread:
> 
> >
> > but a bigger problem might the following thing:
> >
> > vscnprintf()
> >  pointer()
> >   ptr_to_id()
> >    initialize_ptr_secret()
> >     get_random_bytes()
> >      _get_random_bytes()
> >       extract_crng()
> >        _extract_crng()
> >         spin_lock_irqsave(&crng->lock, flags);   <<<<<
> >
> >
> > this, once again, can deadlock. can it? just like before:
> 
> So, actually, then, we need to do this as an initcall. Fortunately,
> that simplifies things greatly. Here's a rough sketch of what that
> looks like, which you'll probably need to debug and refine:
> 
> 
> 
> static siphash_key_t ptr_secret __ro_after_init;
> static DEFINE_STATIC_KEY_TRUE(no_ptr_secret);
> 
> static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec)
> {
>     if (static_branch_unlikely(&no_ptr_secret))
>         return "(pointer value)";
> 
>     hashval = ....
> 
> }
> 
> static void fill_random_ptr_key(struct random_ready_callback *rdy)
> {
>     get_random_bytes(&ptr_secret, sizeof(ptr_secret));
>     static_branch_disable(&no_ptr_secret);
> }
> 
> static struct random_ready_callback random_ready = {
>     .func = fill_random_ptr_key
> };
> 
> static int __init initialize_ptr_random(void)
> {
>     int ret = add_random_ready_callback(&random_ready);
> 
>     if (!ret)
>         return 0;
>     else if (ret == -EALREADY) {
>         fill_random_ptr_key(&random_ready);
>         return 0;
>     }
> 
>     return ret;
> }
> early_initcall(initialize_ptr_random);

Thanks for this Jason. This is _conceptually_ what I wanted since before v1, I obviously did not ask
the right questions. Not to worry, we got there in the end. The process works, thanks to every
bodies patience :)

Implemented for v6 as suggested (including __read_mostly), you even got it fast for the usual
case. Thanks, I learned a whole bunch from this email.

Tobin.